Enable round-tripping and updating of SourceBuffer timestamp offset

Enable round-tripping and updating of SourceBuffer timestamp offset

This is the second 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,jamesr@chromium.org
BUG=249422
TEST=No local linux regressions of http/tests/media/media-source/ layout tests

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168163

[wolenetz, 2014-02-25 03:57:26]

Please take a look:
jamesr: public/ OWNERS review,
acolwell: everything.

Note, the prerequisite Chromium-side CL is also in CR @
https://codereview.chromium.org/178153004/

Thank you!

[acolwell GONE FROM CHROMIUM, 2014-02-25 17:50:56]

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:543: m_timestampOffset =
m_webSourceBuffer->getTimestampOffset();
See my comments in the Chromium-side changes. I think this should be a
conditional update and getTimestampOffset() should be something along the lines
of getTimestampOffsetUpdate(). m_timestampOffset should only be updated if
getTimestampOffsetUpdate() returns something other than NaN.

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:702: m_timestampOffset =
m_webSourceBuffer->getTimestampOffset();
ditto. Perhaps put the update logic in a updateTimestampOffsetIfNecessary()
helper.

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/W...
File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/W...
Source/modules/mediasource/WebKitSourceBuffer.cpp:147: m_timestampOffset =
m_webSourceBuffer->getTimestampOffset();
Remove this. As your comment states, the timestampOffset should not be updated
for the prefixed API since it is never in 'sequence' mode.

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

Thank you for PS1 comments, acolwell@.

Same reviewers: please take a look at PS2. The prerequisite chromium CL is still
in CR at the moment.

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:543: m_timestampOffset =
m_webSourceBuffer->getTimestampOffset();
On 2014/02/25 17:50:57, acolwell wrote:
> See my comments in the Chromium-side changes. I think this should be a
> conditional update and getTimestampOffset() should be something along the
lines
> of getTimestampOffsetUpdate(). m_timestampOffset should only be updated if
> getTimestampOffsetUpdate() returns something other than NaN.

Done.

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
Source/modules/mediasource/SourceBuffer.cpp:702: m_timestampOffset =
m_webSourceBuffer->getTimestampOffset();
On 2014/02/25 17:50:57, acolwell wrote:
> ditto. Perhaps put the update logic in a updateTimestampOffsetIfNecessary()
> helper.

Done.

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/W...
File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/W...
Source/modules/mediasource/WebKitSourceBuffer.cpp:147: m_timestampOffset =
m_webSourceBuffer->getTimestampOffset();
On 2014/02/25 17:50:57, acolwell wrote:
> Remove this. As your comment states, the timestampOffset should not be updated
> for the prefixed API since it is never in 'sequence' mode.

Done.

[wolenetz, 2014-02-25 20:34:01]

On 2014/02/25 20:33:32, wolenetz wrote:
> Thank you for PS1 comments, acolwell@.
> 
> Same reviewers: please take a look at PS2. The prerequisite chromium CL is
still
> in CR at the moment.
> 
>
https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
> File Source/modules/mediasource/SourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
> Source/modules/mediasource/SourceBuffer.cpp:543: m_timestampOffset =
> m_webSourceBuffer->getTimestampOffset();
> On 2014/02/25 17:50:57, acolwell wrote:
> > See my comments in the Chromium-side changes. I think this should be a
> > conditional update and getTimestampOffset() should be something along the
> lines
> > of getTimestampOffsetUpdate(). m_timestampOffset should only be updated if
> > getTimestampOffsetUpdate() returns something other than NaN.
> 
> Done.
> 
>
https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/S...
> Source/modules/mediasource/SourceBuffer.cpp:702: m_timestampOffset =
> m_webSourceBuffer->getTimestampOffset();
> On 2014/02/25 17:50:57, acolwell wrote:
> > ditto. Perhaps put the update logic in a updateTimestampOffsetIfNecessary()
> > helper.
> 
> Done.
> 
>
https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/W...
> File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/178763006/diff/1/Source/modules/mediasource/W...
> Source/modules/mediasource/WebKitSourceBuffer.cpp:147: m_timestampOffset =
> m_webSourceBuffer->getTimestampOffset();
> On 2014/02/25 17:50:57, acolwell wrote:
> > Remove this. As your comment states, the timestampOffset should not be
updated
> > for the prefixed API since it is never in 'sequence' mode.
> 
> Done.

