Increased memory consumption as tracks are switched about strawberry

[guprobr]

I'm sorry for so much flooding and controversial reports. I have finally narrowed down the problem, thanks to your informations, its the covers. I disabled notifications, the playing widget and did this:

void MainWindow::AlbumCoverLoaded(const Song &song, const AlbumCoverLoaderResult &result) {

  if (song != song_playing_) return;
/*
  song_ = song;
  album_cover_ = result.album_cover;

  emit AlbumCoverReady(song, result.album_cover.image);

  const bool enable_change_art = song.is_collection_song() && !song.effective_albumartist().isEmpty() && !song.album().isEmpty();
  album_cover_choice_controller_->show_cover_action()->setEnabled(result.success && result.type != AlbumCoverLoaderResult::Type::Unset);
  album_cover_choice_controller_->cover_to_file_action()->setEnabled(result.success && result.type != AlbumCoverLoaderResult::Type::Unset);
  album_cover_choice_controller_->cover_from_file_action()->setEnabled(enable_change_art);
  album_cover_choice_controller_->cover_from_url_action()->setEnabled(enable_change_art);
  album_cover_choice_controller_->search_for_cover_action()->setEnabled(app_->cover_providers()->HasAnyProviders() && enable_change_art);
  album_cover_choice_controller_->unset_cover_action()->setEnabled(enable_change_art && !song.art_unset());
  album_cover_choice_controller_->clear_cover_action()->setEnabled(enable_change_art && !song.art_manual().isEmpty());
  album_cover_choice_controller_->delete_cover_action()->setEnabled(enable_change_art && result.success && result.type != AlbumCoverLoaderResult::Type::Unset);

  GetCoverAutomatically(); */

}

Tested A LOT and there is no incremental memory when switching music, only the expected memory of the size of the song, obviously.

I'm learning a lot with this code and I'm very grateful for your explanations, unfortunately I'm still very newbie in C++, Qt and gStreamer programming, but I find amazing to have a chance to learn so much and even perphaps help a bit.

[guprobr]

This seems to improve A LOT of the memory growth, it diminishes a LOT! :D

I think is a pretty good hint: I've observed a significant reduction in memory growth after commenting out the fading functionality in SetImage(). This suggests that fading is a major contributor to the memory issue of album covers.

It's crucial to test with multiple tracks to assess the impact accurately. While it may appear that memory initially increases, it eventually stabilizes and doesn't exhibit continuous growth. This behavior differs from previous observations where memory constantly grew, and much faster, in much larger increments.

Before commenting out the fading, I did some other changes like passing "image" to SetImage() as a pointer, but I don't think its necessary and it was a lot of work for absolutely no improvement. The way I see it, the main reason COVERS are causing growth of memory, is the FADING effect, because of that shared pointer. I believe its not releasing it and it accumulates on the heap. Besides other things that may do too, I think anyway its such little memory compared to the images covers that I don't think it would be necessary to debug that.

When I comment these lines below, I can't see breaking any functionality. Only basically fixing the memory growth, but not fixing it completely unfortunately. (?)

void ContextAlbum::SetImage(QImage image) {

  if (image.isNull()) {
    image = image_strawberry_;
  }

  if (downloading_covers_) {
    downloading_covers_ = false;
    spinner_animation_.reset();
  }

  QImage image_previous = image_original_;
  QPixmap pixmap_previous = pixmap_current_;
  //qreal opacity_previous = pixmap_current_opacity_;

  image_original_ = image;
  pixmap_current_opacity_ = 0.0;
  ScaleCover();

  /*if (!pixmap_previous.isNull()) {
    SharedPtr<PreviousCover> previous_cover = make_shared<PreviousCover>();
    previous_cover->image = image_previous;
    previous_cover->pixmap =  pixmap_previous;
    previous_cover->opacity = opacity_previous;
    previous_cover->timeline.reset(new QTimeLine(kFadeTimeLineMs), [](QTimeLine *timeline) { timeline->deleteLater(); });
    previous_cover->timeline->setDirection(QTimeLine::Backward);
    previous_cover->timeline->setCurrentTime(timeline_fade_->state() == QTimeLine::Running ? timeline_fade_->currentTime() : kFadeTimeLineMs);
    QObject::connect(&*previous_cover->timeline, &QTimeLine::valueChanged, this, [this, previous_cover]() { FadePreviousCover(previous_cover); });
    QObject::connect(&*previous_cover->timeline, &QTimeLine::finished, this, [this, previous_cover]() { FadePreviousCoverFinished(previous_cover); });
    previous_covers_ << previous_cover;
    previous_cover->timeline->start();
  } 

  if (timeline_fade_->state() == QTimeLine::Running) {
    timeline_fade_->stop();
  }*/
  timeline_fade_->start();

}

[jonaski]

