Firefox developer edition appears to have a problem with mp4.

VERIFIED FIXED in Firefox 40

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: martijn, Assigned: jya)

Tracking

({regression})

40 Branch
mozilla41
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox39 unaffected, firefox40+ verified, firefox41+ verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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.

Comment 1

4 years ago
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

Updated

4 years ago
Blocks: 1156689

Comment 3

4 years ago
Posted video example.mp4

Comment 4

4 years ago
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
(Assignee)

Comment 5

4 years ago
Properly read box's 64 bits size... How did that ever worked ?
Attachment #8615076 - Flags: review?(ajones)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
(Assignee)

Comment 6

4 years ago
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)
(Assignee)

Comment 7

4 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/dc8b730cbb6d
https://hg.mozilla.org/mozilla-central/rev/31b08880d272
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 13

4 years ago
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+
(Assignee)

Comment 16

4 years ago
(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.