836927 - Get Windows Media Foundation backend to pass Mochitests on TBPL/Try

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

The reason we haven't been able to enable the WMF backend for H.264/AAC/MP3 playback on Windows yet is because I've had random failures on TryServer.

The failures typically manifest om mtest-1 win7 opt runs as:

TEST-UNEXPECTED-FAIL | Shutdown | application timed out after 330 seconds with no output 
PROCESS-CRASH | Shutdown | application crashed [@ CrashingThread(void *)] 

The test run log shows that the state machine thread is blocked waiting on the decode thread to shutdown, and the decode thread is stuck, thus causing a shutdown hang and an injected crash at the end of the mtest-1 run.

The decode thread stack is typically:

8  mfreadwrite.dll + 0xa65e
eip = 0x6ccea65f   esp = 0x32b2f78c   ebp = 0x32b2f7dc
Found by: previous frame's frame pointer
9  xul.dll!mozilla::WMFReader::DecodeAudioData() [WMFReader.cpp:4a8695a18c49 : 487 + 0x2b]
eip = 0x6bed4ed4   esp = 0x32b2f7e4   ebp = 0x32b2f84c
Found by: previous frame's frame pointer
10  xul.dll!mozilla::MediaDecoderReader::DecodeToTarget(__int64) [MediaDecoderReader.cpp:4a8695a18c49 : 484 + 0x6]
eip = 0x6bec1429   esp = 0x32b2f854   ebp = 0x32b2f8a8
Found by: call frame info
11  xul.dll!mozilla::WMFReader::Seek(__int64,__int64,__int64,__int64) [WMFReader.cpp:4a8695a18c49 : 724 + 0xc]
eip = 0x6bed4830   esp = 0x32b2f8b0   ebp = 0x32b2f8cc
Found by: call frame info
12  xul.dll!mozilla::MediaDecoderStateMachine::DecodeSeek() [MediaDecoderStateMachine.cpp:4a8695a18c49 : 1939 + 0x1f]
eip = 0x6bebfe00   esp = 0x32b2f8d4   ebp = 0x32b2f91c
Found by: call frame info
13  xul.dll!mozilla::MediaDecoderStateMachine::DecodeThreadRun() [MediaDecoderStateMachine.cpp:4a8695a18c49 : 494 + 0x6]
eip = 0x6bec0077   esp = 0x32b2f924   ebp = 0x32b2f930
Found by: call frame info

I have also seen this stuck in DecodeVideoData() as well.

I have been able to reproduce this locally on a Win7 (not Win8) when shutting down after looping on test_seek.html for a few minutes.

The problem I believe is that our WMFByteStream's behaviour doesn't match WMF's IMFByteStream's implementation's behaviour, and one of WMF's decoder's logic is tripping up due to this.

So WMFByteStream needs to be made bug-compatible with WMF's IMFByteStream implementation.

Fortunately I've (finally!) discovered how to instantiate WMF's IMFByteStream implementation, and have reverse engineered its behaviour. Patch forthcoming...

Comment 1

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

My Visual Studio/C++ project to reverse engineer WMF's IMFByteStream's behaviour has been commited to the ether for posterity here:

https://github.com/cpearce/IMFByteStreamBehaviour

Comment 2

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

Attachment: Patch v1

* Add MediaResource::IsTransportSeekable() to report whether the stream supports Byte Range requests. We can't rely on MediaDecoder::IsTransportSeekable() inside WMFByteStream because then we'd have to expose MediaDecoder there, and we can't pass the value in because it might not be initialized at the time that the WMFByteStream is created.
* Make MediaResource ref counted. WMFByteStream needs to hold a strong reference to it, so that it doesn't crash if it tries to use the stream after the media element has been destroyed and shutdown the stream.
* Report MFBYTESTREAM_HAS_SLOW_SEEK capability if the transport isn't seekable. This causes WMF to assume it has to download the resource sequentially in the live stream case, and fixes bug 824877.
* Rename a few things to make their purpose clearer.
* Change some error codes and some behaviour of edge cases. This should make our implementation match WMF's implementation.
* Green On Try! (un-cleaned up patch): https://tbpl.mozilla.org/?tree=Try&rev=11f655afd514 I think the orange there is bug 603147.

Comment 3

[Paul Adenot (:padenot)]

Comment on attachment 708869 [details] [diff] [review]
Patch v1

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

Maybe you could stick your findings [1] at the top of the header, or reference the github repo, or comment inline where those particular behaviour are implemented.

[1]: https://github.com/cpearce/IMFByteStreamBehaviour/blob/master/ByteStreamBehaviour/ByteStreamBehaviour.cpp#L505

::: content/media/wmf/WMFByteStream.cpp
@@ +82,5 @@
> +    // Explicitly release the MediaResource reference. We *must* do this on
> +    // the main thread, so we must explicitly release it here, we can't rely
> +    // on the destructor to release it, since if this event runs before its
> +    // dispatch call returns the destructor may run on the non-main thread.
> +    mResource = nullptr;    

nit: trailing spaces.

@@ +425,3 @@
>    *aCapabilities = MFBYTESTREAM_IS_READABLE |
> +                   MFBYTESTREAM_IS_SEEKABLE |
> +                   MFBYTESTREAM_IS_PARTIALLY_DOWNLOADED |

Maybe you should use MediaResource::IsDataCachedToEndOfResource(0) or something. Not sure if it matters, though, but according to [1], "This flag is cleared after all of the data has been downloaded".

[1]: http://msdn.microsoft.com/en-us/library/windows/desktop/ms698962%28v=vs.85%29.aspx

Comment 4

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

(In reply to Paul Adenot (:padenot) from comment #3)
> Comment on attachment 708869 [details] [diff] [review]
> @@ +425,3 @@
> >    *aCapabilities = MFBYTESTREAM_IS_READABLE |
> > +                   MFBYTESTREAM_IS_SEEKABLE |
> > +                   MFBYTESTREAM_IS_PARTIALLY_DOWNLOADED |
> 
> Maybe you should use MediaResource::IsDataCachedToEndOfResource(0) or
> something. Not sure if it matters, though, but according to [1], "This flag
> is cleared after all of the data has been downloaded".
> 
> [1]:
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms698962%28v=vs.
> 85%29.aspx

Thanks Paul, that sounds like a thoroughly reasonable change to make.

But given how long it took me to get this green, I want to test that thoroughly before I make that change. So in interests of getting WMF enabled faster, I'll go ahead and land the patch, and test that change and land it in a separate bug if it's safe.

Comment 5

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

(In reply to Chris Pearce (:cpearce) from comment #2)
> Created attachment 708869 [details] [diff] [review]
> Patch v1
> 
> * Add MediaResource::IsTransportSeekable() to report whether the stream
> supports Byte Range requests. We can't rely on
> MediaDecoder::IsTransportSeekable() inside WMFByteStream because then we'd
> have to expose MediaDecoder there, and we can't pass the value in because it
> might not be initialized at the time that the WMFByteStream is created.

Oops, I forgot to implement IsTransportSeekable() on BufferedMediaResource, I'll add that and have it return true before landing.

Comment 6

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

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

Comment 7

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

Caught by Warnings as errors. Bustage fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aad0225ff8f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1f54c7df317
https://hg.mozilla.org/mozilla-central/rev/3aad0225ff8f