Also, PS2 includes a rebase.

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

lgtm % nit

https://codereview.chromium.org/178763006/diff/20001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/178763006/diff/20001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:58: virtual double updatedTimestampOffset() =
0;
nit: Add a default implementation that returns NaN so this can be landed
independent of the Chromium side change.

[wolenetz, 2014-02-25 22:34:35]

Thanks, acolwell@.

jamesr@: PTAL @ PS3 for public/ OWNERS review.

https://codereview.chromium.org/178763006/diff/20001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/178763006/diff/20001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:58: virtual double updatedTimestampOffset() =
0;
On 2014/02/25 21:53:53, acolwell wrote:
> nit: Add a default implementation that returns NaN so this can be landed
> independent of the Chromium side change.

Neat! Done.

[wolenetz, 2014-02-25 22:49:34]

I have a style question:

https://codereview.chromium.org/178763006/diff/40001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/178763006/diff/40001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:36: #include <limits>
There's a lint error for the sequencing of header includes. I copied this
sequence from another example in Blink. Should <limits> be before or after
"WebTimeRange.h" here?

[jamesr, 2014-02-25 22:52:01]

Using NaN as a signalling value is really weird.  I don't think we do this
anywhere else in the blink public API.  Is there another sentinel value that's
more semantically meaningful you could use? Why not return the last value set by
setTimeOffset() or something like that?

[acolwell GONE FROM CHROMIUM, 2014-02-25 23:24:31]

On 2014/02/25 22:52:01, jamesr wrote:
> Using NaN as a signalling value is really weird.  I don't think we do this
> anywhere else in the blink public API.  Is there another sentinel value that's
> more semantically meaningful you could use? Why not return the last value set
by
> setTimeOffset() or something like that?

This was intended to signal there was no valid updated value. This was an
attempt to avoid having to keep a copy of the value passed into setTimeOffset()
solely for the purpose of returning it back to this code. The double is
converted to a TimeDelta in the layer just below this code so we wouldn't
actually need the value except to satisfy reflecting it back to code that
already has the value.

How about this? We change the append() signature to the following
bool append(const unsigned char* data, unsigned length, double*
newTimestampOffset);

It returns true if newTimestampOffset with a new value that should be assigned
to m_timestampOffset. I think that actually is a clearer way to express what we
are trying to capture. The offset can only change in response to an append.

[jamesr, 2014-02-25 23:27:39]

> How about this? We change the append() signature to the following
> bool append(const unsigned char* data, unsigned length, double*
> newTimestampOffset);
> 
> It returns true if newTimestampOffset with a new value that should be assigned
> to m_timestampOffset. I think that actually is a clearer way to express what
we
> are trying to capture. The offset can only change in response to an append.

That sounds good to me.

[wolenetz, 2014-02-27 01:03:02]

Same reviewers, PTAL @ patch set 4.
Thanks!

[jamesr, 2014-02-27 01:18:27]

https://codereview.chromium.org/178763006/diff/80001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/178763006/diff/80001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:54: // the Chromium implementation of this
signature has landed. See http://crbug.com/249422.
can you just land the chromium implementation first to avoid this extra step?

it's not clear to me if the return value will remain bool after the transition

[acolwell GONE FROM CHROMIUM, 2014-02-27 01:24:45]

lgtm % nit

