Crash in mozilla::WMFAudioMFTManager::Output(__int64, nsAutoPtr<mozilla::MediaData>&) when trying to play a mp4 file

VERIFIED FIXED in Firefox 36

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: benjamin.baudel, Assigned: jya)

Tracking

(4 keywords)

35 Branch
mozilla38
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox35 wontfix, firefox36+ verified, firefox37 verified, firefox38 verified)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150108202552

Steps to reproduce:

Simply drag and drop, or use into a video html5 tag, some mp4 files on the browser provide a crash.
On Windows 7 + Firefox final release 35.0

Some mp4 files with issue can be found here : 
http://www.kolor.com/bugreports/firefox35_mp4/

Not really tested yet but seems to have issue (not playing) on MacOSX too.


Actual results:

Browser crashes (with ability to send a bug report)

First part of the crash report :
{ "crash_info": { "address": "0x100a97b9", "crashing_thread": 75, "type": "EXCEPTION_INT_DIVIDE_BY_ZERO" }, "crashing_thread": { "frames": [ { "file": "hg:hg.mozilla.org/releases/mozilla-release:content/media/fmp4/wmf/WMFAudioMFTManager.cpp:32e36869f84a", "frame": 0, "function": "mozilla::WMFAudioMFTManager::Output(__int64,nsAutoPtr<mozilla::MediaData> &)", "function_offset": "0x183", "line": 267, "module": "xul.dll", "module_offset": "0x6b97b9", "offset": "0x100a97b9", "registers": { "eax": "0x00001000", "ebp": "0x2096f93c", "ebx": "0x00000000", "ecx": "0x00000000", "edi": "0x2ae6f268", "edx": "0x00000000", "efl": "0x00010246", "eip": "0x100a97b9", "esi": "0x2ae6f250", "esp": "0x2096f8f4" }, "trust": "context" }, { "file": "hg:hg.mozilla.org/releases/mozilla-release:content/media/fmp4/wmf/WMFMediaDataDecoder.cpp:32e36869f84a", "frame": 1, "function": "mozilla::WMFMediaDataDecoder::ProcessOutput()", "function_offset": "0x34", "line": 96, "module": "xul.dll", "module_offset": "0x6b9dd7", "offset": "0x100a9dd7", "trust": "cfi" }, { "file": "hg:hg.mozilla.org/releases/mozilla-release:content/media/fmp4/wmf/WMFMediaDataDecoder.cpp:32e36869f84a", "frame": 2, "function": "mozilla::WMFMediaDataDecoder::ProcessDecode(mp4_demuxer::MP4Sample *)", "function_offset": "0x31", "line": 87, "module": "xul.dll", "module_offset": "0x6b9e65", "offset": "0x100a9e65", "trust": "cfi" }, { "file": "hg:hg.mozilla.org/releases/mozilla-release:xpcom/glue/nsThreadUtils.h:32e36869f84a", "frame": 3, "function": "nsRunnableMethodImpl<void ( mozilla::CDMProxy::*)(unsigned int),unsigned int,1>::Run()", "function_offset": "0x10", "line": 363, "module": "xul.dll", "module_offset": "0x68bd62", "offset": "0x1007bd62", "trust": "cfi" }, { "file": "hg:hg.mozilla.org/releases/mozilla-release:content/media/MediaTaskQueue.cpp:32e36869f84a", "frame": 4, "function": "mozilla::MediaTaskQueue::Runner::Run()", "function_offset": "0x7c", "line": 194, "module": "xul.dll", "module_offset": "0xeab69b", "offset": "0x1089b69b", "trust": "cfi" }, { "file": "hg:hg.mozilla.org/releases/mozilla-release:xpcom/threads/nsThreadPool.cpp:32e36869f84a", "frame": 5, "function": "nsThreadPool::Run()", "function_offset": "0x1f6", "line": 220, "module": "xul.dll", "module_offset": "0x23d3b6", "offset": "0xfc2d3b6", "trust": "cfi" }, { "file": "hg:hg.mozilla.org/releases/mozilla-release:xpcom/threads/nsThread.cpp:32e36869f84a", "frame": 6, "function": "nsThread::ProcessNextEvent(bool,bool *)", "function_offset": "0x2f0", "line": 830, "module": "xul.dll", "module_offset": "0x1d2b70", "offset": "0xfbc2b70", "trust": "cfi_scan" }, { "file": "hg:hg.mozilla.org/releases/mozilla-release:ipc/chromium/src/base/message_loop.cc:32e36869f84a", "frame": 7, "function": "MessageLoop::RunHandler()", "function_offset": "0x4f", "line": 223, "module": "xul.dll", "module_offset": "0x39d1db", "offset": "0xfd8d1db", "trust": "cfi_scan" }, { "file": "hg:hg.mozilla.org/releases/mozilla-release:xpcom/threads/nsThread.cpp:32e36869f84a", "frame": 8, "function": "nsThread::ThreadFunc(void *)", "function_offset": "0x86", "line": 350, "module": "xul.dll", "module_offset": "0x90972", "offset": "0xfa80972", "trust": "cfi_scan" } ], "threads_index": 75, "total_frames": 9 }


Expected results:

Displaying the video (or not if 3K and over due to the Windows codecs usage).
Firefox final release 34.X was working in that way without crash.
Component: Untriaged → General
Keywords: crash
https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3AWMFAudioMFTManager%3A%3AOutput%28__int64%2C+nsAutoPtr%3Cmozilla%3A%3AMediaData%3E%26%29&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1

Seems to be mostly Windows 7, a few Windows 8 crashes as well.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozilla::WMFAudioMFTManager::Output(__int64, nsAutoPtr<mozilla::MediaData>&)]
Component: General → Video/Audio
Ever confirmed: true
Product: Firefox → Core
Summary: Crash when trying to play a mp4 file → Crash in mozilla::WMFAudioMFTManager::Output(__int64, nsAutoPtr<mozilla::MediaData>&) when trying to play a mp4 file
(In reply to benjamin from comment #0)
> Some mp4 files with issue can be found here : 
> http://www.kolor.com/bugreports/firefox35_mp4/

This is very helpful, thanks!

This crashes for me on both 35 and even on nightly:

https://crash-stats.mozilla.com/report/index/11531f59-6b54-4647-8085-ac28c2150115
Crash Signature: [@ mozilla::WMFAudioMFTManager::Output(__int64, nsAutoPtr<mozilla::MediaData>&)] → [@ mozilla::WMFAudioMFTManager::Output(__int64, nsAutoPtr<mozilla::MediaData>&)] [@ mozilla::WMFAudioMFTManager::Output(__int64, nsRefPtr<mozilla::MediaData>&)]
so mAudioBytesPerSample is 0, which is clearly wrong (in the sense that that causes this crash)...

The code is a bit weird, because a while lower down there is:

  // Just assume PCM output for now...
  MOZ_ASSERT(mAudioBytesPerSample == 2);

so it seems we really want this to be 2 anyway...

However, as it is, the value is generated from:

  , mAudioBytesPerSample(aConfig.bits_per_sample / 8)

http://mxr.mozilla.org/mozilla-central/source/dom/media/fmp4/wmf/WMFAudioMFTManager.cpp#73

which gets called from:

WMFDecoderModule::CreateAudioDecoder(const mp4_demuxer::AudioDecoderConfig& aConfig,

which is updated with metadata here:

http://mxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/mp4_demuxer.cpp#120

where the find on the metadata for the relevant data returns 0.

However, we do seem to figure out that it's AAC data and get a profile for it (whatever that means - I'm not at home in this code).

The android libstagefright bits do an assert for bits_per_sample being 16. Don't really know if that would help any here, but it seems like either we should not play audio, or we should assume 2-byte PCM and try to play anyway. Or something.

Perhaps:

http://mxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/DecoderData.cpp#176

just needs to also check that bits_per_sample > 0 ?

Matthew, seems these two divisions are yours... do you know what to do? :-)
Flags: needinfo?(kinetik)
Thanks for digging in to this!

