1253471 - Embeded video returns an error in Firefox

Status: RESOLVED FIXED

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

Description

[EugeneB]

Attachment: mediainfo_log

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

1. Embed a video directly into a Webpage. 

<iframe src='http://embed.azubu.tv/video188762SVgameshowcsgoen' frameborder='0' width='960' height='540' allowfullscreen webkitallowfullscreen mozallowfullscreen></iframe>



Actual results:

There is a decode error for the next Firefox releases:

- Firefox 44.0 +
https://gyazo.com/7ef4d6bc8afd7a5885adfd988481866f

- Firefox 45.0 Beta
https://gyazo.com/e57f592fb749fa972d4165cbd0287951

- Firefox 47.0a1 nightly (2016-03-03)
https://gyazo.com/7097d5434b0717d8f388ee31634664dd


But the video in question has no playback issues on the outdated versions of Firefox:

- Firefox 43.0, 43.03.3
https://gyazo.com/0dbacefbd5cb3f916dd50efb888299fd

It appears that somewhere between version 43 and 44 of Firefox, there was a change to media handling to have the decode error. 


Expected results:

There are no playback issues.

Comment 1

[Loic]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0a1ac3ab8cb90dc9d26bd5702e43f6a51ec018d1&tochange=2a57c0a0cf19b32f5fa9cb72c44a92e3319d14a0

Gerald Squelart — Bug 1232069 - Check box sizes before alloc&copy. r=jya

Comment 2

[Jean-Yves Avenard [:jya]]

if this video doesnt play because of this change it means that the mp4 contains invalid box and is deemed a security risk and as such now refused

Comment 3

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

Debugging a little bit, I see a "moov" box that is 41,543,807 bytes long, or about 40MB, but we have set a limit of 32MB.
(Note: the stream size is >24TB! But it's a TV stream, so I'm guessing they just want to make it pseudo-infinite.)

Jean-Yves, I see you first introduced this 32MB limit in bug 1149278 (which was then reused in bug 1232069, to sanity-check the metadata range before processing). Any particular reason for this number?

Comment 4

[Jean-Yves Avenard [:jya]]

We have to copy the metadata and keep it in RAM.

It was felt that 32MB limit was appropriate, seeing that IIRC, a 60fps video sample table size would exceed a 32MB after about 5 years.

Comment 5

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

Is this really a regression then or is it intended behavior?

Comment 6

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

A bit more data, after relaxing the limit to 64MB:

The data for the video 'trak' takes 22,466,470 bytes, including 18,720,652 for the 'stsz' (table of sizes of all samples), ~2MB in 'stsc' (sample to chunk map) and ~1.5MB in 'co64' (64-bit chunk offsets).
There are 4,680,158 samples, the total duration is 93,603,160,000 us (about 26 hours), that's a framerate of 50Hz.
(jya: How did you get 5 years from 32MB?)

Similar deal on the audio side: trak=~19MB with stsz=~17MB, co64=~1.5MB. 4,387,650 samples.

After parsing, the exported video index table (needed during playback) takes ~214MB (sizeof(MediaSource::Indice)=48, *4.7M).

So what do you think Jean-Yves? Should we increase our limit to allow this kind of video? -- Please let me know if you'd like more details.

Comment 7

[Jean-Yves Avenard [:jya]]

Depends if we're happy to keep a copy of a 64MB blob in RAM on top of an enormous sample table. 

I didn't come up with the 32MB limit. Anthony wanted 16MB to start with. 

I leave that decision to him

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Depends if we're happy to keep a copy of a 64MB blob in RAM on top of an
> enormous sample table. 
> 
> I didn't come up with the 32MB limit. Anthony wanted 16MB to start with. 
> 
> I leave that decision to him

Why do we keep a copy of the moov in RAM?

Comment 9

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

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #8)
> Why do we keep a copy of the moov in RAM?

Though it is a valid question, the copy of the moov is only 15% of the memory taken by the metadata structures for this 26-hour video.
We might indeed want to look at some optimizations (e.g., getting rid of the source metadata after parsing, shrinking the exported index table, etc.)

But in any case the current 32MB box limit is preventing us from playing some videos.

