sskodje / GifImageSource

GifImageSource is a Windows Runtime Component for WinRT projects, made for rendering GIF images in a resource efficient way using Direct2D.
MIT License
15 stars 2 forks source link

Memory leak maybe. #4

Closed BurninBOB closed 8 years ago

BurninBOB commented 8 years ago

I ran into another issue and probably the last one ill have. I have noticed that my ram and cpu will skyrocket as i load more gifs even if it's the same gif. ill explain in the video.

https://1drv.ms/v/s!AuDOkuLnPbGpwPRYC0HpDSl0bGUP5Q

sskodje commented 8 years ago

Yeah, it definitely looks like you got a memory leak issue here.

The question is if this is a bug in the control or a bug in your app. The GifImageSource control should clear the GifImageSource when the parent image is unloaded. Perhaps there is something referring to your Image control that keeps it in scope, hence keeping the image source in scope?

Try put an "unloaded" event on your Image control, and see if it triggers when you close the view where the gif plays. You can also listen for the debug output of the control if you debug with native or mixed on. You should be able to see "Render task was canceled and stopped", "Unloaded image xxx with source: yyy" and "Destructed GifImageSource" roughly in that order. The "Destructed" might not be immediate, as the garbage collection does it's own thing, but the first two messages means the rendering has stopped, and that all the gifs resources has been cleared from memory.

BurninBOB commented 8 years ago

The unloded event seems to trigger but i cant see any information on the RoutedEventArgs. But i dont see "Render task was canceled and stopped" at all in the debug window.

BurninBOB commented 8 years ago

So after some playing around if i set AnimationBehavior.SetImageUriSource(MixImage, null); on OnNavigatingFrom the problem becomes a little less by dumping the ram but when i go back to the page with the image on it, it still seems to add the ram from the last one to it eventually getting larger and larger again. usually the null gets called when i set "SelectedMix" in the viewmodel to null navigating foreward to the image page before l load the new data. even when SetImageUriSource is set to null i dont see Render task was canceled and stopped", "Unloaded image xxx with source: yyy" or "Destructed GifImageSource" ever

gif:AnimationBehavior.ImageUriSource="{Binding SelectedMix.cover_urls.max1024}"

sskodje commented 8 years ago

I'm struggling a bit with recreating this. I made a page on the test app containing a single Image control loading a 30MB GIF image.

   <Image gs:AnimationBehavior.ImageUriSource="http://i.imgur.com/ZvSrgzm.gif" />

and opened it, went back, opened it etc repeatedly while tracking memory usage. The result looks like this. A slight increase in memory allocation, but nothing like the massive leak you are experiencing.

I'm not really sure where to go in tracking down the issue though. Perhaps you could try split out the barebone code necesssary for playing the gif and see if you can repeat it in a blank project. Don't forget you need to set the debugging type to native to see any output from the library.

BurninBOB commented 8 years ago

hmm ok well i created a blank project with code very similar to how i have my app. you will notice there is a viewmodel and each page has its datacontext set to the viewmodel. I setup the templates and other things that im using so that you can see how it works visually. it seems to also do the same thing for both pages. i also left notes so you know any differences with my app. I can at least see that it is destructing the gifs tho. for some reason i cant see that in the real app. you can get the project files here

sskodje commented 8 years ago

Thank you, i have managed to recreate the problem now, and i'll have a go at fixing it tonight

sskodje commented 8 years ago

Alright this was a troublesome one! I believe the problem is a combination of some small mistakes on your part, and some small mistakes on my part that allows it to happen.

First off, when you are setting your ObservableCollection to null and then recreating it, you are unbinding and rebinding the view, recreating the whole view twice. You should for performance reasons not null or recreate an observablecollection, but rather clear it as needed. This is not directly causing the issue, but it's related to the next one.

Then, when you are setting your properties in the ViewModel, you don't check for equality before you fire "onpropertychanged". This means that for every time you set that property with the same value, you trigger a refresh of the image, and the creation of a new GifImageSource instance. This was happening twice or thrice on each image in your code.

        set
        {
                _selectedgifobject = value;
                NotifyPropertyChanged("SelectedGifObject");
        }

should be

        set
        {
            if (_selectedgifobject!= value)
            {
                _selectedgifobject = value;
                NotifyPropertyChanged("SelectedGifObject");
            }
        }

Thirdly, when you run code in the "OnNavigatedTo" and "OnNavigatedFrom", you are actually running it before the view has been created. This is where i believe the heart of the issue lies. When you navigate to MainPage, before the view is created, and before the GifPage has been destroyed, you clear the ObservableCollection, causing the view to recreate, and since the Image is bound to a property of the collection, it also creates a new Image. Now when you go back to GifPage, there are two Images that are bound to the same property. One visible, and one staying without being connected to the visual tree, just soaking up memory. The next time there are three.

I'm not 100% sure on that's how the databinding issues occur, but that's what i've been able to deduce at least.

So what i believe is the cure:

following these steps i have found no more leaks in your example project.

I'll change the code to disallow running the GifImageSource in a control not attached to the visual tree and update it in the near future, but i believe these steps should fix your problems without.

BurninBOB commented 8 years ago

This makes allot of sense! I remember the day i removed the equality check from my collections thinking "Why do i need to do this" well now i know. it also makes sense about nulling them too when i navigate to the page on my app everything is set to a collapse visibility with a loading ring and then visible when completed because i was seeing the old information. ill make these changed in my app and report back. I thank so very much for this!

BurninBOB commented 8 years ago

Got everything changed over in the real app and works great on pages that have ObservableCollections. but the GifPage in my app does not make a difference. Changed the code from OnNavigatedTo to Loaded . In my app the image of the GifPage is not bound to the ObservableCollections it is its own object unrelated to the collection and the image is a string object in side of it. before i would null out the whole object. how should i go about fixing that or is that something that will be fixed when you update?

sskodje commented 8 years ago

I think i have found the issue. It's in the OnImageLoaded event, that keeps a reference to the GifImageSource, preventing it from disposing.

Can you try removing your subscription to that event, and see if that fixes your issues?

BurninBOB commented 8 years ago

Removed it but it didnt make any difference

sskodje commented 8 years ago

You shouldn't do this on release, but try put GC.Collect(); in your unloaded or navigatedfrom. It should force a garbage collection, and you will find out of it's really a leak or just stuff lingering .

I'm really out of ideas here :(

BurninBOB commented 8 years ago

I'm not really sure what to do with GC.Collect(); I've never used it but i just fixed it buy removing the binding and just seting the source uri in code behind. so i'm think it has something to do with the binding to the object in the viewmodel.

sskodje commented 8 years ago

How great that you figured out a workaround. I'll have to investigate a bit around this issue, as it seems some weird stuff is going on with garbage collection.

BurninBOB commented 8 years ago

Thanks for all your work! I'm going to be launching this app in the next few weeks and I would like to give you some credit and have something like Powered by GifImageSource in the "About" page. Maybe replace the "GifImageSource" text with a gif just to give it some flash. Is that alright with you?

sskodje commented 8 years ago

Sure, thank you, and good luck with your app!