https://codereview.chromium.org/178763006/diff/80001/Source/modules/mediasour...
File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/80001/Source/modules/mediasour...
Source/modules/mediasource/WebKitSourceBuffer.cpp:140: // Only "sequence" mode
coded frame processing, not in prefixed API, may update timestamp offset.
nit: This comment is a little confusing. How about something line
"|dummyNewTimestampOffset| is ignored because the prefixed API does not allow
timestampOffset updates."?

[wolenetz, 2014-02-27 01:44:55]

Thanks acolwell@.
jamesr@ PTAL @ PS5.
Thanks!

https://codereview.chromium.org/178763006/diff/80001/Source/modules/mediasour...
File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/80001/Source/modules/mediasour...
Source/modules/mediasource/WebKitSourceBuffer.cpp:140: // Only "sequence" mode
coded frame processing, not in prefixed API, may update timestamp offset.
On 2014/02/27 01:24:45, acolwell wrote:
> nit: This comment is a little confusing. How about something line
> "|dummyNewTimestampOffset| is ignored because the prefixed API does not allow
> timestampOffset updates."?

Done.

https://codereview.chromium.org/178763006/diff/80001/public/platform/WebSourc...
File public/platform/WebSourceBuffer.h (right):

https://codereview.chromium.org/178763006/diff/80001/public/platform/WebSourc...
public/platform/WebSourceBuffer.h:54: // the Chromium implementation of this
signature has landed. See http://crbug.com/249422.
On 2014/02/27 01:18:27, jamesr wrote:
> can you just land the chromium implementation first to avoid this extra step?
> 
> it's not clear to me if the return value will remain bool after the transition

I was going on acolwell@'s separate suggestion to keep the chromium and blink
changes independent. The end result of landing these two will be the chromium
implementation will override the bool append() here, letting me then land
another two independent changes that get rid of the old void append in each of
Blink and Chromium.

Keeping these first two independently land-able improves chances of more rapidly
getting the later two changes landed, too.

[jamesr, 2014-02-27 01:49:55]

The bool seems unnecessary.  If the append() operation does not need to update
the timestamp then it can simply avoid touching the value in the pointed-to
double.  I'd get rid of it

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:488: double newTimestampOffset;
this value is completely unininitialized

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:490: m_timestampOffset =
newTimestampOffset;
so if it's used here (i.e. the append() function returns true without actually
writing a value), we'll get completely undefined bits in here. any reason not to
initialize the local to something obvious like 0.0 on line 488?

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/WebKitSourceBuffer.cpp:140: //
|dummyNewTimestampOffset| is ignored because the prefixed API does not allow
appends to update
if the parameter is only optionally needed by callers, then a cleaner way to
express it is to pass NULL in for callers that don't care and have the
implementation only update the value if it is non-NULL

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

On 2014/02/27 01:49:55, jamesr wrote:
> The bool seems unnecessary.  If the append() operation does not need to update
> the timestamp then it can simply avoid touching the value in the pointed-to
> double.  I'd get rid of it
> 
>
https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
> File Source/modules/mediasource/SourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
> Source/modules/mediasource/SourceBuffer.cpp:488: double newTimestampOffset;
> this value is completely unininitialized
> 
>
https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
> Source/modules/mediasource/SourceBuffer.cpp:490: m_timestampOffset =
> newTimestampOffset;
> so if it's used here (i.e. the append() function returns true without actually
> writing a value), we'll get completely undefined bits in here. any reason not
to
> initialize the local to something obvious like 0.0 on line 488?
> 
>
https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
> File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):
> 
>
https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
> Source/modules/mediasource/WebKitSourceBuffer.cpp:140: //
> |dummyNewTimestampOffset| is ignored because the prefixed API does not allow
> appends to update
> if the parameter is only optionally needed by callers, then a cleaner way to
> express it is to pass NULL in for callers that don't care and have the
> implementation only update the value if it is non-NULL