So I think a decision is needed, whether we accept video streams with humongous metadata (and do whatever we can to play them now, even if we could optimize some things), or keep rejecting them and let other browsers deal with them for now.

Comment 10

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Gerald Squelart [:gerald] from comment #9)
> So I think a decision is needed, whether we accept video streams with
> humongous metadata (and do whatever we can to play them now, even if we
> could optimize some things), or keep rejecting them and let other browsers
> deal with them for now.

Lets just make the limit several times larger.

Comment 11

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

Attachment: MozReview Request: Bug 1253471 - Bump box size limit to 128MB - r?k17e

Review commit: https://reviewboard.mozilla.org/r/38395/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38395/

Comment 12

[Benjamin Kerensa [:bkerensa]]

Gerald, 

Could you answer Comment 5 please?

Comment 13

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

(In reply to Benjamin Kerensa [:bkerensa] from comment #12)
> Could you answer Comment 5 please?

Sorry, it slipped from my radar. Sure:


(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #5)
> Is this really a regression then or is it intended behavior?

I'd say it's intended behavior (limit mp4 boxes to what was thought "reasonable", to prevent OOM attacks) that introduced a regression (some videos that used to be playable became unplayable).

If you absolutely need to classify it one way or another, I'd probably go with the worst option: Regression.

Note that technically, even with the proposed patch the regression is still there: Videos with >128MB of metadata (corresponding to ~83 hours @ 50Hz) won't be played. But this seems even more unlikely than videos with >32MB of metadata (~21h@50Hz).
So we may want to revisit this limit, probably as part of working on follow-up bug 1253974; I'll add a note there.

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to Gerald Squelart [:gerald] from comment #9)
> > Why do we keep a copy of the moov in RAM?
> 
> Though it is a valid question, the copy of the moov is only 15% of the
> memory taken by the metadata structures for this 26-hour video.
> We might indeed want to look at some optimizations (e.g., getting rid of the
> source metadata after parsing, shrinking the exported index table, etc.)

My memory faulted me here.

This constant used to only be used when server didn't report a Content-Length nor a Content-Range, and we had to continue reading all boxes to find the moov.

Seeing that this video in particular would have been unplayable anyway as the server wouldn't have supported seeking, a 32MB limit is perfectly acceptable.

In fact it could be much less IMHO.

It's the security fix in bug 1232069 that extended the use of this constant that created the regression. It affects all version from 44 as it was uplifted there at the time.

Comment 15

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

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> (In reply to Gerald Squelart [:gerald] from comment #9)
> It's the security fix in bug 1232069 that extended the use of this constant
> that created the regression. It affects all version from 44 as it was
> uplifted there at the time.

After further discussion with Jean-Yves, I see that this limit shouldn't have been used in the Metadata extraction routine.
I am preparing a second patch to remove this limit there (but still check for overflows&truncations, and of course actual OOMs).
After this, we can consider the regression fully fixed.

Comment 16

[Loic]

Can you add bug 1232069 to the "Blocks" field, please. I don't have permission for that.

Comment 17

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

(In reply to Loic from comment #16)
> Can you add bug 1232069 to the "Blocks" field, please. I don't have
> permission for that.

I assume you meant "Depends on", as bug 1232069 is already fixed & landed in 44+, so it can't be blocked by this bug here.

Comment 18

[Loic]

I have always thought the logic about regression was to add the regressing bug (here bug 1232069) in the blocks field. :)

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8727215 [details]
MozReview Request: Bug 1253471 - Bump box size limit to 128MB - r?k17e

https://reviewboard.mozilla.org/r/38395/#review35131

Comment 21

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

Tracking then for 46+ - and this seems simple enough to uplift once it lands.

Comment 22

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

(In reply to Loic from comment #18)
> I have always thought the logic about regression was to add the regressing
> bug (here bug 1232069) in the blocks field. :)

The logic still feels backwards to me, but I see it's the accepted way in these lands, so I'll follow the herd. ;-)

Comment 23

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

Comment on attachment 8727215 [details]
MozReview Request: Bug 1253471 - Bump box size limit to 128MB - r?k17e

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

Comment 24

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

Attachment: MozReview Request: Bug 1253471 - Remove Metadata hard-coded limit - r?jya

Instead of relying on some arbitrary limit for ftyp+moov box sizes, we check
for overflow and possible type truncations, and then let memory allocation
routines (e.g. MediaByteBuffer::SetLength) deal with actual memory limitations.

Review commit: https://reviewboard.mozilla.org/r/38613/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38613/

Comment 25

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1697ce02d77f

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8727705 [details]
MozReview Request: Bug 1253471 - Remove Metadata hard-coded limit - r?jya

https://reviewboard.mozilla.org/r/38613/#review35259

::: media/libstagefright/binding/MoofParser.cpp:199
(Diff revision 1)
> -  if (!metadata->SetLength(ftyp.Length() + moov.Length(), fallible)) {
> +  if (!metadata->SetLength(totalLength.value(), fallible)) {

nit: could leave those as is as we now longer care for overflow.

make the patch smaller

Comment 27

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/38395/#review35261

::: media/libstagefright/binding/Box.cpp:18
(Diff revision 2)
> -const uint64_t Box::kMAX_BOX_READ = 32 * 1024 * 1024;
> +const uint64_t Box::kMAX_BOX_READ = 128 * 1024 * 1024;

are you still going on with those?

imho you shouldn't

Comment 28

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

Comment on attachment 8727215 [details]
MozReview Request: Bug 1253471 - Bump box size limit to 128MB - r?k17e

(In reply to Jean-Yves Avenard [:jya] from comment #27)
> https://reviewboard.mozilla.org/r/38395/#review35261
> 
> ::: media/libstagefright/binding/Box.cpp:18
> (Diff revision 2)
> > -const uint64_t Box::kMAX_BOX_READ = 32 * 1024 * 1024;
> > +const uint64_t Box::kMAX_BOX_READ = 128 * 1024 * 1024;
> 
> are you still going on with those?
> 
> imho you shouldn't

Anthony gave us the green light to do what we think is best, so I'm obsoleting this patch.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08207e3060c

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 31

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

Comment on attachment 8727705 [details]
MozReview Request: Bug 1253471 - Remove Metadata hard-coded limit - r?jya

Approval Request Comment
[Feature/regressing bug #]: Bug 1232069 prevents long videos from playing

[User impact if declined]: Some sites with very long videos will keep showing a misleading "decode error" (they work in other browsers)

[Describe test coverage new/current, TreeHerder]: Lots of media tests that exercize the MP4 metadata parser, Aurora try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=acdfb6f79b01 , Beta try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=51d0be116d60

[Risks and why]: Some risks associated with new code that checks for potentially-harmful overflows&truncations, but the use of CheckedInt clearly shows what is being tested, and that code has been exercized through try & local testing (in particular, verified that the solution to bug 1232069 is still working as intended).

[String/UUID change made/needed]: None.


(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> Tracking then for 46+ - and this seems simple enough to uplift once it lands.

Please note that a different patch (from the time of that comment) has landed, which is not as simple.
I still believe it should be uplifted to re-enable playback on some websites.

Comment 32

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

EugeneB, could you please verify this issue is fixed as expected on a latest Nightly build, say 03/09/2016? Thanks!

Comment 33

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

Comment on attachment 8727705 [details]
MozReview Request: Bug 1253471 - Remove Metadata hard-coded limit - r?jya

Improves playback of large videos (>32 MB), taking it in Aurora47 and Beta46.

Comment 34

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

(In reply to Ritu Kothari (:ritu) from comment #33)
> Improves playback of large videos (>32 MB), taking it in Aurora47 and Beta46.

Thank you Ritu.

Just to be clear: The 32 MB limit was just for parts of the metadata ('ftyp' and 'moov' boxes), the full videos would be orders of magnitude larger, more than 20 hours long (which could be anything from GBs to TBs depending on duration, resolution and other factors).

Comment 35

[EugeneB]

(In reply to Ritu Kothari (:ritu) from comment #33)

The initial issue was not reproduced on a latest Nightly build 48.0a1(2016-03-10).

flags: needinfo?([e-mail])

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1f8fe089c3d2