Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

Implemented MediaElement ObjectDisposedException fix #1870

Merged
merged 1 commit into from Jun 29, 2022
Merged

Implemented MediaElement ObjectDisposedException fix #1870

Show file tree
Hide file tree
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
8 changes: 6 additions & 2 deletions ...nityToolkit/Xamarin.CommunityToolkit/Views/MediaElement/Android/FormsVideoView.android.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,14 @@ protected void ExtractMetadata(MediaMetadataRetriever retriever)

public override async void SetVideoURI(global::Android.Net.Uri? uri, IDictionary<string, string>? headers)
{
base.SetVideoURI(uri, headers);
Copy link
Contributor

Choose a reason for hiding this comment

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

if we cache the url in a local variable, does it work?

var localUri = uri;
// rest of the code use `localUri`

Copy link
Author

Choose a reason for hiding this comment

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

It should work without any issues, but can you elaborate why we should do that? The call to the base method throws an exception (if the object is disposed), so it wouldn't matter if we cache the argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

That will avoid the usage of a disposed member. I looked into the SetMetadata method and can't see any dispose logical there... So it's something else that's disposing the URL, probably because we're using an async void and it's not waited.

Copy link
Author

Choose a reason for hiding this comment

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

Take a look here. The uri that is passed down to SetVideoURI() is never disposed. To elaborate, MediaElement is working correctly unless you close the parent page while it's loading metadata, which will Dispose() the renderer and in turn dispose FormsVideoView. In the old implementation, metadata awaiting was before the base method call. If the user closes the page while we are awaiting metadata updates, then the next call to base.SetVideoURI will throw and since we are in a async void method we can't catch any exceptions outside. Of course, we can do this after awaiting:

Suggested change
base.SetVideoURI(uri, headers);
try
{
base.SetVideoURI(uri, headers);
}
catch(ObjectDisposedException) { }

but that is bad code in my opinion.

I caught this bug by accident, by opening and closing the page really fast. After rearranging them, there were no more exceptions. The other bug (not set to null after disposing) was more frequent since the MetadataRetrieved event was raised before the base call and the renderer tried to access the disposed instance first.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anvuks I believe that your PR fixes the issue. I just want to make sure that your PR doesn't cause any side effects. I'll test it tomorrow and merge it if everything works during my test. Also, I invite @peterfoot to take a look on this if he can

Copy link

Choose a reason for hiding this comment

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

@pictos @anvuks can you guys tell me where can i find the file that need to be edited by adding try and catch ???


// this instance could get disposed during awaiting, so a call to the base method (AFTER awaiting)
// would throw ObjectDisposedException and be impossible to catch due to async void
if (uri != null)
{
await SetMetadata(uri, headers);

base.SetVideoURI(uri, headers);
}
}

protected async Task SetMetadata(global::Android.Net.Uri uri, IDictionary<string, string>? headers)
Expand Down
10 changes: 8 additions & 2 deletions ...olkit/Xamarin.CommunityToolkit/Views/MediaElement/Android/MediaElementRenderer.android.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public class MediaElementRenderer : FrameLayout, IVisualElementRenderer, IViewRe
VisualElementTracker? tracker;
protected MediaController? controller;
protected MediaPlayer? mediaPlayer;
protected FormsVideoView view;
protected FormsVideoView? view;
bool isDisposed;
int? defaultLabelFor;

Expand All @@ -47,9 +47,14 @@ public MediaElementRenderer(Context context)

public override float Alpha
{
get => view.Alpha;
get => view?.Alpha ?? throw new ObjectDisposedException(typeof(FormsVideoView).FullName);
set
{
if (view == null)
{
throw new ObjectDisposedException(typeof(FormsVideoView).FullName);
}

// VideoView opens a separate Window above the current one.
// This is because it is based on the SurfaceView.
// And we cannot set alpha or perform animations with it because it is not synchronized with your other UI elements.
Expand Down Expand Up @@ -509,6 +514,7 @@ protected virtual void ReleaseControl()
view.SetOnPreparedListener(null);
view.SetOnCompletionListener(null);
view.Dispose();
view = null;
}

if (controller != null)
Expand Down