Introduce DecodeTimestamp class to make it easier to distiguish presentation and decode timestamps.

Introduce DecodeTimestamp class to make it easier to distiguish presentation and decode timestamps.

DecodeTimestamp is a thin wrapper around base::TimeDelta that makes it
possible to easily differentiate decode timestamps from presentation
timestamps. Using a separate type also forces conversions between these
two domains to be done explicitly through conversion functions. This
change introduces the class and updates all the variables that contain
decode timestamps. It also puts in place the necessary conversion calls
for converting between timestamp types. There should be no functional
change as a result of this patch. This class is mainly to allow the
compiler to highlight where conversions between timestamp types occur.

Having DecodeTimestamp will make it much easier to rework
SourceBufferStream so that it properly handles B-frame content and
reports buffered ranges in terms of presentation time instead of
decode time.

BUG=398130
TEST=All existing tests still pass.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289031

[wolenetz, 2014-08-08 21:30:05]

First round of comments. I'll need to look more closely at source_buffer_stream*
based on your answers.

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
File media/base/stream_parser_buffer.h (right):

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
media/base/stream_parser_buffer.h:23: DecodeTimestamp() {};
lint nit: s/;//

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
media/base/stream_parser_buffer.h:52: DecodeTimestamp operator+(const
base::TimeDelta& rhs) const {
nit: Is there no expected need for adding two DTS? I'm struggling to find where
this might be useful, but if so, we could add something like:
DecodeTimestamp operator+(const DecodeTimestamp& rhs) const {
  return DecodeTimestamp(ts_ - rhs.ts_);
}

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
media/base/stream_parser_buffer.h:60: int64 operator/(const base::TimeDelta&
rhs) const {
nit: Is there no expected need for dividing or multiplying a DTS by an int? Like
above, I suppose we could add if and when that might be necessary, unless you
have specific reason why we shouldn't ever add such. (and if so, please document
that reason in code comment here)

https://codereview.chromium.org/447963003/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (left):

https://codereview.chromium.org/447963003/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:1150: 
nit: I like blank line after } in this case, and dropping this line is less
consistent with the rest of this file. Please add blank line.

https://codereview.chromium.org/447963003/diff/1/media/filters/frame_processo...
File media/filters/frame_processor.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/frame_processo...
media/filters/frame_processor.cc:438: // We use base::TimeDelta instead of
double.
nit: s/instead/and DecodeTimestamp instead/

