Closed Bug 1336271 Opened 7 years ago Closed 7 years ago

mp4 video at m.ina.fr seems to not be supported by Firefox, though it plays in Chrome.

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox54 --- affected

People

(Reporter: wisniewskit, Assigned: ayang)

References

(
URL
)

Details

(Keywords: compat, stale-bug, Whiteboard: [webcompat])

Attachments

(1 file, 1 obsolete file)

Bug 1336271 - use MediaByteRangeSet to keep metadata stream in case of invalid 'ftyp' box.
59 bytes, text/x-review-board-request
Details 
The video at the bug URL cannot be played in Firefox, but can in Chrome on the same devices. It is served up from [1] (as served to me from the original page in the "see also" link). Firefox claims for me that the video MIME type is not supported, but I'm not sure if it should be or not, based on the output from mp4info:

>mp4info version 2.0.0
>/home/tom/v.mp4:
>Track	Type	Info
>1	video	H264 Main@3, 32.600 secs, 354 kbps, 384x288 @ 25.000000 fps
>2	audio	MPEG-4 AAC LC, 32.320 secs, 0 kbps, 48000 Hz
> Name: extrait_Ina
> Artist: Ina.fr
> Composer: Ina.fr
> Encoded with: Lavf51.12.1
> Comments: extrait_Ina

I've tested in Firefox 47, 51.0.1, and 54/nightly in Gentoo Linux, as well as 51.0.1 and 54/nightly in Windows 10, with the same results.

[1] https://mp4.ina.fr/lecture/lire/site/visio:1144855/securekey/fRxncV72XaQHc0Eh66vt_b51zF_cgn-I_N8onIQO8wH3_PRtcPtkYFnPopduvGRaRmptVAfr4fqe8Oi8wh6L_8sfvsABloPNF0iab68SM9Y.mp4
Also, note that the page serves a different video to Firefox on Android, but if you try to play the link in comment 0 it will not play in Android Firefox.
URL: http://m.ina.fr/video/PUB3774399020/f...
The file is improperly muxed. The File Type box (ftyp) *must* be prior the Movie Box (moov)

From ISO 14496-12.

4.3: File Type Box
"This box must be placed as early as possible in the file (e.g. after any obligatory signature, but before any
significant variable-size boxes such as a Movie Box, Media Data Box, or Free Space). It identifies which
specification is the ‘best use’ of the file"

6.2.3 Box Order
"In order to improve interoperability and utility of the files, the following rules and guidelines shall be followed
for the order of boxes:
1) The file type box ‘ftyp’ shall occur before any variable-length box (e.g. movie, free space, media
data). Only a fixed-size box such as a file signature, if required, may precede it."

Allowing the file type box to be incorrectly placed after the movie box would be opening a can of worms, as there are field in the moov that depends on the version defined in the ftyp.

Anthony, I think this should be closed as invalid, or at worse, Won't fix.

INA should be fixing their content.
It's also the first time I've ever seen such blatant error.
Status: NEW → UNCONFIRMED
Ever confirmed: false
Flags: needinfo?(ajones)
Not that I'm against this assessment, but should we also be informing the Chrome team so they can reject this content instead of playing it, for web compatibility's sake?
Flags: needinfo?(jyavenard)
This is for chrome's team to decide, we have no control over what they do. You should probably report that to them.
That this content is invalid is fully defined in the spec.
Flags: needinfo?(jyavenard)
Hmm. Alright, though I fear that getting everyone else to change seems like an uphill battle at best. The video plays without complaint in Chrome, Safari, Edge, QuickTime Player, Windows Media Player, Windows Movies & TV, VLC, mplayer...

I also get the sinking suspicion that this sort of thing could become the next "it works in everything but Firefox, so it's Firefox that sucks" situation. Maybe it would be wise to consider a compatibility spec for these things, given that we've already moved that way with CSS and JS?
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Anthony, I think this should be closed as invalid, or at worse, Won't fix.

Do you think it is easier to fix it or to get INA to fix their content?
Flags: needinfo?(ajones)
We should probably play it despite it being invalid. Ordering in MP4 files is often unpredictable and/or invalid in the real world.
Given many players can play it, for the sake of users, it would be better to try to play it unless fixing this could cause some serious side effect.
I will try to contact INA in 
https://webcompat.com/issues/4186
Keywords: compat
Priority: -- → P1
Assignee: nobody → gsquelart
Comment on attachment 8863583 [details]
Bug 1336271 - Allow 'ftyp' after 'moov' in improperly-muxed MP4s -

https://reviewboard.mozilla.org/r/135342/#review138358

I doubt that this will be sufficient.

The issue is with reading the metadata. The ftyp must be read and placed prior the moov to ensure the metadata are correctly decoded.

The other problem here is we will now parse the entire file if the ftyp isn't present.
Attachment #8863583 - Flags: review?(jyavenard)
Comment on attachment 8863583 [details]
Bug 1336271 - Allow 'ftyp' after 'moov' in improperly-muxed MP4s -

