Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] MediaElement is not currently being Garbage Collected #1702

Closed
2 tasks done
marco-skizza opened this issue Feb 20, 2024 · 9 comments · Fixed by #1736
Closed
2 tasks done

[BUG] MediaElement is not currently being Garbage Collected #1702

marco-skizza opened this issue Feb 20, 2024 · 9 comments · Fixed by #1736
Labels
bug Something isn't working 📽️ MediaElement Issue/PR that has to do with MediaElement unverified

Comments

@marco-skizza
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Did you read the "Reporting a bug" section on Contributing file?

Current Behavior

As it seems, the MediaElement doesn't ever get Garbage Collected.

Expected Behavior

The MediaElement should get Garbage Collected eventually, and with it the containing page.

Steps To Reproduce

  • Run the sample
  • Click the button to get to the page with the MediaElement and navigate back
  • Do this several times
  • In the Debug console observe, that ~MyPage() never gets called

Link to public reproduction project repository

https://github.com/marco-skizza/CommunityMauiMediaElement

Environment

- CommunityToolkit.Maui.MediaElement: 3.0.1
- OS: iPadOS 17.3.1 on an iPad mini 6
- .NET MAUI: 8.0.6

Anything else?

Thanks for looking at this!

@marco-skizza marco-skizza added bug Something isn't working unverified labels Feb 20, 2024
@TheCodeTraveler
Copy link
Collaborator

Hey @marco-skizza! This doesn't fix the bug, but there's a workaround where you should disconnect the handler when you're done using MediaElement to avoid memory leaks.

Here's a PR we recently merged that demonstrates how we're now doing this work-around in the SampleApp:
9c83cd4

@bijington - Do we have anything in our docs to note this? We totally should!

@bijington
Copy link
Contributor

@brminnick we do indeed:

https://learn.microsoft.com/en-gb/dotnet/communitytoolkit/maui/views/mediaelement#clean-up-mediaelement-resources

@marco-skizza
Copy link
Contributor Author

Thanks for your feedback @brminnick

I'm already disposing the handler.
I'm just currently using OnHandlerChanging instead of Unloaded on iOS.
This is because since .NET 8 the approach with Unloaded doesn't work for me anymore on iOS.
See also: #1663

But even though I'm disconnecting the handler, the MediaElement never gets Garbage Collected...

@pictos
Copy link
Member

pictos commented Feb 20, 2024

This is because since .NET 8 the approach with Unloaded doesn't work for me anymore on iOS.

@marco-skizza what's happening?

@marco-skizza
Copy link
Contributor Author

@pictos I try to explain...

I just updated my sample project.
Currently there are two points to hightlight:

    1. When navigating to the page with the MediaElement and back again multiple times, the MediaElement doesn't get Garbage Collected. Even when the handler was disposed.
    • iOS: Problem exists
    • WinUI: Don't know, because I don't know how to disconnect the handler on WinUI accordingly
    • Android: Couldn't test
    1. The Unloaded event can't be used anymore for disconnecting the handler...
    • a) The Unloaded events gets triggered when navigating to a third page, so if I disposed the handler, the MediaElement would already be disposed when navigating back.
      • iOS: Problem exists
      • WinUI: Problem exists
      • Android: Couldn't test
    • b) The Unloaded event gets also called on iOS when entering the fullscreen mode of the MediaElement, so if I disposed the handler, the MediaElement would already be disposed when leaving fullscreen mode.
      • iOS: Problem exists
      • WinUI: Don't know - didn't find a way to enter fullscreen mode on WinUI
      • Android: Couldn't test

I hope this makes any sense...

@vhugogarcia vhugogarcia added the 📽️ MediaElement Issue/PR that has to do with MediaElement label Feb 21, 2024
@TheCodeTraveler
Copy link
Collaborator

Hey @marco-skizza!

After some refactoring, I was able to trigger the finalizer in MyPage on both iOS + Android, despite it using the MediaElement. I've re-added the unverified for now.

I've opened a Pull Request to your sample reproduction: marco-skizza/CommunityMauiMediaElement#1

This fork contains the updated sample. To trigger the disposal of MyPage, continue to click the Button on each page to navigate back-and-forth between MyPage and MyThirdPage. With the debugger attached and a breakpoint in ~MyPage(), you will see the page does get disposed.

image

@marco-skizza
Copy link
Contributor Author

Notes from the Pull Request:

I played around with your code a bit and found out that it starts "leaking" again when you add the MyViewModel back to the MyPage as binding context. So I can imagine that there may be some "leaking" with the source binding Source="{Binding FilePath}" in MediaElement...

It may be a similar problem as: dotnet/maui#20710
It affects not every binding in .NET MAUI - but I found that on the CollectionView by investigating as well.
But we can also wait and see what this MAUI Issue brings to light...

@marco-skizza
Copy link
Contributor Author

marco-skizza commented Feb 22, 2024

Hi @brminnick

After playing around with your code some more I found out the following:

Your code seems to work, because you have a source binding Source="{Binding FilePath}" defined, but forgot to attach the ViewModel - which left the MediaElement uninitialized as I suppose. After setting the source to a valid path, even without a binding like Source="embed://Sister.m4a", it started leaking again. So I strongly believe that there is a leak in the MediaElement itself.

I hope you can look at this when time allows. Many Thanks!

@marco-skizza
Copy link
Contributor Author

Hi @brminnick

It seems - at least on iOS - that besides disconnecting the handler, one has to dispose the MediaElement itself as well:

        MyMediaElement.Handler?.DisconnectHandler();
        MyMediaElement.Dispose();

This fixed the Memory Leak for me - together with the fix above.

I'll open a PR when I tested this some more...

TheCodeTraveler pushed a commit that referenced this issue Mar 7, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working 📽️ MediaElement Issue/PR that has to do with MediaElement unverified
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants