Make the timestamp unroll offset consistent accross PES pids.

Make the timestamp unroll offset consistent accross PES pids.

BUG=411015
Committed: https://crrev.com/97fd1b87e604ff82db4f6dc84d1cf945e9854094
Cr-Commit-Position: refs/heads/master@{#295129}

[damienv1, 2014-09-04 21:36:50]

damienv@chromium.org changed reviewers:
+ acolwell@chromium.org, wolenetz@chromium.org

[damienv1, 2014-09-04 21:37:23]

Note: will add a timestamp_unroller_unittest in an upcoming patch.

Thanks for the review.

[wolenetz, 2014-09-05 21:18:50]

I mostly have questions related to "what exact problem are we trying to solve
here?" in relation to the spec:

https://codereview.chromium.org/539343002/diff/20001/media/formats/mp2t/times...
File media/formats/mp2t/timestamp_unroller.h (right):

https://codereview.chromium.org/539343002/diff/20001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.h:25: // where k is estimated so that the
unrolled timestamp is as close as
This seems different than expected, or I misunderstand, versus
MPEG2TS_timestampOffset in
https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/mp2t-byte-stream-...:
If a stream contains a discontinuity that is not near 2 ^ 33, then k doesn't
help. How are discontinuities like that handled?

https://codereview.chromium.org/539343002/diff/40001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/539343002/diff/40001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.cc:162:
stream_parser_buffer->SetDecodeTimestamp(
If not set, buffer's DTS will be assumed to be PTS. See
StreamParserBuffer::GetDecodeTimestamp()
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/stream_...).
These two lines are unnecessary.

https://codereview.chromium.org/539343002/diff/40001/media/formats/mp2t/mp2t_...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/539343002/diff/40001/media/formats/mp2t/mp2t_...
media/formats/mp2t/mp2t_stream_parser.cc:597: // Compensate if some timestamps
are negative.
I'm a bit confused why this compensation is required. MSE coded frame processing
algorithm and byte stream specification for mpt2s do not currently make
allowance for this, IIUC. Rather, if PTS >= 0, and DTS < 0 parse (decode) error
should result; if PTS < (0 or whatever appendWindowStart is, which is >= 0), and
DTS < 0, then either the frame is dropped without error or is partially trimmed
or used as subsequent decoder preroll (and DTS + PTS are adjusted to be >=
appendWindowStart), without error.
Do these conditions break common mp2ts, and if so, do the MSE specs need
adjustment accordingly to match this compensation logic? Or is the following
verbiage in the MSE mp2ts byte stream format spec what this compensation is
implementing for realz: "When a discontinuity is detected,
MPEG2TS_timestampOffset must be adjusted to make the timestamps after the
discontinuity appear to come immediately after the timestamps before the
discontinuity"? (And if the latter, setTimestampOffset() and abort() need to
reset time_offset_ to base::TimeDelta, IIUC.)
Either way, I would prefer you split this compensation piece out to a separate
CL.

[damienv1, 2014-09-05 21:52:03]

There was 2 possible problems:
1) first, in the previous implementation, the MPEG2 TS timestamp offset could be
different between the audio and video stream. That's an issue. Although in
almost 99% of the cases it was alright (only at discontinuity boundary, you get
a different result).

2) We have somehow to map MPEG2-TS timestamps to the media timeline. Timestamps
in MPEG-2 TS have no relationship to the Chrome media timeline.
Here the MSE spec (at least the section on MPEG-2 TS) could be more detailed and
mention what should be done regarding the global offset that needs to be
applied.
I would say that every time the stream parser is flushed (reset), the Mpeg2 TS
could offset all the timestamps so that the first DTS is always 0. (Then the JS
app can add a timeoffset to map the start correctly onto the media timeline).

https://codereview.chromium.org/539343002/diff/40001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/539343002/diff/40001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.cc:162:
stream_parser_buffer->SetDecodeTimestamp(
On 2014/09/05 21:18:49, wolenetz wrote:
> If not set, buffer's DTS will be assumed to be PTS. See
> StreamParserBuffer::GetDecodeTimestamp()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/stream_...).
> These two lines are unnecessary.