https://reviewboard.mozilla.org/r/135342/#review138358

It was sufficient to play the ina.fr video.

It's true that it will parse the entire file if there is no ftyp, but it would also already happen if there was no moov instead. ;-)
I'll let someone else implement a better fix...
Assignee: gsquelart → nobody
Status: UNCONFIRMED → NEW
Ever confirmed: true
another approach would be to totally ignore the ftyp like I did with MSE.

we don't really need it anymore. only checked by the sniffer
Flags: needinfo?(jyavenard)
Alfredo, 
What do you think?
Flags: needinfo?(ayang)
Assignee: nobody → ayang
Flags: needinfo?(ayang)
Comment on attachment 8864033 [details]
Bug 1336271 - use MediaByteRangeSet to keep metadata stream in case of invalid 'ftyp' box.

https://reviewboard.mozilla.org/r/135746/#review138776
Attachment #8864033 - Flags: review?(jyavenard) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a2d18631fac&selectedJob=96449203

Try failed at dom/media/test/test_eme_non_mse_fails.html.

E/SampleTable(55551): subsample descriptions overflow sample aux info buffer
W/MPEG4Extractor(55551): Error -1007 parsing chunck at offset 990 looking for first track

It looks like ignoring ftyp causes problem in stagefright.
Ok, I think I found the problem. Stagefright parsing 'saio' dependes on streaming absolute position. If we ignore ftyp, it causes the wrong position when parsing moov.
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c7d3f3cb9e8
ignore ftyp box when parsing mp4 metadata. r=jya
Comment on attachment 8864033 [details]
Bug 1336271 - use MediaByteRangeSet to keep metadata stream in case of invalid 'ftyp' box.

https://reviewboard.mozilla.org/r/135746/#review139516

::: media/libstagefright/binding/MoofParser.cpp:162
(Diff revisions 1 - 2)
>    RefPtr<mp4_demuxer::BlockingStream> stream = new BlockingStream(mSource);
>  
> +  mozilla::MediaByteRange initRange;
>    BoxContext context(stream, byteRanges);
>    for (Box box(&context, mOffset); box.IsAvailable(); box = box.Next()) {
> +    initRange = initRange.Span(box.Range());

but this will only works if ftyp is present.

and this is just assuming that ftyp is "somewhere" prior the moov.

The other issue with this, is that if the moov is at the end, the init range will now be the entire file, causing massive memory allocation.
Attachment #8864033 - Flags: review+
(In reply to Jean-Yves Avenard [:jya] from comment #22)
> Comment on attachment 8864033 [details]
> Bug 1336271 - ignore ftyp box when parsing mp4 metadata.
> 
> https://reviewboard.mozilla.org/r/135746/#review139516
> 
> ::: media/libstagefright/binding/MoofParser.cpp:162
> (Diff revisions 1 - 2)
> >    RefPtr<mp4_demuxer::BlockingStream> stream = new BlockingStream(mSource);
> >  
> > +  mozilla::MediaByteRange initRange;
> >    BoxContext context(stream, byteRanges);
> >    for (Box box(&context, mOffset); box.IsAvailable(); box = box.Next()) {
> > +    initRange = initRange.Span(box.Range());
> 
> but this will only works if ftyp is present.
> 
> and this is just assuming that ftyp is "somewhere" prior the moov.
> 
> The other issue with this, is that if the moov is at the end, the init range
> will now be the entire file, causing massive memory allocation.

Stagefright requires the stream from the beginning, it doesn't matter ftyp is present or not.

And yes, the init range could be the whole file, so we need a parser be able to parse moov only, for example, rust mp4 parser. :-)
But I think this problem is another issue, not relate to this bug. Maybe it is worth to do including moov only if rust mp4 parser is enable.
backed out for 07:00 < alfredo> Tomcat|Sheriffduty: could you please help to back out  https://hg.mozilla.org/integration/autoland/rev/2c7d3f3cb9e8? There could be memory issue in it.
Flags: needinfo?(ayang)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fa8134024ea
Backed out changeset 2c7d3f3cb9e8 on request for possible memory issues
Attachment #8864033 - Attachment is obsolete: true
Flags: needinfo?(ayang)
Attachment #8864033 - Flags: review?(jyavenard)
Alfredo, did this one slip through the cracks by any chance?
Flags: needinfo?(ayang)
It may get better way to fix it after bug 1370175 landed.
Flags: needinfo?(ayang)
See Also: → 1377839
It appears that they fixed the video at that link on their end, since that the video now works, even in older Firefox versions. Hopefully that doesn't mean this is now inactionable, as I'm afraid I didn't save a copy of the old file.
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
(In reply to Thomas Wisniewski from comment #29)
> It appears that they fixed the video at that link on their end

\o/
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
In bug 1423659, it removes the reading of 'ftyp' box. So I believe the original bug will be fixed though there is no bug video to verify it.
See Also: → 1423659
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: