1280346 - A mp4 video is no longer playable since firefox 35

Status: VERIFIED FIXED

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

Description

[Duan Yao]

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.

Comment 1

[Duan Yao]

Attachment: a.mp4 (part1)

no longer playable since firefox 35.

Comment 2

[Duan Yao]

Attachment: a.mp4 (part2)

Please unzip a.zip.001 and a.zip.002 together with 7zip to get the test case a.mp4.

Comment 3

[Pascal Chevrel:pascalc (PTO until April 26)]

I can confirm that the video doesn't display in Firefox with 50.0a1, Chris does it fall into your scope?

Comment 4

[Simona Badau, Desktop QA]

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

Comment 5

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

jya: any ideas?

Comment 6

[Jean-Yves Avenard [:jya]]

I've started looking at it.

the video track is simply ignored here.

Comment 7

[Jean-Yves Avenard [:jya]]

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.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1280346: [mp4] Always use SPS dimensions if available.

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/

Comment 9

[Ralph Giles (:rillian)]

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.

Comment 10

[Jean-Yves Avenard [:jya]]

(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

Comment 11

[Jean-Yves Avenard [:jya]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/128c1454e51f
Only consider a video info as invalid if image dimensions are null. r=rillian

Comment 13

[Iris Hsiao [:ihsiao]]

sorry had to backout for the Mochitest failed, https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=128c1454e51f99155b4aa7222e604bd24a014fbf

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/8e9ae4616b56
Backed out changeset 128c1454e51f for Mochitest failed

Comment 15

[Jean-Yves Avenard [:jya]]

going with a different approach. much nicer and will also help issues like bug 1206948.

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63634/diff/1-2/

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63634/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/faf614ed04be
[mp4] Always use SPS dimensions if available. r=gerald,rillian

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/faf614ed04be

Comment 21

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Any plans for uplift this patch to Aurora or even Beta?

Comment 22

[Jean-Yves Avenard [:jya]]

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

Comment 23

[Gerry Chang [:gchang]]

Hi Florin,
Can we have someone to verify this in nightly first?

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Duan, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I'd like this to bake on Nightly a bit more before considering uplifting to Aurora50.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 27

[Andrei Vaida [:avaida]]

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.

Comment 28

[Duan Yao]

I also confirm this issue was fixed on 51.0a1 (2016-08-24) win64.

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(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 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

Fix was verified on Nightly by two people, Aurora50+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0bdfbe111a2

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8770016 [details]
Bug 1280346: [mp4] Always use SPS dimensions if available.

Fixing the approval flag from comment 26.

Comment 33

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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.