Yes, I know. But this implicit DTS assignment is not correctly implemented in
DecodeTimestamp.
e.g.
Let's say I want to add an offset on both the pts and dts outside of this file,
I'll do:
set_timestamp(timestamp() + offset)
SetDecodeTimestamp(DecodeTimestamp() + offset)
and suddenly the DTS is applied twice the offset !

So there is sth wrong in the design of PTS/DTS inside StreamParserBuffer...

https://codereview.chromium.org/539343002/diff/40001/media/formats/mp2t/mp2t_...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/539343002/diff/40001/media/formats/mp2t/mp2t_...
media/formats/mp2t/mp2t_stream_parser.cc:597: // Compensate if some timestamps
are negative.
Timestamps in MPEG-2 TS are defined modulo 2^33. So in itself, these timestamps
have no relationship at all to the media timeline that is used in Chrome.
And there is no notion of negative timestamps in MPEG-2 TS.

You need to provide a global offset that will be applied in the same way to
every single timestamp.
Here, I apply a global offset so that MSE get positive DTS.

Note: the MPEG2TS_timestampsOffset you are referring to is inside the
TimestampUnroller.

On 2014/09/05 21:18:49, wolenetz wrote:
> I'm a bit confused why this compensation is required. MSE coded frame
processing
> algorithm and byte stream specification for mpt2s do not currently make
> allowance for this, IIUC. Rather, if PTS >= 0, and DTS < 0 parse (decode)
error
> should result; if PTS < (0 or whatever appendWindowStart is, which is >= 0),
and
> DTS < 0, then either the frame is dropped without error or is partially
trimmed
> or used as subsequent decoder preroll (and DTS + PTS are adjusted to be >=
> appendWindowStart), without error.
> Do these conditions break common mp2ts, and if so, do the MSE specs need
> adjustment accordingly to match this compensation logic? Or is the following
> verbiage in the MSE mp2ts byte stream format spec what this compensation is
> implementing for realz: "When a discontinuity is detected,
> MPEG2TS_timestampOffset must be adjusted to make the timestamps after the
> discontinuity appear to come immediately after the timestamps before the
> discontinuity"? (And if the latter, setTimestampOffset() and abort() need to
> reset time_offset_ to base::TimeDelta, IIUC.)
> Either way, I would prefer you split this compensation piece out to a separate
> CL.

[damienv1, 2014-09-05 23:02:20]

https://codereview.chromium.org/539343002/diff/60001/media/formats/mp2t/mp2t_...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/539343002/diff/60001/media/formats/mp2t/mp2t_...
media/formats/mp2t/mp2t_stream_parser.cc:586: 
Remove line.

https://codereview.chromium.org/539343002/diff/60001/media/formats/mp2t/ts_se...
File media/formats/mp2t/ts_section_pes.h (right):

