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

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[benjamin]

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.

Comment 1

[:Gijs (he/him)]

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.

Comment 2

[:Gijs (he/him)]

(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

Comment 3

[:Gijs (he/him)]

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? :-)

Comment 4

[Matthew Gregan [: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.

Comment 5

[Matthew Gregan [:kinetik]]

Adding others who know more in this area for opinions.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: imported patch _ensure_video_frame_pts_is_positive.patch

Under some (unknown) circumstances, IMF returns a frame with a pts negative even though input pts were correct. So treats those as 0

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: imported patch _set_pcm_output_parameters.patch

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

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8551032 [details] [diff] [review]
imported patch _set_pcm_output_parameters.patch

noticed something not quite right while uploading

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Configure WMF decoder to output PCM 16

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

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Treat negative WMF's output sample timestamp as zero

Under some (unknown) circumstances, IMF returns a frame with a pts negative even though input pts were correct. So treats those as 0

Comment 11

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

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.

Comment 12

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

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.

Comment 13

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

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...

Comment 14

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1d32c46f2143
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f16ef283fee5

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d32c46f2143
https://hg.mozilla.org/mozilla-central/rev/f16ef283fee5

Comment 16

[:Gijs (he/him)]

[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.

Comment 17

[Jean-Yves Avenard [:jya]]

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 18

[Ralph Giles (:rillian)]

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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Indeed, too late for 35

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/26e3eea17055
https://hg.mozilla.org/releases/mozilla-aurora/rev/da05f625a519

https://hg.mozilla.org/releases/mozilla-beta/rev/e017341d2486
https://hg.mozilla.org/releases/mozilla-beta/rev/cd88be2b57ac

Comment 21

[Ada [:adalucinet]]

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?

Comment 22

[Jean-Yves Avenard [:jya]]

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.