https://codereview.chromium.org/447963003/diff/1/media/filters/frame_processor.h
File media/filters/frame_processor.h (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/frame_processo...
media/filters/frame_processor.h:101: // with timestamp |segment_timestamp|.
nit: s/with timestamp/with decode timestamp/

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:80: void Seek(DecodeTimestamp timestamp);
Ditto of .h: If eventual intent is to use PTS, use base::TimeDelta in interface
and short-term FromPTS hacks in implementation.

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:1370:
ranges_.back()->GetBufferedEndTimestamp().ToPresentationTime();;
lint nit: s/;;/;/

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.h:69: void OnNewMediaSegment(DecodeTimestamp
media_segment_start_time);
Here and below, is the eventual intent to signal DTS or PTS? If the latter, ISTM
we should keep base::TimeDelta here and put FromPTS hacks in the implementation,
not this interface.

https://codereview.chromium.org/447963003/diff/1/media/formats/mp4/track_run_...
File media/formats/mp4/track_run_iterator.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/formats/mp4/track_run_...
media/formats/mp4/track_run_iterator.cc:79: return
DecodeTimestamp::FromMicroseconds(
nit: why not FromPresentationTime(TimeDeltaFromRational(...))?

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:447: return
DecodeTimestamp::FromPresentationTime(kInfiniteDuration());
nit: We have an explicit kNoDecodeTimestamp(). Does an explicit
kInfiniteDecodeTimestamp() make sense for consistency?

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
File media/formats/webm/webm_cluster_parser.h (right):

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.h:55: // |ready_buffers_| must be empty.
Moves all |buffers_| with timestamp
nit: s/timestamp/decode timestamp/

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.h:261: // this exclusive upper bound. Set
to kNoTimestamp(), meaning not yet
nit: s/kNoTimestamp()/kNoDecodeTimestamp()/

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.h:262: // calculated, by Reset() and
Parse(). If kNoTimestamp(), then
nit: ditto

[acolwell GONE FROM CHROMIUM, 2014-08-11 17:05:06]

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
File media/base/stream_parser_buffer.h (right):

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
media/base/stream_parser_buffer.h:23: DecodeTimestamp() {};
On 2014/08/08 21:30:04, wolenetz wrote:
> lint nit: s/;//

Done.

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
media/base/stream_parser_buffer.h:52: DecodeTimestamp operator+(const
base::TimeDelta& rhs) const {
On 2014/08/08 21:30:04, wolenetz wrote:
> nit: Is there no expected need for adding two DTS? I'm struggling to find
where
> this might be useful, but if so, we could add something like:
> DecodeTimestamp operator+(const DecodeTimestamp& rhs) const {
>   return DecodeTimestamp(ts_ - rhs.ts_);
> }

I can't think of any reason to do that. I've only added operators that are
actually being used right now. We can add such an operator when it is actually
needed.

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
media/base/stream_parser_buffer.h:60: int64 operator/(const base::TimeDelta&
rhs) const {
On 2014/08/08 21:30:04, wolenetz wrote:
> nit: Is there no expected need for dividing or multiplying a DTS by an int?
Like
> above, I suppose we could add if and when that might be necessary, unless you
> have specific reason why we shouldn't ever add such. (and if so, please
document
> that reason in code comment here)

I am only implementing operators that are actually being used. When other
operators are actually needed they can be added.

https://codereview.chromium.org/447963003/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (left):

https://codereview.chromium.org/447963003/diff/1/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:1150: 
On 2014/08/08 21:30:04, wolenetz wrote:
> nit: I like blank line after } in this case, and dropping this line is less
> consistent with the rest of this file. Please add blank line.

Done.

https://codereview.chromium.org/447963003/diff/1/media/filters/frame_processo...
File media/filters/frame_processor.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/frame_processo...
media/filters/frame_processor.cc:438: // We use base::TimeDelta instead of
double.
On 2014/08/08 21:30:04, wolenetz wrote:
> nit: s/instead/and DecodeTimestamp instead/

Done.

https://codereview.chromium.org/447963003/diff/1/media/filters/frame_processor.h
File media/filters/frame_processor.h (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/frame_processo...
media/filters/frame_processor.h:101: // with timestamp |segment_timestamp|.
On 2014/08/08 21:30:04, wolenetz wrote:
> nit: s/with timestamp/with decode timestamp/

Done.

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:80: void Seek(DecodeTimestamp timestamp);
On 2014/08/08 21:30:05, wolenetz wrote:
> Ditto of .h: If eventual intent is to use PTS, use base::TimeDelta in
interface
> and short-term FromPTS hacks in implementation.

Ditto. These changes document current flow of DTS throughout the code. Fixing
http://crbug.com/398130, http://crbug.com/398141 , and http://crbug.com/402502
will ultimately force this to get changed to a base::TimeDelta. For now, I think
it is better to actually document what type is really being used for all calls
to this method by making it clear in the signature.

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:1370:
ranges_.back()->GetBufferedEndTimestamp().ToPresentationTime();;
On 2014/08/08 21:30:04, wolenetz wrote:
> lint nit: s/;;/;/

Done.

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.h:69: void OnNewMediaSegment(DecodeTimestamp
media_segment_start_time);
On 2014/08/08 21:30:05, wolenetz wrote:
> Here and below, is the eventual intent to signal DTS or PTS? If the latter,
ISTM
> we should keep base::TimeDelta here and put FromPTS hacks in the
implementation,
> not this interface.

But the ultimate caller (frame_processor.cc) is passing a DTS into this code.
For now this change is intended to document the flow of DTS through the
different layers of code. Ultimately this should be PTS, but I think this
interface should reflect what is actually being passed in right now. I've filed
http://crbug.com/402502 to track changing this code and added a comment
referring to this bug.

https://codereview.chromium.org/447963003/diff/1/media/formats/mp4/track_run_...
File media/formats/mp4/track_run_iterator.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/formats/mp4/track_run_...
media/formats/mp4/track_run_iterator.cc:79: return
DecodeTimestamp::FromMicroseconds(
On 2014/08/08 21:30:05, wolenetz wrote:
> nit: why not FromPresentationTime(TimeDeltaFromRational(...))?

Done.

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:447: return
DecodeTimestamp::FromPresentationTime(kInfiniteDuration());
On 2014/08/08 21:30:05, wolenetz wrote:
> nit: We have an explicit kNoDecodeTimestamp(). Does an explicit
> kInfiniteDecodeTimestamp() make sense for consistency?

I don't think it is worth it since I'm pretty sure this is the only usage. I've
changed the code to explicitly use base::TimeDelta::Max() since really that is
what this code cares about and it doesn't really have anything to do with
duration.

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
File media/formats/webm/webm_cluster_parser.h (right):

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.h:55: // |ready_buffers_| must be empty.
Moves all |buffers_| with timestamp
On 2014/08/08 21:30:05, wolenetz wrote:
> nit: s/timestamp/decode timestamp/

Done.

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.h:261: // this exclusive upper bound. Set
to kNoTimestamp(), meaning not yet
On 2014/08/08 21:30:05, wolenetz wrote:
> nit: s/kNoTimestamp()/kNoDecodeTimestamp()/

Done.

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.h:262: // calculated, by Reset() and
Parse(). If kNoTimestamp(), then
On 2014/08/08 21:30:05, wolenetz wrote:
> nit: ditto

Done.

[wolenetz, 2014-08-12 00:09:01]

lgtm % nits:

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
File media/base/stream_parser_buffer.h (right):

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
media/base/stream_parser_buffer.h:52: DecodeTimestamp operator+(const
base::TimeDelta& rhs) const {
On 2014/08/11 17:05:05, acolwell wrote:
> On 2014/08/08 21:30:04, wolenetz wrote:
> > nit: Is there no expected need for adding two DTS? I'm struggling to find
> where
> > this might be useful, but if so, we could add something like:
> > DecodeTimestamp operator+(const DecodeTimestamp& rhs) const {
> >   return DecodeTimestamp(ts_ - rhs.ts_);
> > }
> 
> I can't think of any reason to do that. I've only added operators that are
> actually being used right now. We can add such an operator when it is actually
> needed.

Acknowledged.

https://codereview.chromium.org/447963003/diff/1/media/base/stream_parser_buf...
media/base/stream_parser_buffer.h:60: int64 operator/(const base::TimeDelta&
rhs) const {
On 2014/08/11 17:05:05, acolwell wrote:
> On 2014/08/08 21:30:04, wolenetz wrote:
> > nit: Is there no expected need for dividing or multiplying a DTS by an int?
> Like
> > above, I suppose we could add if and when that might be necessary, unless
you
> > have specific reason why we shouldn't ever add such. (and if so, please
> document
> > that reason in code comment here)
> 
> I am only implementing operators that are actually being used. When other
> operators are actually needed they can be added.

Acknowledged.

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.cc:80: void Seek(DecodeTimestamp timestamp);
On 2014/08/11 17:05:06, acolwell wrote:
> On 2014/08/08 21:30:05, wolenetz wrote:
> > Ditto of .h: If eventual intent is to use PTS, use base::TimeDelta in
> interface
> > and short-term FromPTS hacks in implementation.
> 
> Ditto. These changes document current flow of DTS throughout the code. Fixing
> http://crbug.com/398130, http://crbug.com/398141 , and http://crbug.com/402502
> will ultimately force this to get changed to a base::TimeDelta. For now, I
think
> it is better to actually document what type is really being used for all calls
> to this method by making it clear in the signature.

Acknowledged.

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/447963003/diff/1/media/filters/source_buffer_...
media/filters/source_buffer_stream.h:69: void OnNewMediaSegment(DecodeTimestamp
media_segment_start_time);
On 2014/08/11 17:05:06, acolwell wrote:
> On 2014/08/08 21:30:05, wolenetz wrote:
> > Here and below, is the eventual intent to signal DTS or PTS? If the latter,
> ISTM
> > we should keep base::TimeDelta here and put FromPTS hacks in the
> implementation,
> > not this interface.
> 
> But the ultimate caller (frame_processor.cc) is passing a DTS into this code.
> For now this change is intended to document the flow of DTS through the
> different layers of code. Ultimately this should be PTS, but I think this
> interface should reflect what is actually being passed in right now. I've
filed
> http://crbug.com/402502 to track changing this code and added a comment
> referring to this bug.

Acknowledged.

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
File media/formats/webm/webm_cluster_parser.cc (right):

https://codereview.chromium.org/447963003/diff/1/media/formats/webm/webm_clus...
media/formats/webm/webm_cluster_parser.cc:447: return
DecodeTimestamp::FromPresentationTime(kInfiniteDuration());
On 2014/08/11 17:05:06, acolwell wrote:
> On 2014/08/08 21:30:05, wolenetz wrote:
> > nit: We have an explicit kNoDecodeTimestamp(). Does an explicit
> > kInfiniteDecodeTimestamp() make sense for consistency?
> 
> I don't think it is worth it since I'm pretty sure this is the only usage.
I've
> changed the code to explicitly use base::TimeDelta::Max() since really that is
> what this code cares about and it doesn't really have anything to do with
> duration.

Acknowledged.

https://codereview.chromium.org/447963003/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/447963003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:306: const media::DecodeTimestamp&
decode_timestamp,
nit: s/media:://

https://codereview.chromium.org/447963003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1140: // Delete all ranges that begin
after |duration|.
nit: s/duration/duration_dts/ for now at least.

[acolwell GONE FROM CHROMIUM, 2014-08-12 01:47:10]

Thanks for the review.

https://codereview.chromium.org/447963003/diff/20001/media/filters/source_buf...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/447963003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:306: const media::DecodeTimestamp&
decode_timestamp,
On 2014/08/12 00:09:00, wolenetz wrote:
> nit: s/media:://

Done.

https://codereview.chromium.org/447963003/diff/20001/media/filters/source_buf...
media/filters/source_buffer_stream.cc:1140: // Delete all ranges that begin
after |duration|.
On 2014/08/12 00:09:00, wolenetz wrote:
> nit: s/duration/duration_dts/ for now at least.

Done.

[acolwell GONE FROM CHROMIUM, 2014-08-12 01:47:15]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-08-12 01:56:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/447963003/60001

[commit-bot: I haz the power, 2014-08-12 04:57:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-12 07:23:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-12 07:23:21]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[acolwell GONE FROM CHROMIUM, 2014-08-12 16:47:24]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-08-12 16:50:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/447963003/60001

[commit-bot: I haz the power, 2014-08-12 18:29:54]

Change committed as 289031

[damienv1, 2014-08-14 23:11:33]

https://codereview.chromium.org/447963003/diff/60001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.cc (left):

https://codereview.chromium.org/447963003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.cc:169:
stream_parser_buffer->SetDecodeTimestamp(current_pts);
Should not be removed: most audio streams in Mpeg2 TS do not have dts, in that
case, dts is equal to the pts.

[wolenetz, 2014-08-14 23:32:51]

https://codereview.chromium.org/447963003/diff/60001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.cc (left):

https://codereview.chromium.org/447963003/diff/60001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.cc:169:
stream_parser_buffer->SetDecodeTimestamp(current_pts);
On 2014/08/14 23:11:33, damienv1 wrote:
> Should not be removed: most audio streams in Mpeg2 TS do not have dts, in that
> case, dts is equal to the pts.

If not explicitly set (or if kNoDecodeTimestamp()), then dts is taken from pts.
So if you have no differing dts to set vs pts, you don't need to set it.