(In reply to :Gijs Kruitbosch from comment #3)
> Matthew, seems these two divisions are yours... do you know what to do? :-)

I just moved them further down in the function in bug 1091704, I don't own them. ;-)

Looking at source.01-720p_SD.mp4, we read the 'mp4a' chunk at offset 9735 and get a valid looking num_channels and sample_rate, but zero for sample_size.  

I don't know if we can rely on these values since the definitive values are supposed to be derived from the audio codec metadata as I understand it, but AudioDecoderConfig::IsValid() makes it look like we want to treat these values as definitive (i.e. we are requiring them).  So it does seem like adding a check to IsValid() for sample_size is appropriate, but I'm not the resident expert in this area.
Flags: needinfo?(kinetik)
Adding others who know more in this area for opinions.
Flags: needinfo?(jyavenard)
Flags: needinfo?(cpearce)
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
Under some (unknown) circumstances, IMF returns a frame with a pts negative even though input pts were correct. So treats those as 0
Attachment #8551031 - Flags: review?(cpearce)
Do not use demuxer's encoded frame's bit depth for processing our output (they won't be the same). Also configure the audio decoding transform to output 16 bits PCM
Attachment #8551032 - Flags: review?(cpearce)
Comment on attachment 8551032 [details] [diff] [review]
imported patch _set_pcm_output_parameters.patch

noticed something not quite right while uploading
Attachment #8551032 - Attachment is obsolete: true
Attachment #8551032 - Flags: review?(cpearce)
Attachment #8551031 - Attachment is obsolete: true
Attachment #8551031 - Flags: review?(cpearce)
Do not use demuxer's encoded frame's bit depth for processing our output (they won't be the same). Also configure the audio decoding transform to output 16 bits PCM
Attachment #8551045 - Flags: review?(cpearce)
Under some (unknown) circumstances, IMF returns a frame with a pts negative even though input pts were correct. So treats those as 0
Attachment #8551046 - Flags: review?(cpearce)
Comment on attachment 8551046 [details] [diff] [review]
Treat negative WMF's output sample timestamp as zero

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

::: dom/media/fmp4/wmf/WMFVideoMFTManager.cpp
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include <algorithm>

It's important that the WMFVideoMFTManager.h include comes first, so that the header is compilable without other includes added. So move your #include <algorithm> down.
Attachment #8551046 - Flags: review?(cpearce) → review+
Comment on attachment 8551045 [details] [diff] [review]
Configure WMF decoder to output PCM 16

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

Solves the problem, but I'd like to avoid adding more coupling in our code where possible.

::: dom/media/fmp4/wmf/MFTDecoder.cpp
@@ +101,5 @@
>      if (FAILED(hr)) {
>        continue;
>      }
>      if (subtype == mOutputSubtype) {
> +      if (subtype == MFAudioFormat_PCM) {

This is bad because you're adding coupling between the MFTDecoder and the WMFAudioMFTManager; the assumption there based on logic here.

So please change this patch so that instead of passing a GUID to MFTDecoder::SetMediaTypes() for the output type, pass an instance of IMFMediaType, which we store in MFTDecoder::mOutputSubtype. Since mOutputSubtypeis no longer a subType, I think we should rename to mOutputType.

In WMFAudioMFTManager::Init(), create an IMFMediaType for the output, and set MF_MT_MAJOR_TYPE, MF_MT_SUBTYPE, MF_MT_SAMPLE_SIZE and MF_MT_AUDIO_BITS_PER_SAMPLE on it, and pass that to MFTDecoder::SetMediaTypes(), which needs to store it.

Then in MFTDecoder::SetOutputType in this loop you can call IMFAttributes::Compare():
http://msdn.microsoft.com/en-us/library/windows/desktop/bb970349%28v=vs.85%29.aspx
(IMFMediaType inherits from IMFAttributes).

Call Compare on mOutputType, passing in the available outputType.

You need to pass a MF_ATTRIBUTES_MATCH_OUR_ITEMS comparison type for the comparison to work. Then you will select an appropriate output type.

You also need to change the video path, but for that output type you should only need the major and sub type set.
Attachment #8551045 - Flags: review?(cpearce) → review-
Flags: needinfo?(cpearce)
Comment on attachment 8551045 [details] [diff] [review]
Configure WMF decoder to output PCM 16

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

We can take this on beta, but I'd like a cleaner fix on Nightly longer term...
Attachment #8551045 - Flags: review- → review+
Keywords: regression
Blocks: 1123269
https://hg.mozilla.org/mozilla-central/rev/1d32c46f2143
https://hg.mozilla.org/mozilla-central/rev/f16ef283fee5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
[Tracking Requested - why for this release]:
crashy regressions are sad regressions

Not sure it makes sense to track for 35; I guess that ship has sailed and I don't know if the patch would be safe enough to take in a point release, and/or if we are looking at that or not.
I personally think it is a significant regression. It would affect a lot of mp4 playback.

Bug 1123269 is better.

I can also create another patch that is safer in that it doesn't use any new WMF API, and simply remove the assert and where the division by 0 occurs.

Having said that the confidence is pretty high
Comment on attachment 8551046 [details] [diff] [review]
Treat negative WMF's output sample timestamp as zero

Approval Request Comment
[Feature/regressing bug #]: MSE/WMFDecoder mp4 playback.
[User impact if declined]: Crashes playing some video files.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Both changes are straightforward and low risk.
[String/UUID change made/needed]: None.

This request applies to both patches on this bug. This is a high-priority crash fix for MSE.
Attachment #8551046 - Flags: approval-mozilla-beta?
Attachment #8551046 - Flags: approval-mozilla-aurora?
Attachment #8551046 - Flags: approval-mozilla-beta?
Attachment #8551046 - Flags: approval-mozilla-beta+
Attachment #8551046 - Flags: approval-mozilla-aurora?
Attachment #8551046 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
Reproduced on Nightly 2015-01-12, Windows 7 x64, with str from comment 0: bp-a1a1a712-3f3b-4370-8542-337b82150126
Verified as fixed with 36.0 beta 3 (Build ID: 20150122214638), Aurora 37.0a2 (Build ID: 20150125004007) and Nightly 38.0a1 (Build ID: 20150125030202) on Windows 7 x64 and Mac OS X 10.9.5 - crash not reproducible.
In Socorro there are 0 reports for both signatures after this fix landed:
--- mozilla::WMFAudioMFTManager::Output(__int64, nsRefPtr<mozilla::MediaData>&) -> 0 crashes: https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3AWMFAudioMFTManager%3A%3AOutput(__int64%2C+nsRefPtr%3Cmozilla%3A%3AMediaData%3E%26)&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1
--- mozilla::WMFAudioMFTManager::Output(__int64, nsAutoPtr<mozilla::MediaData>&) -> 0 crashes: https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3AWMFAudioMFTManager%3A%3AOutput(__int64%2C+nsAutoPtr%3Cmozilla%3A%3AMediaData%3E%26)&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1 
After some additional exploratory testing, on Mac OS X 10.9.5 while in Private Browsing Window and seek into any video from http://www.kolor.com/bugreports/firefox35_mp4/, the video starts buffering and never plays. Any ideas regarding this?
Status: RESOLVED → VERIFIED
Flags: needinfo?(jyavenard)
Videos plays fine here on 10.10.1 (once I press play that is), site is a tad slow to buffer from. But nothing weird, private browsing mode or not.

Will try in a 10.9 VM tomorrow.
Flags: needinfo?(jyavenard)
See Also: → 1137100
You need to log in before you can comment on or make changes to this bug.