Update VideoViewRenderer.cs #242
Conversation
As requested, I also made the changes here. https://code.videolan.org/videolan/LibVLCSharp/-/merge_requests/20
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| base.Dispose(disposing); | ||
|
|
||
| Control!.MediaPlayer = null; | ||
| Control!.Dispose(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The VideoView gets disposed properly already, from my own testing. I don't think this is necessary.
There was a problem hiding this comment.
Ok, then it should be fine to just avoid updating the MediaPlayer element if the renderer is disposing? (Set the flag _isDisposing = true)?
There was a problem hiding this comment.
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?
| if (_isDisposing) | ||
| { | ||
| return; | ||
| } | ||
| Control.MediaPlayer = e.NewMediaPlayer; |
There was a problem hiding this comment.
The exception, from what I posted the Stacktrace, gets thrown here (if the Control is already disposed)
|
Still reviewing this, please bear with me. Could not always repro with the simulator but on device the error is consistent. The reason this fails is because > 0x8 in LibVLCSharp.Forms.Platforms.Android.VideoViewRenderer.OnMediaPlayerChanging at C:\LibVLCSharp\src\LibVLCSharp.Forms\Platforms\Android\VideoViewRenderer.cs:68,13 C#
0x31 in LibVLCSharp.Forms.Shared.VideoView.OnMediaPlayerChanging at C:\LibVLCSharp\src\LibVLCSharp.Forms\Shared\VideoView.cs:46,13 C#
[External Code]
0x8 in LibVLCSharp.Forms.Shared.VideoView.set_MediaPlayer at C:\LibVLCSharp\src\LibVLCSharp.Forms\Shared\VideoView.cs:39,19 C#
0x13 in LibVLCSharp.Forms.Shared.MediaPlayerElement.OnMediaPlayerChanged at C:\LibVLCSharp\src\LibVLCSharp.Forms\Shared\MediaPlayerElement.xaml.cs:117,17 C#
0xD in LibVLCSharp.Forms.Shared.MediaPlayerElement.MediaPlayerPropertyChanged at C:\LibVLCSharp\src\LibVLCSharp.Forms\Shared\MediaPlayerElement.xaml.cs:158,13 C#
0x1A in LibVlcTestShell.ViewModels.BaseViewModel.OnPropertyChanged at C:\LibVLCSharp\samples\LibVlcTestShell\ViewModels\BaseViewModel.cs:50,13 C#
0x31 in LibVlcTestShell.ViewModels.BaseViewModel.SetProperty<LibVLCSharp.Shared.MediaPlayer> at C:\LibVLCSharp\samples\LibVlcTestShell\ViewModels\BaseViewModel.cs:38,13 C#
0xE in LibVlcTestShell.ViewModels.AboutViewModel.set_MediaPlayer at C:\LibVLCSharp\samples\LibVlcTestShell\ViewModels\AboutViewModel.cs:38,28 C#
0x147 in LibVlcTestShell.ViewModels.AboutViewModel.RefreshWebCamViewAction at C:\LibVLCSharp\samples\LibVlcTestShell\ViewModels\AboutViewModel.cs:63,13 C#I'm not entirely satisfied with your approach to fix this, which feels like a quick workaround for a target we don't support (Shell), and I would like to explore other alternatives. Is it possible for you to modify your app so that |
|
Sure, take the time you need. I provided a very simple test repo. I don't call the Attach() from there, so I guess it's how Xamarin.Form.Shell handles the page recreation and calls the Attach() method twice. |
|
So I made some further test with my repo. I also added a "Restart" button to trigger the reloading of the
private MediaPlayer _mediaPlayer;
/// <summary>
/// Gets the <see cref="LibVLCSharp.Shared.MediaPlayer"/> instance.
/// </summary>
public MediaPlayer MediaPlayer
{
get => _mediaPlayer;
private set => SetProperty(ref _mediaPlayer, value, nameof(MediaPlayer));
}protected bool SetProperty<T>(ref T backingStore, T value,
[CallerMemberName] string propertyName = "",
Action onChanged = null)
{
if (EqualityComparer<T>.Default.Equals(backingStore, value))
return false;
backingStore = value;
onChanged?.Invoke();
OnPropertyChanged(propertyName);
return true;
}
#region INotifyPropertyChanged
public event PropertyChangedEventHandler PropertyChanged;
protected void OnPropertyChanged([CallerMemberName] string propertyName = "")
{
var changed = PropertyChanged;
if (changed == null)
return;
changed.Invoke(this, new PropertyChangedEventArgs(propertyName)); // <=== HERE
}
#endregionThen, the Output: It seems not to be a general problem of the public class VideoViewRenderer : ViewRenderer<LibVLCSharp.Forms.Shared.VideoView, LibVLCSharp.Platforms.Android.VideoView>
{
private bool _isDisposing = false;
/// <summary>
/// Main constructor (empty)
/// </summary>
/// <param name="context">Android context</param>
public VideoViewRenderer(Context context) : base(context)
{
}
/// <summary>
///
/// </summary>
protected override void OnAttachedToWindow()
{
Console.WriteLine($"Attached VideoViewRenderer to window");
base.OnAttachedToWindow();
}
/// <summary>
/// Gets triggered when the object gets disposed
/// </summary>
/// <param name="disposing"></param>
protected override void Dispose(bool disposing)
{
_isDisposing = true;
Console.WriteLine($"Disposing renderer {nameof(VideoViewRenderer)}");
base.Dispose(disposing);
//Control!.MediaPlayer = null;
//Control!.Dispose();
}
/// <summary>
/// Native control management during lifecycle events
/// </summary>
/// <param name="e">lifecycle event</param>
protected override void OnElementChanged(ElementChangedEventArgs<VideoView> e)
{
base.OnElementChanged(e);
if (e.NewElement != null)
{
if (Control == null)
{
SetNativeControl(new LibVLCSharp.Platforms.Android.VideoView(Context!));
e.NewElement.MediaPlayerChanging += OnMediaPlayerChanging;
if (Control!.MediaPlayer != e.NewElement.MediaPlayer)
{
OnMediaPlayerChanging(this, new MediaPlayerChangingEventArgs(Control!.MediaPlayer, e.NewElement.MediaPlayer));
}
}
}
if (e.OldElement != null)
{
e.OldElement.MediaPlayerChanging -= OnMediaPlayerChanging;
}
}
private void OnMediaPlayerChanging(object sender, MediaPlayerChangingEventArgs e)
{
// Avoid updating the MediaPlayer if the Context from the Control is already disposed
if (_isDisposing)
{
Console.WriteLine($"Accessing renderer while disposing");
return;
}
Control.MediaPlayer = e.NewMediaPlayer;
Control.TriggerLayoutChangeListener();
}
}Test repo: https://github.com/AndreasReitberger/LibVlcViewIsNullTest |
|
What about calling |
|
As long as the exception disappears, I'm fine with it. Have you tested it this way? |
|
No I didn't that's a suggestion for you to try if you want to unlock the PR. I don't personnaly have the issue, but agree with mfkl that the solution you provide seems a bit too specific. Could you try to see if it fixes the issue? @mfkl would you be ok to merge such fix? |
|
@jeremyVignelles
|
|
The Xamarin team seems to have acknowledged the bug, so I'll close this for now. Thanks. |
As requested, I also made the changes here.
https://code.videolan.org/videolan/LibVLCSharp/-/merge_requests/20
Description of Change
Added an override for the "Dispose" method and set a private boolean to true if the object is disposing.
Then, if it's disposing, no upadate of the MediaPlayer element is happening for the disposing object.
This avoids the "View is null" exception on Android.
Issues Resolved
API Changes
None
Platforms Affected
Behavioral/Visual Changes
None
Before/After Screenshots
Not applicable
Testing Procedure
PR Checklist