Closed
Bug 836927
Opened 8 years ago
Closed 8 years ago
Get Windows Media Foundation backend to pass Mochitests on TBPL/Try
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: cpearce, Assigned: cpearce)
References
Details
Attachments
(1 file)
27.64 KB,
patch
|
padenot
:
review+
|
Details | Diff | Splinter Review |
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...
Assignee | ||
Comment 1•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 2•8 years ago
|
||
* 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•8 years ago
|
||
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
Attachment #708869 -
Flags: review?(paul) → review+
Assignee | ||
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
(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.
Assignee | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f54c7df317
Assignee | ||
Comment 7•8 years ago
|
||
Caught by Warnings as errors. Bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/3aad0225ff8f
Comment 8•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b1f54c7df317 https://hg.mozilla.org/mozilla-central/rev/3aad0225ff8f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•