Thanks @guprobr, there is obviously a hug memory leak there!
The shared pointers are not deleting the memory in FadePreviousCoverFinished even though it's removed from the previous_covers_ list, because the signal/slot connection is still holding a copy of the shared pointer!
I'm embarrassed, but very grateful that you found this, I owe you. 70c2b99 should fix it.
I'd like to send you 100 USD, do you have Paypal?

[jonaski]

We need to look into if this pattern is used elsewhere in the codebase too, I hope not.

[jonaski]

I suggest to try heaptrack, it's very good: https://github.com/KDE/heaptrack

[jonaski]

Here is a very interesting article about memory fragmentation comparing different memory allocators: https://www.qt.io/blog/a-fast-and-thread-safe-pool-allocator-for-qt-part-1

[jonaski]

Closing this as the main issue regarding the leak is fixed. I don't see any more leaks, memory will grow when using the cover manager as it loads lots of covers and picks the best. But if you find anything more leaks, fixes are welcome of course.

[jonaski]

I'm pretty confident that there aren't any leaks in Strawberry, but if there are any it's more likely related to the GLib/gstreamer specific code than Qt.
I suggest to use heaptrack to track the memory usage.
But I can tell you for sure that a lot of memory growth in Strawberry is caused by passing album covers (QImages) through signal/slots all over the place, because in Strawberry the album cover handling is very complicated. the album cover loader is run in it's own thread to load cover from disk. I can reproduce this behavior also in a small test program, I wonder if it's possible to use a better memory allocator to fix that.

[guprobr]

Nice that you could solve the mistery, I kept thinking it was because removeAll() would not work with shared pointers! But I got close LoL

Actually, I still do have paypal!! Usually I would not even accept the donation but I'm unemployed and things are nasty!!!!! So I really appreciate LoL tks tks tks

[guprobr]

the fading of the OSD is very different, but its good to take a look as it uses timelines too

[guprobr]

LoL I'm going to share the script I've made to help debug the memory growth... but it can be used to any program, check the $1 $2 $3 params at the beginning...

#!/bin/bash

# Set process to monitor mem usage
STRAWGLER=${1:-"strawberry"};
# Set loop interval (in seconds)
INTERVAL=${2:-30}; 
# Set memory threshold (in percentage)
MEM_THRESHOLD=${3:-20};
export LC_ALL=C;

strawgler_main() {

# Loop continuously, restart process if reaches threshold!
while true; do
  # Get total RAM in Megabytes
  TOTAL_MEM=$(free -m | awk '/Mem:/{print $2}')
  # Get memory in MB used by the monitored process 
  STRAWGLER_MEM=$(ps -eO rss,fname | grep ${STRAWGLER}$ | awk '{sum+=$2} END {print sum/1024}')
  # Calculate memory usage percentage
  MEM_USAGE=$(echo "scale=2; 100 * $STRAWGLER_MEM / $TOTAL_MEM" | bc)
  echo "${STRAWGLER_MEM} MB (${MEM_USAGE}%)";
  ###notify-send -i ${STRAWGLER} -a ${STRAWGLER} "${STRAWGLER_MEM} MB ($MEM_USAGE%)";
  # Check if memory usage exceeds threshold
  if [[ $(echo "$MEM_USAGE >= $MEM_THRESHOLD" | bc -l) -eq 1 ]]; then
    echo "${STRAWGLER} memory usage ($MEM_USAGE%) exceeded threshold. Restarting..."
    notify-send -i ${STRAWGLER} -a ${STRAWGLER} "${STRAWGLER} mem: ($MEM_USAGE%)" "process Exceeded threshold. Restarting!";
    # Send HUP signal to gracefully restart process
    killall -HUP ${STRAWGLER};
    # Wait to allow shutdown
    sleep 1;
    # then Start "${STRAWGLER}" in background
    ${STRAWGLER} &
  fi

  # Wait some time before checking again
  sleep ${INTERVAL};

done

}

## Check if there is an instance already running
if [ $( pgrep -x "$( basename $0 )" | grep -v $$ | wc -l ) -gt 1 ]; then 
    echo "Tried to spawn more than one script, ¡there can be only one!";
    exit 1;
else
    strawgler_main ${1};
fi

[guprobr]

I did install but still have to figure how to use it LoL
I remember there was some kind of GUI mentioned in the --help

[guprobr]

wow just saw the screenshots looks veeeeeeeeeeeeeeeeeeeery cool

[guprobr]

I'm suspecting now there might be something in the moodbar code; by disabling moodbar on the settings, memory growth seems to decrease too!

[guprobr]

Another thing besides the moodbar I''m going to investigate, is the album cover online search. I've just unset a cover, clicked to find covers online to set the cover (in the context widget directly) - memory increased by 300 MB by finding six covers, and it does not garbage collect. The covers pixmaps seem to stay allocated. It increases by searching covers and then again when cover is set. I tried to set pixmap cache to 1MB only but looks like it got nothing to do with it lol