jamesr: Thanks. For some reason I was just kept focusing on bringing the new
value to this code instead of bringing the memory location to the code that may
need to update it.

[wolenetz, 2014-02-27 03:29:58]

Thank you, jamesr@. Please take a look at patch set 6.

I've retained the bool return value following the logic in comment #9
(https://codereview.chromium.org/178763006/#msg9):
Essentially, |*newTimestampOffset| is meant to only be an optional and
conditional output of append; it's input value is meaningless. The offset can
only change in response to an append (or explicit setting). We also don't want
to introduce confusion: the input value is meaningless and explicitly is not
used as the offset-before-doing-the-append (because doing so would require
additional double->TimeDelta conversions.)

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
File Source/modules/mediasource/SourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:488: double newTimestampOffset;
On 2014/02/27 01:49:56, jamesr wrote:
> this value is completely unininitialized

Done.

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/SourceBuffer.cpp:490: m_timestampOffset =
newTimestampOffset;
On 2014/02/27 01:49:56, jamesr wrote:
> so if it's used here (i.e. the append() function returns true without actually
> writing a value), we'll get completely undefined bits in here. any reason not
to
> initialize the local to something obvious like 0.0 on line 488?

Done.

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
File Source/modules/mediasource/WebKitSourceBuffer.cpp (right):

https://codereview.chromium.org/178763006/diff/100001/Source/modules/mediasou...
Source/modules/mediasource/WebKitSourceBuffer.cpp:140: //
|dummyNewTimestampOffset| is ignored because the prefixed API does not allow
appends to update
On 2014/02/27 01:49:56, jamesr wrote:
> if the parameter is only optionally needed by callers, then a cleaner way to
> express it is to pass NULL in for callers that don't care and have the
> implementation only update the value if it is non-NULL

Done.

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

On 2014/02/27 03:29:58, wolenetz wrote:
> Thank you, jamesr@. Please take a look at patch set 6.
> 
> I've retained the bool return value following the logic in comment #9
> (https://codereview.chromium.org/178763006/#msg9%29:
> Essentially, |*newTimestampOffset| is meant to only be an optional and
> conditional output of append; it's input value is meaningless. The offset can
> only change in response to an append (or explicit setting). We also don't want
> to introduce confusion: the input value is meaningless and explicitly is not
> used as the offset-before-doing-the-append (because doing so would require
> additional double->TimeDelta conversions.)

wolenetz: I've come around to jamesr's suggestion. I think just passing
&m_timestampOffset down is good. It also clearly indicates at the call site that
m_timestampOffset may have been changed by the append() call. I think doing this
may also allow us to simplify some code on the Chromium side.

[wolenetz, 2014-02-27 21:52:22]

Same reviewers, please take a look at patch set 7.
I've reworked this per comment #18 (and all of jamesr@'s comments). It now
requires the Chromium side CL (https://codereview.chromium.org/178153004/) to
land first, to reduce the churn to public/platform/*.

Thanks!

[jamesr, 2014-02-27 23:46:46]

lgtm

[wolenetz, 2014-02-28 00:57:52]

On 2014/02/27 23:46:46, jamesr wrote:
> lgtm

Thank you.
acolwell@: Do you want to re-review this before I send this to CQ (once the
chromium first part lands)?

[wolenetz, 2014-02-28 18:59:00]

On 2014/02/28 00:57:52, wolenetz wrote:
> On 2014/02/27 23:46:46, jamesr wrote:
> > lgtm
> 
> Thank you.
> acolwell@: Do you want to re-review this before I send this to CQ (once the
> chromium first part lands)?

I synced it's good-to-go with acolwell@ offline. I'll send this to CQ shortly.

[wolenetz, 2014-02-28 18:59:16]

The CQ bit was checked by wolenetz@chromium.org

[commit-bot: I haz the power, 2014-02-28 18:59:59]

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

[commit-bot: I haz the power, 2014-02-28 20:18:27]

Change committed as 168163