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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

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

Comment 1

[Jean-Yves Avenard [:jya]]

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)

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Use 32 bits signed for storing CTS in order to properly calculate PTS

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

Comment 3

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

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.

Comment 4

[Jean-Yves Avenard [:jya]]

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);

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Use 32 bits signed for storing CTS in order to properly calculate PTS

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.

Comment 6

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

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.

Comment 7

[Matt Woodrow (:mattwoodrow)]

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

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Use 32 bits signed for storing CTS in order to properly calculate PTS

Carrying r+

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Use 32 bits signed for storing CTS in order to properly calculate PTS

rebase

Comment 10

[Jean-Yves Avenard [:jya]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23c386e2acdc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23c386e2acdc

Comment 12

[Ralph Giles (:rillian)]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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