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.

Sign up for GitHub

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

Update VideoViewRenderer.cs #242

Closed
wants to merge 1 commit into from
Closed

Update VideoViewRenderer.cs #242

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/LibVLCSharp.Forms/Platforms/Android/VideoViewRenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ namespace LibVLCSharp.Forms.Platforms.Android
/// Xamarin.Forms custom renderer for the Android VideoView
/// </summary>
public class VideoViewRenderer : ViewRenderer<LibVLCSharp.Forms.Shared.VideoView, LibVLCSharp.Platforms.Android.VideoView>
{
{
private bool _isDisposing = false;

/// <summary>
/// Main constructor (empty)
/// </summary>
Expand All @@ -23,6 +25,19 @@ public VideoViewRenderer(Context context) : base(context)
{
}

/// <summary>
/// Gets triggered when the object gets disposed
/// </summary>
/// <param name="disposing"></param>
protected override void Dispose(bool disposing)
{
_isDisposing = true;
base.Dispose(disposing);

Control!.MediaPlayer = null;
Control!.Dispose();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very rare for ViewRenderers to dispose of the Control. Not sure if this is a common leak or if it should not be done.

Copy link
Author

@AndreasReitberger AndreasReitberger Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically it's enough to set the "_isDisposing = true" here. Just avoid to update the MediaPlayer in the OnMediaPlayerChanging event. I just thought to do also the cleanup here for the Control.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check whether Xamarin.Forms performs the Dispose internally on the native Control? It's not obvious from some glances, but I think it might not. In which case we have a leak, including in the Apple renderer. If so, feel free to update it as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I try to figure out where the Control gets disposed and report back. I never had such issues on iOS, only Android.
Also, all is working fine if you do not use Shell. So I guess it's the way how Xamarin.Forms handles the page re-creation if you are using Shell.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this answers the disposing behaviour of Shell. @PureWeen wrote this on a Xamarin.Forms bug.

It should be a lot better right now in MAUI. One of the issues here is that on XF shell aggressively disposes and recreates renderers every time you move between tabs/flyout items. With MAUI we aren't doing that and as you move around the native stacks are all maintained.

If anyone is up for testing this out a bit on MAUI let me know your results.

xamarin/Xamarin.Forms#7521 (comment)

So I guess, when the Page is disposing, the MediaPlayerElement on it gets disposed as well (so the native control).
Then, Shell recreates the page.

Copy link
Member

@mfkl mfkl Sep 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The VideoView gets disposed properly already, from my own testing. I don't think this is necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then it should be fine to just avoid updating the MediaPlayer element if the renderer is disposing? (Set the flag _isDisposing = true)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to avoid something if it does not happen in the first place. This really shouldn't happen in the first place, but it's too easy to place the blame on Xamarin Shell only.

Do you think it could be possible to reproduce without Shell?

}

/// <summary>
/// Native control management during lifecycle events
/// </summary>
Expand Down Expand Up @@ -53,6 +68,11 @@ protected override void OnElementChanged(ElementChangedEventArgs<VideoView> e)

private void OnMediaPlayerChanging(object sender, MediaPlayerChangingEventArgs e)
{
// Avoid updating the MediaPlayer if the Context from the Control is already disposed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you actually encounter this case? The repro code and stacktrace you shared does not include these lines, so I wonder why the defensive check here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this happens each time, if you navigate using Shell. If you re-visit a page, the old render gets disposed and a new one is created.
You can check it here:
https://github.com/AndreasReitberger/LibVlcViewIsNullTest

Just run the test repo and switch from "About" to "About2" and back to "About". Then you see the exception. If you set a breakpoint on "OnMediaPlayerChanging" and "Dispose", you'll see when it's called.

if (_isDisposing)
{
return;
}
Control.MediaPlayer = e.NewMediaPlayer;
Copy link
Author

@AndreasReitberger AndreasReitberger Sep 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception, from what I posted the Stacktrace, gets thrown here (if the Control is already disposed)

Control.TriggerLayoutChangeListener();
}
Expand Down