830171 - Change WMFReader to use IMFSourceReader in async mode

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We should experiment with changing the WMFReader to using the IMFSourceReader in async mode. We currently use the SourceReader in sync mode, and if the MediaResource is shut down before the sync call to create the source reader completes the call may never complete on Win7 and we get shutdown hangs. In particular this failure mode is hit on TryServer mochitests, and is the reason why WMF isn't enabled by default. Hopefully if the MediaResoure is shut down before the async call to create the source reader is completed, we can just tell the source reader to shutdown, and it will....

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Argh. This approach doesn't work. When we run the IMFSourceReader in async mode, the only function that actually runs asynchronously is  IMFSourceReader::ReadSample, all other functions are still synchronous, including MFCreateSourceReaderFromByteStream, which is where the hang is.

I'll have to think of something else...

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Argh, actually this approach does work. We can get stuck inside IMFSourceReader::ReadSample() when running in synchronous mode if the MediaResource is closed while we're reading. This is manifesting as shutdown hangs in debug builds locally and on Try when I've got the patches in bug 839031 included.

When I change to use the IMFSourceReader in async mode we can shutdown the decode thread properly when the MediaResource is closed, and the shutdown hangs are fixed.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1: Use IMFSourceReader in async mode

* Create a new class that implement IMFSourceReaderCallback; WMFSourceReaderCallback.
* WMFReader creates a WMFSourceReaderCallback and passes it into IMFSourceReader when it's being created. The WMFSourceReaderCallback is also passed to the WMFByteStream.
* To read a sample the WMFReader calls IMFSourceReader::ReadSample() and then calls WMFSourceReaderCallback::Wait() to await the result.
* When WMF has read a sample it calls WMFSourceReaderCallback::OnReadSample(), and the WMFSourceReaderCallback then unblocks the WMFReader's thread which is calling Wait().
* When the WMFByteStream detects that the underlying MediaResource has closed (when a read or seek operation fails) it notifies the WMFSourceReaderCallback, which also unblocks any WMFReader thread blocks in Wait(). This happens when the media element is being shutdown, and prevents our shutdown hang.

This patch is based on top of the patches for bug 839055 and bug 839031 respectively. We need this patch for those patches to land, otherwise we get test failures on tbpl, and possibly shutdown hangs in the wild (I've only seen shutdown hangs in debug builds, but there's no reason why they couldn't occur in opt/pgo builds in the wild too).

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 714977 [details] [diff] [review]
Patch v1: Use IMFSourceReader in async mode

Review of attachment 714977 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/wmf/WMF.h
@@ +27,5 @@
>  #include <stdio.h>
>  #include <mferror.h>
>  #include <propvarutil.h>
>  #include <wmcodecdsp.h>
> +#include <shlwapi.h>

This does not seem to be used. It compiles fine without this line.

::: content/media/wmf/WMFSourceReaderCallback.cpp
@@ +35,5 @@
> +
> +NS_IMPL_THREADSAFE_ADDREF(WMFSourceReaderCallback)
> +NS_IMPL_THREADSAFE_RELEASE(WMFSourceReaderCallback)
> +
> +WMFSourceReaderCallback::WMFSourceReaderCallback() 

nit: trailing space.

@@ +75,5 @@
> +
> +  mResultStatus = aReadStatus;
> +  mStreamFlags = aStreamFlags;
> +
> +  // Set the sentinal value and notify the monitor, so that threads waiting

s/sentinel/sentinal/ ? But it's more likely to be me not knowing that word, since you use the term twice.

@@ +139,5 @@
> +  *aStreamFlags = mStreamFlags;
> +  *aTimeStamp = mTimestamp;
> +  *aSample = mSample;
> +  HRESULT hr = mResultStatus;
> +  

nit: trailing spaces.

::: content/media/wmf/WMFSourceReaderCallback.h
@@ +8,5 @@
> +
> +#include "WMF.h"
> +#include "mozilla/ReentrantMonitor.h"
> +#include "mozilla/RefPtr.h"
> +#include "nsISupportsImpl.h "

nit: space before closing "

@@ +39,5 @@
> +  STDMETHODIMP OnFlush(DWORD);
> +
> +  // Causes the calling thread to block waiting for the
> +  // IMFSourceReader::ReadSample() result callback to occur, or for the
> +  // WMFByteStream to be closed. 

nit: trailing space.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks Paul!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e103247c9f

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/b5e103247c9f