Closed Bug 1280346 Opened 4 years ago Closed 3 years ago

A mp4 video is no longer playable since firefox 35

Categories

(Core :: Audio/Video: Playback, defect, P1)

35 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 --- fix-optional
firefox50 --- verified
firefox51 --- verified

People

(Reporter: duanyao.ustc, Assigned: jya)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

Play the MP4 file attached with firefox.


Actual results:

No video is shown (audio is OK).

I tested multiple versions of firefox (all tests are performed on Windows 7 and 10 64bit), the result is:

  Firefox 33.1.1 and 34.0.5: play OK.
  Firefox 35.0.1, 40.0.3, 47.0, and 49a1: no video.

So it seems a regression between 34.0.5 and 35.0.1 . 

If I re-mux this file with ffmpeg, it is playable with firefox 35+:

  ffmpeg -i a.mp4 -c copy b.mp4

So I think this is an issue in firefox's mp4 demuxer.


Expected results:

The video should play normally.
Attached file a.mp4 (part1)
no longer playable since firefox 35.
Attached file a.mp4 (part2)
Please unzip a.zip.001 and a.zip.002 together with 7zip to get the test case a.mp4.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 49 Branch → 35 Branch
I can confirm that the video doesn't display in Firefox with 50.0a1, Chris does it fall into your scope?
Flags: needinfo?(cpearce)
Using the mozregression tool narrowed the regression window to:
Last good revision: dc352a7bf234 (2014-08-26)
First bad revision: 0753f7b93ab7 (2014-08-27)
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dc352a7bf234&tochange=0753f7b93ab7
jya: any ideas?
Flags: needinfo?(cpearce) → needinfo?(jyavenard)
Priority: -- → P1
I've started looking at it.

the video track is simply ignored here.
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
the main metadata indicate that the video dimensions are 0x288 and stagefright just ignores it.

The regression was introduced when we enabled the MP4Reader (bug 1057879).

I see that the [avc1] atom also contains the dimensions ; I'll see what ffmpeg does here.
Display dimensions are actually determined from the SPS NAL with h264 and as such we don't really care on what is found in the container (which may be incorrect).

Review commit: https://reviewboard.mozilla.org/r/63634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63634/
Attachment #8770016 - Flags: review?(giles)
Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

https://reviewboard.mozilla.org/r/63634/#review60682

Do we do something sane with the display image? I thought that was used for cropping in some streams.
Attachment #8770016 - Flags: review?(giles) → review+
(In reply to Ralph Giles (:rillian) needinfo me from comment #9)

> Do we do something sane with the display image? I thought that was used for
> cropping in some streams.

the only tests done was checking that the dimensions were non-zero but that's also the info used to determine if we had a video track or not.

For other video codecs (vpX) display dimensions == picture dimensions so I don't believe anything more need to be done.

if the display dimensions are rubbish, it should be handled by layout
https://reviewboard.mozilla.org/r/63634/#review60682

We do.. But as far as validations is concerned, they should never be 0 unless the picture image is itself 0
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/128c1454e51f
Only consider a video info as invalid if image dimensions are null. r=rillian
sorry had to backout for the Mochitest failed, https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=128c1454e51f99155b4aa7222e604bd24a014fbf
Flags: needinfo?(jyavenard)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/8e9ae4616b56
Backed out changeset 128c1454e51f for Mochitest failed
going with a different approach. much nicer and will also help issues like bug 1206948.
Flags: needinfo?(jyavenard)
Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

https://reviewboard.mozilla.org/r/63634/#review70922

r+ with style nit:

::: media/libstagefright/binding/DecoderData.cpp:214
(Diff revision 2)
>  
>  bool
>  MP4VideoInfo::IsValid() const
>  {
> -  return mDisplay.width > 0 && mDisplay.height > 0;
> +  return (mDisplay.width > 0 && mDisplay.height > 0)
> +    || (mImage.width > 0 && mImage.height > 0);

Here you put '||' at the start of a new line (which I personally prefer). But higher in the file, and in MP4Demuxer.cpp, the operators are at the end of lines. Please go for consistency.
Attachment #8770016 - Flags: review?(gsquelart) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/faf614ed04be
[mp4] Always use SPS dimensions if available. r=gerald,rillian
https://hg.mozilla.org/mozilla-central/rev/faf614ed04be
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Any plans for uplift this patch to Aurora or even Beta?
Blocks: 1057879
Flags: needinfo?(jyavenard)
Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

Approval Request Comment
[Feature/regressing bug #]:1057879
[User impact if declined]: some mp4 will not play
[Describe test coverage new/current, TreeHerder]: in central; lots of mochitests
[Risks and why]: Low, we already use that code path later down the track, we just moved it up slightly. 
[String/UUID change made/needed]: none
Flags: needinfo?(jyavenard)
Attachment #8770016 - Flags: approval-mozilla-beta?
Attachment #8770016 - Flags: approval-mozilla-aurora?
Hi Florin,
Can we have someone to verify this in nightly first?
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Hello Duan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(duanyao.ustc)
I'd like this to bake on Nightly a bit more before considering uplifting to Aurora50.
It will be nice to fix. but it's been a regression for some time now so it seems reasonable to keep it in 50 or 51.
Flags: needinfo?(florin.mezei) → needinfo?(andrei.vaida)
I've managed to reproduce this bug using the sample *.mp4 file from
Comment 1 and Comment 2, on 47.0-build3(20160604131506).

This is verified fixed on 51.0a1 (2016-08-24), using Windows 10 x64,
Mac OS X 10.11.1 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: needinfo?(andrei.vaida)
I also confirm this issue was fixed on 51.0a1 (2016-08-24) win64.
Flags: needinfo?(duanyao.ustc)
(In reply to Duan Yao from comment #28)
> I also confirm this issue was fixed on 51.0a1 (2016-08-24) win64.

Fantastic! Thanks for the quick follow up.
Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

Fix was verified on Nightly by two people, Aurora50+
Attachment #8770016 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

Fixing the approval flag from comment 26.
Attachment #8770016 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
I managed to reproduce this issue on 47.0a2 (2016-04-21), using the .mp4 attachment from comment 1 and comment 2.
The bug is verified fixed on 50.0b1 build2 (20160920155715), using
- Windows 10 x64
- Ubuntu 14.04 x86
- Mac OS X 10.11. 
I will set the corresponding flags.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.