Closed Bug 1171067 Opened 5 years ago Closed 5 years ago

Firefox developer edition appears to have a problem with mp4.

Categories

(Core :: Audio/Video, defect)

40 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox39 --- unaffected
firefox40 + verified
firefox41 + verified

People

(Reporter: martijn, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(3 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150602004005

Steps to reproduce:

I've tried to play an mp4 video in Firefox Developer Edition (40.0a2) that works in both the regular (consumer) edition of Firefox (38.0.5) and WMP. I've uploaded the file to this location for you to reproduce:
http://51north.nl/projects/firefix/example.mp4


Actual results:

It throws an error instead of playing the video.

The console reads:
Media resource http://51north.nl/projects/firefix/example.mp4 could not be decoded.

The video player itself says:
Video can't be played because the file is corrupt.


Expected results:

I had expected the video to work in the developer edition like it does in regular Firefox.
Regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7723b15ea695&tochange=1e8d30cb367e

Probably bug 1156689.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jyavenard)
Keywords: regression
Blocks: 1156689
Attached video example.mp4
Via local build:
Last Good: d5282779b0ca
First Bad: 69d8168282fc

Triggered by: 69d8168282fc	Jean-Yves Avenard — Bug 1156689: Part8. Use new MoofParser::HasMetadata in MP4Metadata. r=kentuckyfriedtakahe
Properly read box's 64 bits size... How did that ever worked ?
Attachment #8615076 - Flags: review?(ajones)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
looking at the code, it also doesn't properly calculate sizes that are set to 0, which according to ISO/IEC 14496-12:2012: means to the end of the file.

This wouldn't work with this domain however, as the HTTP server doesn't properly report the size of the file
Flags: needinfo?(jyavenard)
Handle box's size marked as 0 as per ISO/IEC 14496-12:2012 as per subclause 4.2
Attachment #8615084 - Flags: review?(ajones)
Tracked for 40 and 41, to insure the video will play as it does in earlier versions.
Comment on attachment 8615076 [details] [diff] [review]
Properly read 64bits header's size

Review of attachment 8615076 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libstagefright/binding/Box.cpp
@@ +70,5 @@
>      MediaByteRange bigLengthRange(headerRange.mEnd,
>                                    headerRange.mEnd + sizeof(bigLength));
>      if ((mParent && !mParent->mRange.Contains(bigLengthRange)) ||
>          !byteRange->Contains(bigLengthRange) ||
> +        !mContext->mSource->CachedReadAt(aOffset+8, bigLength,

Nit: more spaces in aOffset + 8
Attachment #8615076 - Flags: review?(ajones) → review+
Comment on attachment 8615084 [details] [diff] [review]
Part2. Properly hande box size marked as 0

Review of attachment 8615084 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a test file and a quick mochitest.
Attachment #8615084 - Flags: review?(ajones) → review+
Comment on attachment 8615076 [details] [diff] [review]
Properly read 64bits header's size

Approval Request Comment
[Feature/regressing bug #]: bug 1045909
[User impact if declined]: Videos can't be played (and regression in Aurora as this code is now more used)
[Describe test coverage new/current, TreeHerder]: In m-c, manual testing.
[Risks and why]: Low. This is implementing as per specification
[String/UUID change made/needed]: None
Attachment #8615076 - Flags: approval-mozilla-beta?
Attachment #8615076 - Flags: approval-mozilla-aurora?
Jean, from comment 1 and comment 4 it sounds like 39 was unaffected by this regression. So we may only need to uplift this to aurora.
Flags: needinfo?(jyavenard)
Comment on attachment 8615076 [details] [diff] [review]
Properly read 64bits header's size

Approved for uplift to aurora.
Attachment #8615076 - Flags: approval-mozilla-beta?
Attachment #8615076 - Flags: approval-mozilla-beta-
Attachment #8615076 - Flags: approval-mozilla-aurora?
Attachment #8615076 - Flags: approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) from comment #14)
> Jean, from comment 1 and comment 4 it sounds like 39 was unaffected by this
> regression. So we may only need to uplift this to aurora.

The code is still used even in beta but only under some circumstances. It just happens to *always* be used in aurora.

I still think it should be uplifted to beta, as we won't be able to play some videos without it.
Flags: needinfo?(jyavenard)
Flags: qe-verify+
Verified fixed on Firefox 40 Beta 4 (buildID: 20150713153304) and latest Aurora 41.0a2 (buildID: 20150719004007).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.