Enable round-tripping and updating of WebSourceBufferImpl timestamp offset

Enable round-tripping and updating of WebSourceBufferImpl timestamp offset

This is the first part of a three-sided MediaSource
Chromium/Blink+roll Blink/Chromium set of changes to enable
JavaScript web apps to continue to round-trip
SourceBuffer.timestampOffset set/get if SourceBuffer append
operations do not update the timestamp offset, and to
retrieve any updated timestamp offset as can occur during
upcoming "sequence" append mode compliant coded frame
processing.

R=acolwell@chromium.org
BUG=249422
TEST=All media_unittests and http/tests/media/media-source layout tests pass locally on Linux

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

[wolenetz, 2014-02-25 02:27:48]

PTAL. Thanks!

[wolenetz, 2014-02-25 03:25:42]

On 2014/02/25 02:27:48, wolenetz wrote:
> PTAL. Thanks!

Hold on. Please don't review PS2. Blink-side change that enables this revealed
in layout tests that the default initialized value for
|user_specified_timestamp_offset_| was not 0.0, but something like -2.xe-98.
I'll have a fix shortly.

[wolenetz, 2014-02-25 03:44:47]

PTAL @ PS3. Thank you.

[acolwell GONE FROM CHROMIUM, 2014-02-25 17:24:27]

https://codereview.chromium.org/178153004/diff/60001/content/renderer/media/w...
File content/renderer/media/websourcebuffer_impl.h (right):

https://codereview.chromium.org/178153004/diff/60001/content/renderer/media/w...
content/renderer/media/websourcebuffer_impl.h:32: virtual double
getTimestampOffset();
nit: s/getT/t/

https://codereview.chromium.org/178153004/diff/60001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/178153004/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:441: return user_specified_timestamp_offset_;
I think we should just return NaN here. Blink already knows what the user
specified one is. We only really need to signal when CFP updates the timestamp
offset. ISTM that the bool should just keep track of whether the CFP has updated
the timestamp offset. Perhaps rename this method to GetUpdatedTimestampOffset()?

It also might be worth keeping all this code in terms of TimeDelta and keeping
all the double business in WebSourceBufferImpl. kNoTimestamp() could be the
placeholder for NaN.

https://codereview.chromium.org/178153004/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:442: else
nit: drop else.

[wolenetz, 2014-02-25 20:32:20]

PTAL @ PS4. Thank you.

https://codereview.chromium.org/178153004/diff/60001/content/renderer/media/w...
File content/renderer/media/websourcebuffer_impl.h (right):

https://codereview.chromium.org/178153004/diff/60001/content/renderer/media/w...
content/renderer/media/websourcebuffer_impl.h:32: virtual double
getTimestampOffset();
On 2014/02/25 17:24:28, acolwell wrote:
> nit: s/getT/t/

Dropped 'get'. I added 'updated' to follow the convention of the underlying
ChunkDemuxer and SourceState implementations.

https://codereview.chromium.org/178153004/diff/60001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/178153004/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:441: return user_specified_timestamp_offset_;
On 2014/02/25 17:24:28, acolwell wrote:
> I think we should just return NaN here. Blink already knows what the user
> specified one is. We only really need to signal when CFP updates the timestamp
> offset. ISTM that the bool should just keep track of whether the CFP has
updated
> the timestamp offset. Perhaps rename this method to
GetUpdatedTimestampOffset()?
> 
> It also might be worth keeping all this code in terms of TimeDelta and keeping
> all the double business in WebSourceBufferImpl. kNoTimestamp() could be the
> placeholder for NaN.

Done, using TimeDelta, name change, and kNoTimestamp() to indicate no change has
occurred since construction or last SetTimestampOffset().

https://codereview.chromium.org/178153004/diff/60001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:442: else
On 2014/02/25 17:24:28, acolwell wrote:
> nit: drop else.

Done.

[acolwell GONE FROM CHROMIUM, 2014-02-25 21:50:41]

lgtm % nit

https://codereview.chromium.org/178153004/diff/80001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/178153004/diff/80001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:128: // If
|using_user_specified_timestamp_offset_| is true, meaning
nit: I wonder if the name should be something like
timestamp_offset_updated_by_append_. It just seems like 'updated' should be in
the name of the var that controls the return value of this function.

[wolenetz, 2014-02-25 22:56:29]

Thanks. Currently holding off on sending to CQ until the "is NaN the best way to
signal" question is resolved on the Blink CL @
https://codereview.chromium.org/178763006/#msg8

https://codereview.chromium.org/178153004/diff/80001/media/filters/chunk_demu...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/178153004/diff/80001/media/filters/chunk_demu...
media/filters/chunk_demuxer.cc:128: // If
|using_user_specified_timestamp_offset_| is true, meaning
On 2014/02/25 21:50:41, acolwell wrote:
> nit: I wonder if the name should be something like
> timestamp_offset_updated_by_append_. It just seems like 'updated' should be in
> the name of the var that controls the return value of this function.

Done. That does make more sense :)

[wolenetz, 2014-02-26 23:04:22]

PTAL @ PS6. I've reworked it to follow the suggestion made on the blink side CL
@ https://codereview.chromium.org/178763006/#msg10

[acolwell GONE FROM CHROMIUM, 2014-02-26 23:43:50]