https://codereview.chromium.org/539343002/diff/60001/media/formats/mp2t/ts_se...
media/formats/mp2t/ts_section_pes.h:22: explicit
TsSectionPes(scoped_ptr<EsParser> es_parser,
explicit can be removed.

[damienv1, 2014-09-06 00:25:21]

Please take another look. Thanks !
This CL now only addresses the problem of consistent unrolling across the audio
and video stream.
I leave the global offset for a different CL.

[wolenetz, 2014-09-15 22:15:40]

Looking pretty good. By 'global offset', I assume you mean hooking up
FrameProcessor's discontinuity detection with logic specific to
MP2TS_timestampOffset as described in
https://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/mp2t-byte-stream-...,
right?

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
File media/formats/mp2t/timestamp_unroller.cc (right):

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:34: // Consider 3 possibilities to
estimate the missing high bits of |time|.
nit: there is no local |time|. s/time/timestamp/ ?

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:37: int64 time0 =
((previous_unrolled_time_high - 1) << nbits) | timestamp;
It looks like this could introduce a negative int64 result, if
previous_unrolled_time_high is 0, and time0 wins the diff'ing, below.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:39: int64 time2 =
((previous_unrolled_time_high + 1) << nbits) | timestamp;
It seems overflow could occur here. Impact of implementation-specific
sign-extending on bit shifting needs at least comments in this code block to
help prove correctness.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:46: if (diff0 < 0)
nit: std::abs here and below?

[damienv1, 2014-09-15 22:27:36]

Thanks for the review. Please see my comments.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/mp2t_...
File media/formats/mp2t/mp2t_stream_parser.cc (right):

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/mp2t_...
media/formats/mp2t/mp2t_stream_parser.cc:586: 
Remove extra line.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
File media/formats/mp2t/timestamp_unroller.cc (right):

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:34: // Consider 3 possibilities to
estimate the missing high bits of |time|.
On 2014/09/15 22:15:39, wolenetz wrote:
> nit: there is no local |time|. s/time/timestamp/ ?

Done.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:37: int64 time0 =
((previous_unrolled_time_high - 1) << nbits) | timestamp;
On 2014/09/15 22:15:39, wolenetz wrote:
> It looks like this could introduce a negative int64 result, if
> previous_unrolled_time_high is 0, and time0 wins the diff'ing, below.

Correct, there is nothing that prevents negativity at this level.
This module addresses only the continuity.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:39: int64 time2 =
((previous_unrolled_time_high + 1) << nbits) | timestamp;
On 2014/09/15 22:15:39, wolenetz wrote:
> It seems overflow could occur here. Impact of implementation-specific
> sign-extending on bit shifting needs at least comments in this code block to
> help prove correctness.

The underlying assumption is that 64 bit is enough to represent the timeline (an
int64 can represent several years of playback when timestamps are coded on the
90kHz scale).
I'll try to add more comments in the code.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:46: if (diff0 < 0)
On 2014/09/15 22:15:39, wolenetz wrote:
> nit: std::abs here and below?

std::abs is defined only for int/float/double.
Only c++ 11 is guaranteed to have the templated version of std::abs

So I prefer to stick to the current approach.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/ts_se...
File media/formats/mp2t/ts_section_pes.h (right):

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/ts_se...
media/formats/mp2t/ts_section_pes.h:22: explicit
TsSectionPes(scoped_ptr<EsParser> es_parser,
Remove explicit.

[wolenetz, 2014-09-15 23:24:36]

lgtm % my previous nits and the comment being updated.
From chat, we need to nail down potential ambiguity in the MSE byte stream
registry for mp2ts around offset management: 
* is audio/video parsing order deterministic (streamparser api will eventually
require non-decreasing DTS buffers across buffers across all streams within a
media segment)?
* how exactly do SourceBuffer's "sequence" appendMode's updating of the
js-visible SourceBuffer.timestampOffset interact with the js-invisible
mp2tsoffset?

These are orthogonal to this CL; damienv@ to follow-up separately, including the
possible need to make the mp2ts parser behavior compliant with non-decreasing
DTS buffer emission within a segment.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
File media/formats/mp2t/timestamp_unroller.cc (right):

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:37: int64 time0 =
((previous_unrolled_time_high - 1) << nbits) | timestamp;
On 2014/09/15 22:27:36, damienv1 wrote:
> On 2014/09/15 22:15:39, wolenetz wrote:
> > It looks like this could introduce a negative int64 result, if
> > previous_unrolled_time_high is 0, and time0 wins the diff'ing, below.
> 
> Correct, there is nothing that prevents negativity at this level.
> This module addresses only the continuity.

Acknowledged.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:39: int64 time2 =
((previous_unrolled_time_high + 1) << nbits) | timestamp;
On 2014/09/15 22:27:36, damienv1 wrote:
> On 2014/09/15 22:15:39, wolenetz wrote:
> > It seems overflow could occur here. Impact of implementation-specific
> > sign-extending on bit shifting needs at least comments in this code block to
> > help prove correctness.
> 
> The underlying assumption is that 64 bit is enough to represent the timeline
(an
> int64 can represent several years of playback when timestamps are coded on the
> 90kHz scale).
> I'll try to add more comments in the code.

Acknowledged.

https://codereview.chromium.org/539343002/diff/80001/media/formats/mp2t/times...
media/formats/mp2t/timestamp_unroller.cc:46: if (diff0 < 0)
On 2014/09/15 22:27:36, damienv1 wrote:
> On 2014/09/15 22:15:39, wolenetz wrote:
> > nit: std::abs here and below?
> 
> std::abs is defined only for int/float/double.
> Only c++ 11 is guaranteed to have the templated version of std::abs
> 
> So I prefer to stick to the current approach.

Acknowledged.

[damienv1, 2014-09-16 16:29:15]

Thanks for the review !
If no other comments, I'll merge the CL around noon.

[damienv1, 2014-09-16 17:53:25]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-09-16 17:55:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/539343002/140001

[wolenetz, 2014-09-16 17:56:16]

still lgtm % a further nit introduced since PS5:

https://codereview.chromium.org/539343002/diff/140001/media/formats/mp2t/time...
File media/formats/mp2t/timestamp_unroller.cc (right):

https://codereview.chromium.org/539343002/diff/140001/media/formats/mp2t/time...
media/formats/mp2t/timestamp_unroller.cc:41: //    of 2^33 than the one used for
the previous timestamps)
nit: s/than/as

[wolenetz, 2014-09-16 17:57:30]

The CQ bit was unchecked by wolenetz@chromium.org

[damienv1, 2014-09-16 19:01:13]

The CQ bit was checked by damienv@chromium.org

[damienv1, 2014-09-16 19:01:23]

The CQ bit was unchecked by damienv@chromium.org

[damienv1, 2014-09-16 19:01:36]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-09-16 19:04:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/539343002/160001

[commit-bot: I haz the power, 2014-09-16 19:47:18]

Committed patchset #9 (id:160001) as 1b7fe9a5d3ecd6eba947f92d50580084c180321c

[commit-bot: I haz the power, 2014-09-16 19:47:59]

Patchset 9 (id:??) landed as
https://crrev.com/97fd1b87e604ff82db4f6dc84d1cf945e9854094
Cr-Commit-Position: refs/heads/master@{#295129}

[wolenetz, 2014-09-16 21:22:56]

On 2014/09/15 23:24:36, wolenetz wrote:
> lgtm % my previous nits and the comment being updated.
> From chat, we need to nail down potential ambiguity in the MSE byte stream
> registry for mp2ts around offset management: 
> * is audio/video parsing order deterministic (streamparser api will eventually
> require non-decreasing DTS buffers across buffers across all streams within a
> media segment)?
> * how exactly do SourceBuffer's "sequence" appendMode's updating of the
> js-visible SourceBuffer.timestampOffset interact with the js-invisible
> mp2tsoffset?
> 
> These are orthogonal to this CL; damienv@ to follow-up separately, including
the
> possible need to make the mp2ts parser behavior compliant with non-decreasing
> DTS buffer emission within a segment.

I followed-up on these orthogonal questions with acolwell@:
1. mp2ts parser should emit buffers across streams in a non-decreasing decode
timestamp order. While the current StreamParser NewBuffersCB interface
partitions audio vs video vs text buffers, the parser internally must hold back
buffers from emission until it is sure that the buffer can be emitted without
another stream then emitting a buffer with lower DTS. The threshold for holding
back should be somewhere in mpeg2ts specifications (outside MSE), along the
lines of ~1 second. So processing (& emitting) buffers in stream order without
any such hold-back may cause unexpected coded frame processing algorithm
behavior further along the pipeline. Eventually, the interface for NewBuffersCB
will enforce this (see https://crbug.com/338484).

2. I was incorrectly conflating coded-frame-processing-algorithm's discontinuity
detection with mpeg2-ts-bytestream-mse-spec discontinuity. mp2ts stream parser
must handle the explicit TS "discontinuity flag" as well as 2^33 wraparound. The
internal mpeg2_timestampoffset should start at 0 and *only* become nonzero by
one of those two types of discontinuities. Further along in the pipeline, the
coded frame processing algorithm will do the usual discontinuity detection, too.