unosquare / ffmediaelement

FFME: The Advanced WPF MediaElement (based on FFmpeg)
https://unosquare.github.io/ffmediaelement/
Other
1.17k stars 238 forks source link

UI deadlock due to async methods #588

Open markoweb2 opened 2 years ago

markoweb2 commented 2 years ago

UI deadlock due to async methods

I think that all the async methods in the class MediaElement are incorrectly using ConfigureAwait(true), because I encountered an UI deadlock (details in steps to reproduce section).

Take for example in the file MediaElement.cs:

public ConfiguredTaskAwaitable<bool> Open(Uri uri) => Task.Run(async () =>
{
        try { return await MediaCore.Open(uri).ConfigureAwait(false); }
        catch (Exception ex) { PostMediaFailedEvent(ex); }

        return false;
}).ConfigureAwait(true);

I believe the culprit do be the final outer ConfigureAwait(true). All references to async deadlocks that I found when googling, refered that all libraries should use ConfigureAwait(false) wherever possible instead. For example: https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

I will admit, I'm not yet super familiar with all the nuances of async and await, but I do believe that the code used below is perfectly reasonable and should work without having to go through extra hoops.

Issue Categories

Version Information

Steps to Reproduce

  1. Create a new WPF project
  2. Add FFME control (name = media)
  3. wire up Window_Loaded event to do media.Open(...) and media.Play()
  4. wire up the Media_Ended event handler for the FFME control, and use the following code:
    
    private void media_MediaEnded(object sender, EventArgs e)
    {
    this.CloseVideo();
    this.OpenVieo();
    }

private async void CloseVideo() { await this.media.Close(); }

private async void OpenVieo() { await this.media.Open(new Uri(@"C:\Data\VIDEO0081.mp4")); await this.media.Play(); }

 5. run the application, after the first video has finished playing, a UI deadlock occurs

## Expected Results

 - the UI not to deadlock, upon issuing simple Close(), Open(), Play() commands.

## Sample Code

### XAML
```xml
<Window x:Class="WpfAppNET5.Window2"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        xmlns:ffme="clr-namespace:Unosquare.FFME;assembly=ffme.win"
        Title="Window2" Height="450" Width="800" Loaded="Window_Loaded">
    <Grid>
        <ffme:MediaElement Name="media" LoadedBehavior="Manual" MediaEnded="media_MediaEnded" />
    </Grid>
</Window>

C

private async void media_MediaEnded(object sender, EventArgs e)
{
    // strangely, doing it like this will not cause a deadlock
    // but I need to seperate Close() and Open() due to app logic
    await this.media.Close();
    await this.media.Open(new Uri(@"C:\Data\VIDEO0081.mp4"));
    await this.media.Play();
}

// this code also works (my current workaround)
private void media_MediaEnded(object sender, EventArgs e)
{
    this.CloseVideo();
    this.OpenVideo();
}

private void CloseVideo()
{
    this.media.Close().GetAwaiter().GetResult();
}

private void OpenVideo()
{
    this.media.Open(new Uri(@"C:\Data\VIDEO0081.mp4")).GetAwaiter().GetResult();
    this.media.Play().GetAwaiter().GetResult();
}
markoweb2 commented 2 years ago

My bad. .GetAwaiter().GetResult(); still resulted in deadlocks in certain scenarios.

The correct code to use is instead:

private async void media_MediaEnded(object sender, EventArgs e)
{
    await this.CloseVideo();
    await this.OpenVideo();
}

private async Task CloseVideo()
{
    await this.media.Close();
}

private async Task OpenVideo()
{
    await this.media.Open(new Uri(@"C:\Data\VIDEO0081.mp4"));
    await this.media.Play();
}
markoweb2 commented 2 years ago

Hmh, I'm going to reopen the issue, because I'm still struggling with UI deadlocks in certain scenarios. And my gut feeling is telling me, it's all because the FFME library uses ConfigureAwait(true).

Also, looking at Microsoft's .net source code, they never use this type of method signature: public ConfiguredTaskAwaitable<bool> Play()

It's always like this public Task<bool> Play(). Because of this ,I'm thinking that FFME should change it's public methods to this style:

public async Task<bool> Play()
{
    try { return await MediaCore.Play().ConfigureAwait(false); }
    catch (Exception ex) { PostMediaFailedEvent(ex); }
    return false;
}

And the answers in this link seem to agree with my thinking - https://stackoverflow.com/questions/32551534/is-it-advantageous-to-use-configureawaitfalse-in-a-library-that-directly-retur

I'm also going to ask for a feature request as it is all very related to this issue. Please provide synchronous methods for people who don't want to deal with all these async problems. I for one don't want to waste any more time fighting these UI deadlocks. My videos are played in a separate window that runs in a separate thread with a separate dispatcher, I gain nothing with all these async methods, only headaches.

Also, doing something as simple as media.Close() inside a Window/Form Closing event handler is impossible with async methods. The only option is to use a hacky workaround of initially canceling the close, with e.cancel = true; Then fire off a this.Dispatcher.BeginInvoke(async () => { await media.Close(); this.Close(); }); You also need to wire up some logic, that the initial close would be cancelled, but the second time you close, you would actually allow the close...

One option to not cause a breaking change is to create new OpenSync(), CloseSync(), etc methods. But if breaking change is to be caused by fixing the method signatures by using the style public async Task..., then it's better to use the standard pattern of OpenAsync(), CloseAsync(), etc for async methods and Open(), Close(), etc for synchronous methods.

a44281071 commented 2 years ago

Also lock In view model deactive: Maybe lock UI after window closed.

await MediaElement.Close();

I think it is not suitable to use UI thread to await a Play() or Stop() async result. Maybe we can create a new thread to get Player Control state and use TaskCompletionSource return a waiting Task(new thread).

benwilkinson commented 1 year ago

I'm running up to 8 Media simultaneously each in their own window. Play/Pause commands come from a controller as separate tasks for each Media and am experiencing deadlocks more often the more Media elements there are. A play/pause/seek command will take ~15-75ms for each usually. Occasionally (the more Medias the more likely) it will take 1-3 second to complete a Pause. Usually when this happens at least a couple of the Media have paused within the regular time, and all the rest will have had to wait for one of the other commands to complete.

I'm running with: Unosquare.FFME.Library.EnableWpfMultiThreadedVideo = true;