https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:117: TimeDelta* new_timestamp_offset);
nit: Any reason not to use kNoTimestamp() to signal "not updated"?

https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:1449: return timestamp_offset_updated;
nit: Just return false here.

[wolenetz, 2014-02-27 00:54:42]

On 2014/02/26 23:43:50, acolwell wrote:
>
https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
> File media/filters/chunk_demuxer.cc (right):
> 
>
https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
> media/filters/chunk_demuxer.cc:117: TimeDelta* new_timestamp_offset);
> nit: Any reason not to use kNoTimestamp() to signal "not updated"?
> 
>
https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
> media/filters/chunk_demuxer.cc:1449: return timestamp_offset_updated;
> nit: Just return false here.

Woops - I accidentally deleted that PS6. I've uploaded a new one with fixes for
these comments.

[wolenetz, 2014-02-27 01:05:34]

On 2014/02/27 00:54:42, wolenetz wrote:
> On 2014/02/26 23:43:50, acolwell wrote:
> >
>
https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
> > File media/filters/chunk_demuxer.cc (right):
> > 
> >
>
https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
> > media/filters/chunk_demuxer.cc:117: TimeDelta* new_timestamp_offset);
> > nit: Any reason not to use kNoTimestamp() to signal "not updated"?
> > 
> >
>
https://codereview.chromium.org/178153004/diff/130001/media/filters/chunk_dem...
> > media/filters/chunk_demuxer.cc:1449: return timestamp_offset_updated;
> > nit: Just return false here.
> 
> Woops - I accidentally deleted that PS6. I've uploaded a new one with fixes
for
> these comments.

s/I've/I'll/ :/

[wolenetz, 2014-02-27 02:15:41]

New PS6 posted. PTAL. Note, the Blink-side interface change for WSB::append() is
still in discussion, so this PS6 may need to change further.

[wolenetz, 2014-02-27 03:32:31]

Please take a look @ patch set 7: it is aligned with the current Blink-side
change (https://codereview.chromium.org/178763006/ PS6).

Thank you.

[acolwell GONE FROM CHROMIUM, 2014-02-27 18:27:21]

https://codereview.chromium.org/178153004/diff/170001/content/renderer/media/...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/178153004/diff/170001/content/renderer/media/...
content/renderer/media/websourcebuffer_impl.cc:73: demuxer_->AppendData(id_,
data, length, &new_offset_if_updated);
Sorry to keep causing thrashing on this. Based on my comments on the Blink-side
changes, I think it makes sense to just pass the timestamp_offset into this
call. Since we wouldn't need to return a bool is should be pretty simple to just
have the real append code update the *timestamp_offset.

https://codereview.chromium.org/178153004/diff/170001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/178153004/diff/170001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:442: *new_timestamp_offset = timestamp_offset_;
If we pass timestamp_offset here then we can just make this code be
if (timestamp_offset_updated_by_append_ && timestamp_offset)
  *timestamp_offset = timestamp_offset_.InSecondsF();

and a bunch of the other code becomes unnecessary.

In the future, we may consider just setting timestamp_offset_ with
*timestamp_offset and remove the SetTimestampOffset() in favor of an
isParsingMediaSegment() method.

[wolenetz, 2014-02-27 21:48:48]

PTAL @ patch set 8.
I've also reworked the Blink-side change
(https://codereview.chromium.org/178763006/) to depend on this Chromium-side
change landing first, to reduce churn in Blink public/platform/* for this
append() interface change.

Thanks!

https://codereview.chromium.org/178153004/diff/170001/content/renderer/media/...
File content/renderer/media/websourcebuffer_impl.cc (right):

https://codereview.chromium.org/178153004/diff/170001/content/renderer/media/...
content/renderer/media/websourcebuffer_impl.cc:73: demuxer_->AppendData(id_,
data, length, &new_offset_if_updated);
On 2014/02/27 18:27:21, acolwell wrote:
> Sorry to keep causing thrashing on this. Based on my comments on the
Blink-side
> changes, I think it makes sense to just pass the timestamp_offset into this
> call. Since we wouldn't need to return a bool is should be pretty simple to
just
> have the real append code update the *timestamp_offset.

Done.

https://codereview.chromium.org/178153004/diff/170001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/178153004/diff/170001/media/filters/chunk_dem...
media/filters/chunk_demuxer.cc:442: *new_timestamp_offset = timestamp_offset_;
On 2014/02/27 18:27:21, acolwell wrote:
> If we pass timestamp_offset here then we can just make this code be
> if (timestamp_offset_updated_by_append_ && timestamp_offset)
>   *timestamp_offset = timestamp_offset_.InSecondsF();
> 
> and a bunch of the other code becomes unnecessary.
> 
> In the future, we may consider just setting timestamp_offset_ with
> *timestamp_offset and remove the SetTimestampOffset() in favor of an
> isParsingMediaSegment() method.

Done.

[acolwell GONE FROM CHROMIUM, 2014-02-27 22:22:22]

lgtm

[wolenetz, 2014-02-27 22:32:01]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-02-27 22:33:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/178153004/210001

[commit-bot: I haz the power, 2014-02-28 01:06:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/178153004/210001

[commit-bot: I haz the power, 2014-02-28 01:54:10]

Change committed as 254010