Closed Bug 1111311 Opened 5 years ago Closed 5 years ago

Always treat MP4's cts offset as 32 bits signed integers

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

CTS offsets are supposed to be either unsigned 32 bits integers or signed 32 bits integers.

However, various MP4 appears to use negative offsets and mark the offset as being unsigned.

As we store the negative 32 bits integer into a 64 bits signed one, we end up with extremely large cts offsets which leads to invalid pts calculations which breaks playback.

CTS offsets should be handled as 32 bits signed integers to avoid the problem
See Also: → 1111317
Here is an example where negative cts offset causes playback to stall

http://people.mozilla.org/~jyavenard/mediatest/avc3.mp4
(this is reconstructed from BBC MSE test page: http://rdmedia.bbc.co.uk/dash/ondemand/bbb/avc3/1/client_manifest-common_init.mpd)
Blocks: 1105778
Keep cts offset a 32 bits signed. This prevents issue when a time offset is incorrectly marked as version 0 which indicates that the offset is an unsigned bit. Chrome, FFmpeg, VLC all appears to handle the offset in the same fashion: always treat it as 32 bits signed
Attachment #8536246 - Flags: review?(ajones)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Comment on attachment 8536246 [details] [diff] [review]
Use 32 bits signed for storing CTS in order to properly calculate PTS

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

We need this fix in MoofParser as well.
Attachment #8536246 - Flags: review?(ajones) → review-
i had another patch for that.
I separated it.

I also believe in MPEG4Source::read, the cts handling is wrong.
should either be a int32_t there, or
replace:
mBuffer->meta_data()->setInt64(
                    kKeyTime, ((int64_t)cts * 1000000) / mTimescale);

with:
mBuffer->meta_data()->setInt64(
                    kKeyTime, ((int32_t)cts * 1000000LL) / mTimescale);
This allows the videos to play properly, as well as http://rdmedia.bbc.co.uk/dash/ondemand/bbb/avc3/1/client_manifest-common_init.mpd. The DASH pauses after a few seconds, but that's unrelated and is another problem.
Attachment #8536975 - Flags: review?(ajones)
Attachment #8536246 - Attachment is obsolete: true
Comment on attachment 8536975 [details] [diff] [review]
Use 32 bits signed for storing CTS in order to properly calculate PTS

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

::: media/libstagefright/binding/MoofParser.cpp
@@ +201,4 @@
>      if (flags & 0x800) {
>        if (version == 0) {
>          ctsOffset = reader->ReadU32();
>        } else {

Remove these 3 lines while you're at it.
Attachment #8536975 - Flags: review?(ajones) → review+
Comment on attachment 8536975 [details] [diff] [review]
Use 32 bits signed for storing CTS in order to properly calculate PTS

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

::: media/libstagefright/binding/MoofParser.cpp
@@ +201,4 @@
>      if (flags & 0x800) {
>        if (version == 0) {
>          ctsOffset = reader->ReadU32();
>        } else {

MPEG4Extractor has something similar that can also go away.

http://mxr.mozilla.org/mozilla-central/source/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#3163
Attachment #8536975 - Attachment is obsolete: true
Attachment #8537521 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/23c386e2acdc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8537539 [details] [diff] [review]
Use 32 bits signed for storing CTS in order to properly calculate PTS

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, users more likely to receive flash video from youtube.
[Describe test coverage new/current, TBPL]: landed on m-c
[Risks and why]: Low, this affects MSE-specific code.
[String/UUID change made/needed]: None.
Attachment #8537539 - Flags: approval-mozilla-aurora?
Attachment #8537539 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.