Pass a TimeTicks along video capture pipeline to represent capture time

Pass a TimeTicks along video capture pipeline to represent capture time

The goal of this change is to give more accurate information about when
a video frame is generated. VideoFrame already provides a TimeDelta
value as the presentation timestamp. This is unchanged but we provide
an additional TimeTicks value to indicate the estimated capture time of
the video frame.

It accurately records the time when the videoframe is generated for local
sources such as camera and tab capture device. This value will be an
estimate for remote sources such as WebRTC.

After this change cast streaming API is able to collect accurate
information about how much time is spent in capturing.

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

[Alpha Left Google, 2014-05-18 00:01:22]

I intentionally didn't put in anything related to date because I think it's a
separate problem.

[Ami GONE FROM CHROMIUM, 2014-05-19 19:01:46]

Bunch of nits below, but my main question is why should start_ticks be fed with
every single frame instead of being an at-most-once callback near the beginning
of the stream?!?

https://codereview.chromium.org/287313002/diff/1/chrome/renderer/media/cast_r...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/287313002/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_rtp_stream.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Are the perf #'s in the CL description mean & std-dev?  
Should note that if they are.

I'm confused/distressed by std-dev being so high relative to the mean.  Are
mean+stddev meaningful metrics for the distributions involved?

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
File content/common/media/video_capture.h (right):

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:38: // |format| - Format and additional
information such as frame rate.
neither of these 2 lines are adding value.

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:40: // information for the beginning of
this stream of video frames.
These 2 lines would be clearer as:
The time at the start of this stream, if available.

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:42: // this stream is generated. It is
possible for this value to be null if
s/stream is/stream was/

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:43: // this timing information cannot be
determined. There is no gurantee that
typo: gurantee

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:45: // can only provide this timing
information as an estimation.
estimation -> estimate

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:47: // |video_frame->timestamp() +
start_ticks| will give an estimation in local
estimation -> estimate

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:48: // time when the frame is generated.
s/is/was/

https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_video_sink.h:29: const base::TimeTicks&
start_ticks)>
doco this is the stream start time.

https://codereview.chromium.org/287313002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/287313002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:123: // We
cannot estimate the local time when this stream starts so give a
Is there a crbug for this?
Should be, and should be a TODO.

https://codereview.chromium.org/287313002/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc/video_destination_handler.cc (right):

https://codereview.chromium.org/287313002/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc/video_destination_handler.cc:70: // The time when
this stream starts is unknown so give a null value to
Ditto

[Alpha Left Google, 2014-05-19 20:40:12]

> Bunch of nits below, but my main question is why should start_ticks be fed
with
> every single frame instead of being an at-most-once callback near the
beginning
> of the stream?!?

Let's discuss this first before anything else.

I made this choice so as to keep the API more consistent and more convenient for
the sinks to implement.

The signal for |start_ticks| is best delivered on the IO thread because it is
used where VideoFrame is consumed. 
However the APIs between MediaStreamVideoCapturerSource, MediaStreamVideoTrack
and MediaStreamVideoSink are all on the render thread. The only exception is
frame delivery through a callback on the IO thread. I do not want to create
another exception for |start_ticks|.

Having a separate callback on the IO thread for |start_ticks| requires the
consumer to have an extra IO thread method. And need to find ways to combine it
with VideoFrames.

I think it's a lot more complicated overall to have a separate |start_ticks|
signal.

[miu, 2014-05-19 21:59:37]

https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_video_sink.h:29: const base::TimeTicks&
start_ticks)>
Throughout this patch: Why must we pass this value with every frame deliver
callback?  It's redundant and wasteful.  Instead, we should have a separate
callback that is only invoked once, just before the first frame, to provide the
start_time.

This would also solve a second problem throughout this patch: There are a lot of
places where you just shove a "null" TimeTicks in when invoking the delivery
callback.  With my suggestion, those implementations that do not support
providing a start_time, would simply never call the
"ProvideStartTimeTicksCallback."

I know this is extra plumbing work, but we should strive to maintain clean,
simple interfaces (which helps us write clean, simple implementations).  We've
been asking this of the WebRTC folks as they clean-up the code in
chrome/renderer/media, so we should be good citizens and do the same.

[Ami GONE FROM CHROMIUM, 2014-05-19 22:14:11]

On 2014/05/19 20:40:12, Alpha wrote:
> The signal for |start_ticks| is best delivered on the IO thread because it is
> used where VideoFrame is consumed.

Is start_ticks only used by cast_rtp_stream.cc or are there other
non-pass-through readers of the value?

[Ami GONE FROM CHROMIUM, 2014-05-19 22:14:16]

https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_video_sink.h:30: VideoSinkDeliverFrameCB;
Is it easy to see why VideoSinkDeliverFrameCB and VideoCaptureDeliverFrameCB
should not be the same typedef?

[Alpha Left Google, 2014-05-19 22:25:51]

On 2014/05/19 21:59:37, miu wrote:
>
https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
> File content/public/renderer/media_stream_video_sink.h (right):
> 
>
https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
> content/public/renderer/media_stream_video_sink.h:29: const base::TimeTicks&
> start_ticks)>
> Throughout this patch: Why must we pass this value with every frame deliver
> callback?  It's redundant and wasteful.  Instead, we should have a separate
> callback that is only invoked once, just before the first frame, to provide
the
> start_time.
> 
> This would also solve a second problem throughout this patch: There are a lot
of
> places where you just shove a "null" TimeTicks in when invoking the delivery
> callback.  With my suggestion, those implementations that do not support
> providing a start_time, would simply never call the
> "ProvideStartTimeTicksCallback."
> 
> I know this is extra plumbing work, but we should strive to maintain clean,
> simple interfaces (which helps us write clean, simple implementations).  We've
> been asking this of the WebRTC folks as they clean-up the code in
> chrome/renderer/media, so we should be good citizens and do the same.

Hey Yuri I just answered the question you raised. I'll reiterate here.

My opinion is that having a separate callback doesn't make the code cleaner. For
the following reasons:
1. There's no guarantee that this start_ticks value is available before a frame
is delivered. Especially for remote video streams. 
2. This separate callback needs special handling to be called on the IO thread.
Such that it can be used together with a VideoFrame. Having a separate callback
makes it more complicated for consumer to use this value.

My intention of this change wasn't to do this quickly to get the job done. In
fact I tried implementing different approaches (the one you mentioned as well)
and I concluded that this approach is more consistent with existing APIs and
simplest for consumers (sinks) to implement. It also handle the case where
start_ticks is available after a few video frames are generated, as in the case
of remote video stream.

[Ami GONE FROM CHROMIUM, 2014-05-19 23:03:47]

On Mon, May 19, 2014 at 3:25 PM, <hclam@chromium.org> wrote:
> simplest for consumers (sinks) to implement.

Can you say more about why this is?  
AFAICT all the existing receivers simply pass the value along (which they'd do
in their impl of the new callback, too), except for cast_rtp_stream.cc which
would set a class member and then read it in the OnVideoFrame member.
 
> It also handle the case where
> start_ticks is available after a few video frames are generated, as in the
case
> of remote video stream.

How does the dedicated-callback version not do the same?

[miu, 2014-05-19 23:57:58]

On 2014/05/19 23:03:47, Ami Fischman wrote:
> On Mon, May 19, 2014 at 3:25 PM, <mailto:hclam@chromium.org> wrote:
> > simplest for consumers (sinks) to implement.
> 
> Can you say more about why this is?  
> AFAICT all the existing receivers simply pass the value along (which they'd do
> in their impl of the new callback, too), except for cast_rtp_stream.cc which
> would set a class member and then read it in the OnVideoFrame member.
>  
> > It also handle the case where
> > start_ticks is available after a few video frames are generated, as in the
> case
> > of remote video stream.
> 
> How does the dedicated-callback version not do the same?

Ami: I chatted with Alpha about this, and it turns out he made a trade-off
decision (i.e., between redundant arg-passing and the approach we favor).  He
tried our approach, but felt it resulted in too much additional complexity.  I'm
on-the-fence, but leaning toward thinking he made the right decision here.

Alpha: Perhaps you could explain to us why MediaStreamVideoSink should be
different from MediaStreamAudioSink?

[Alpha Left Google, 2014-05-20 00:49:31]

On 2014/05/19 23:03:47, Ami Fischman wrote:
> On Mon, May 19, 2014 at 3:25 PM, <mailto:hclam@chromium.org> wrote:
> > simplest for consumers (sinks) to implement.
> 
> Can you say more about why this is?  
> AFAICT all the existing receivers simply pass the value along (which they'd do
> in their impl of the new callback, too), except for cast_rtp_stream.cc which
> would set a class member and then read it in the OnVideoFrame member.

The current design of MediaStreamVideoSink is to register (AddToVideoTrack) on
the render thread and then receive frames on the IO thread. There is only one
method
that is ever called on the IO thread. Other signals that are unrelated to video
frames
are delivered on the render thread.

One nice feature of this design is no additional object is required to handle
calls on the IO thread. See CastVideoSink::OnVideoFrame
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/me...

The method is static and only needs to connect the bound parameters with the
frame.
Being a static method it eliminates a lot of object life cycle issues. This is
possible because there is only one callback.

> > It also handle the case where
> > start_ticks is available after a few video frames are generated, as in the
> case
> > of remote video stream.
> 
> How does the dedicated-callback version not do the same?

Having a dedicated callback will do the same.

[Ami GONE FROM CHROMIUM, 2014-05-21 20:43:48]

On Mon, May 19, 2014 at 5:49 PM, <hclam@chromium.org> wrote:

> On 2014/05/19 23:03:47, Ami Fischman wrote:
>
>> On Mon, May 19, 2014 at 3:25 PM, <mailto:hclam@chromium.org> wrote:
>> > simplest for consumers (sinks) to implement.
>>
>
>  Can you say more about why this is?
>> AFAICT all the existing receivers simply pass the value along (which
>> they'd do
>> in their impl of the new callback, too), except for cast_rtp_stream.cc
>> which
>> would set a class member and then read it in the OnVideoFrame member.
>>
>
> The current design of MediaStreamVideoSink is to register
> (AddToVideoTrack) on
> the render thread and then receive frames on the IO thread. There is only
> one
> method
> that is ever called on the IO thread. Other signals that are unrelated to
> video
> frames
> are delivered on the render thread.
>
> One nice feature of this design is no additional object is required to
> handle
> calls on the IO thread. See CastVideoSink::OnVideoFrame
> https://code.google.com/p/chromium/codesearch#chromium/
> src/chrome/renderer/media/cast_rtp_stream.cc&l=228
>
> The method is static and only needs to connect the bound parameters with
> the
> frame.
> Being a static method it eliminates a lot of object life cycle issues.
> This is
> possible because there is only one callback.


What is it about having two callbacks that would prevent this approach?


>
>  > It also handle the case where
>> > start_ticks is available after a few video frames are generated, as in
>> the
>> case
>> > of remote video stream.
>>
>
>  How does the dedicated-callback version not do the same?
>>
>
> Having a dedicated callback will do the same.
>
> https://codereview.chromium.org/287313002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alpha Left Google, 2014-05-21 20:55:21]

> What is it about having two callbacks that would prevent this approach?

There needs to be an object that can receive two callbacks, save the TimeTicks
to a member variable, read by another method. And then we have to worry about
object life cycle, etc. This is what I would like to avoid.

[Ami GONE FROM CHROMIUM, 2014-05-21 21:18:18]

On 2014/05/21 20:55:21, Alpha wrote:
> > What is it about having two callbacks that would prevent this approach?
> 
> There needs to be an object that can receive two callbacks, save the TimeTicks
> to a member variable, read by another method. And then we have to worry about
> object life cycle, etc. This is what I would like to avoid.

OIC.  Are your worries allayed by using base:Owned?  I.e. something like:

struct StartTimeHolder {
  TimeTicks start_time;
};

static void OnVideoStartTime(StartTimeHolder* holder, TimeTicks st) {
  DCHECK(holder->start_time.is_null());
  holder->start_time = st;
}

StartTimeHolder* start_time_holder = new StartTimeHolder();
VideoStartTimeCB video_start_time_cb =
    base::Bind(&OnVideoStartTime, base::Owned(start_time_holder));
VideoSinkDeliverFrameCB on_video_frame_cb =
    base::Bind(&CastVideoSink::OnVideoFrame,
               // Unretained is safe because this callback is
               // deleted before the corresponding VideoStartTimeCB.
               base::Unretained(start_time_holder), expected_coded_size_,
               error_callback_, frame_input);
AddToVideoTrack(this, video_start_time_cb, on_video_frame_cb, track);

Then OnVideoFrame has access to start_time_holder but the API for delivering
frames doesn't have to mention the start time with every single call.  Lifetime
is guaranteed as long as on_video_frame_cb is cleared before video_start_time_cb
in RemoveSink, and threading isn't an issue since everything happens on the IO
thread.

[Alpha Left Google, 2014-05-22 19:15:05]

> OIC.  Are your worries allayed by using base:Owned?  I.e. something like:
> 
> struct StartTimeHolder {
>   TimeTicks start_time;
> };
> 
> static void OnVideoStartTime(StartTimeHolder* holder, TimeTicks st) {
>   DCHECK(holder->start_time.is_null());
>   holder->start_time = st;
> }
> 
> StartTimeHolder* start_time_holder = new StartTimeHolder();
> VideoStartTimeCB video_start_time_cb =
>     base::Bind(&OnVideoStartTime, base::Owned(start_time_holder));
> VideoSinkDeliverFrameCB on_video_frame_cb =
>     base::Bind(&CastVideoSink::OnVideoFrame,
>                // Unretained is safe because this callback is
>                // deleted before the corresponding VideoStartTimeCB.
>                base::Unretained(start_time_holder), expected_coded_size_,
>                error_callback_, frame_input);
> AddToVideoTrack(this, video_start_time_cb, on_video_frame_cb, track);
> 
> Then OnVideoFrame has access to start_time_holder but the API for delivering
> frames doesn't have to mention the start time with every single call. 
Lifetime
> is guaranteed as long as on_video_frame_cb is cleared before
video_start_time_cb
> in RemoveSink, and threading isn't an issue since everything happens on the IO
> thread.

I think that might create a hard to discover bug.

RemoveSink must be called on the render thread. We cannot gurantee that
CastVideoSink holds the last reference to VideoStartTimeCB and
VideoSinkDeliverFrameCB. Hence there is no gurantee that VideoSinkDeliverFrameCB
is cleared before VideoStartTimeCB.

Assuming I implemented AddToVideoTrack such that it calls VideoStartTimeCB,
clears it and then call VideoSinkDeliverFrameCB subsequently. And then when
CastVideoSink is destroyed on the render thread, the last reference to
VideoStartTimeCB is released and RemoveSink is called. VideoSinkDeliverFrameCB
will continue to be called on the IO thread until the asynchronous action of
removing the sink is completed. And then OnVideoStartTime will access a deleted
start_time_holder.

The design is that AddToVideoTrack and RemoveSink are called on the render while
frame deliveries happen on the IO thread. I think having just one callback
really minimizes the dependency on external objects. It doesn't force the
consumer to define new objects and minimizes the risk of threading issues.

[Ami GONE FROM CHROMIUM, 2014-05-22 19:47:57]

On 2014/05/22 19:15:05, Alpha wrote:

> RemoveSink must be called on the render thread. We cannot gurantee that
> CastVideoSink holds the last reference to VideoStartTimeCB and
> VideoSinkDeliverFrameCB. Hence there is no gurantee that
VideoSinkDeliverFrameCB
> is cleared before VideoStartTimeCB.


Ref-counting: the cause of, and solution to, all of chromium's bugs!
You could give VSDFC a ref on VSTC if you couldn't guarantee their eventual
deref'ing order another way.

> The design is that AddToVideoTrack and RemoveSink are called on the render
while
> frame deliveries happen on the IO thread. I think having just one callback
> really minimizes the dependency on external objects. It doesn't force the
> consumer to define new objects and minimizes the risk of threading issues.

We keep coming back to this issue.  I'm having trouble accepting that somehow
there is a pattern here that is safe precisely because there's a single callback
and which becomes unsafe when there are two callbacks.
IIUC the only reason the single-callback-on-a-different-thread pattern is safe
in the first place is because it's holding a ref to a refcountedthreadsafe
object and delegates to it (LocalVideoFrameInput, if my code-skimming is
correct; can you confirm?).  

Would your worries vanish if instead of doing the addition
(start_ticks_+frame->timestamp) in OnVideoFrame you passed frame's timestamp
unmolested to LVFI and did the addition in LVFI (and delegated OnVideoStartTime
to LVFI, obvs)?

> https://codereview.chromium.org/287313002/

[Alpha Left Google, 2014-05-22 20:33:14]

On 2014/05/22 19:47:57, Ami Fischman wrote:
> On 2014/05/22 19:15:05, Alpha wrote:
> 
> > RemoveSink must be called on the render thread. We cannot gurantee that
> > CastVideoSink holds the last reference to VideoStartTimeCB and
> > VideoSinkDeliverFrameCB. Hence there is no gurantee that
> VideoSinkDeliverFrameCB
> > is cleared before VideoStartTimeCB.
> 
> 
> Ref-counting: the cause of, and solution to, all of chromium's bugs!
> You could give VSDFC a ref on VSTC if you couldn't guarantee their eventual
> deref'ing order another way.

Sure. But I dislike this pattern. If adding refcounting is the solution I'd
rather have a separate refcounted inner class that accepts both calls. The
ownership is at least more clear than one callback owning another.

> 
> > The design is that AddToVideoTrack and RemoveSink are called on the render
> while
> > frame deliveries happen on the IO thread. I think having just one callback
> > really minimizes the dependency on external objects. It doesn't force the
> > consumer to define new objects and minimizes the risk of threading issues.
> 
> We keep coming back to this issue.  I'm having trouble accepting that somehow
> there is a pattern here that is safe precisely because there's a single
callback
> and which becomes unsafe when there are two callbacks.
> IIUC the only reason the single-callback-on-a-different-thread pattern is safe
> in the first place is because it's holding a ref to a refcountedthreadsafe
> object and delegates to it (LocalVideoFrameInput, if my code-skimming is
> correct; can you confirm?).  

I think you're focusing on the usage of this particular case. My main point is
that the single callback pattern simplifies design of the consumer and used this
code only as an example. The fact that we discussed different approaches and
each had subtleties shows how hard it is to write multithreaded code. Yes I can
write thread-safe code that will handle the two callbacks. But will that be
simple? Probably not.

> Would your worries vanish if instead of doing the addition
> (start_ticks_+frame->timestamp) in OnVideoFrame you passed frame's timestamp
> unmolested to LVFI and did the addition in LVFI (and delegated
OnVideoStartTime
> to LVFI, obvs)?
> 
> > https://codereview.chromium.org/287313002/

It is true that the code is safe because of LocalVideoFrameInput being
refcounted thread safe. media/cast itself has well defined APIs and
cast_rtp_stream is a glue between two different APIs. I don't think changing the
API of media/cast to fit the API of MediaStreamVideoSink in content is a good
idea.

[Ami GONE FROM CHROMIUM, 2014-05-22 23:21:26]

Alpha & I spoke f2f; summary:
- we like that streams have a TimeTicks start and frames have a TimeDelta offset
within the stream
- the disconnect on the one-vs-multiple callbacks issue was rooted in a desire
to keep media/cast's API unchanged.

A way to reconcile this disconnect is to have media/cast/ consider stream start
ticks and frame offset delta as well (instead of trying to present frames with
capture ticks).  We should do this if we believe that this is the right
abstraction for all the use-cases (cast, stream-forwarding, and abs time). 
Alpha was going to update this CL to reflect that (he thought it would be
straightforward).

Ronghua/Per/Yuri: would you like to weigh in with assent or concerns?

I think if we have buy-in from libjingle that stream start time should be
separate from frame offset delta then Alpha's update will be the way to go.  If
we don't have buy-in from libjingle then I think it's fine to go with the
current approach until there is a proposal on the table for something better.

[Ronghua Wu (Left Chromium), 2014-05-22 23:32:15]

I think from e2e timestamp project points of view, current approach is
better, i.e. it allows us to adjust the start_tick so that we can keep the
timestamp monotone increasing when the clock shift. Plus in the case of the
remote video, it gives us a chance to adjust the estimation.


On Thu, May 22, 2014 at 4:21 PM, <fischman@chromium.org> wrote:

> Alpha & I spoke f2f; summary:
> - we like that streams have a TimeTicks start and frames have a TimeDelta
> offset
> within the stream
> - the disconnect on the one-vs-multiple callbacks issue was rooted in a
> desire
> to keep media/cast's API unchanged.
>
> A way to reconcile this disconnect is to have media/cast/ consider stream
> start
> ticks and frame offset delta as well (instead of trying to present frames
> with
> capture ticks).  We should do this if we believe that this is the right
> abstraction for all the use-cases (cast, stream-forwarding, and abs time).
> Alpha was going to update this CL to reflect that (he thought it would be
> straightforward).
>
> Ronghua/Per/Yuri: would you like to weigh in with assent or concerns?
>
> I think if we have buy-in from libjingle that stream start time should be
> separate from frame offset delta then Alpha's update will be the way to
> go.  If
> we don't have buy-in from libjingle then I think it's fine to go with the
> current approach until there is a proposal on the table for something
> better.
>
> https://codereview.chromium.org/287313002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-22 23:45:32]

Sorry, what do you mean "current approach"?


On Thu, May 22, 2014 at 4:31 PM, Ronghua Wu <ronghuawu@chromium.org> wrote:

> I think from e2e timestamp project points of view, current approach is
> better, i.e. it allows us to adjust the start_tick so that we can keep the
> timestamp monotone increasing when the clock shift. Plus in the case of the
> remote video, it gives us a chance to adjust the estimation.
>
>
> On Thu, May 22, 2014 at 4:21 PM, <fischman@chromium.org> wrote:
>
>> Alpha & I spoke f2f; summary:
>> - we like that streams have a TimeTicks start and frames have a TimeDelta
>> offset
>> within the stream
>> - the disconnect on the one-vs-multiple callbacks issue was rooted in a
>> desire
>> to keep media/cast's API unchanged.
>>
>> A way to reconcile this disconnect is to have media/cast/ consider stream
>> start
>> ticks and frame offset delta as well (instead of trying to present frames
>> with
>> capture ticks).  We should do this if we believe that this is the right
>> abstraction for all the use-cases (cast, stream-forwarding, and abs time).
>> Alpha was going to update this CL to reflect that (he thought it would be
>> straightforward).
>>
>> Ronghua/Per/Yuri: would you like to weigh in with assent or concerns?
>>
>> I think if we have buy-in from libjingle that stream start time should be
>> separate from frame offset delta then Alpha's update will be the way to
>> go.  If
>> we don't have buy-in from libjingle then I think it's fine to go with the
>> current approach until there is a proposal on the table for something
>> better.
>>
>> https://codereview.chromium.org/287313002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ronghua Wu (Left Chromium), 2014-05-22 23:53:45]

Sorry I meant passing start_tick for each frame.


On Thu, May 22, 2014 at 4:45 PM, Ami Fischman <fischman@chromium.org> wrote:

> Sorry, what do you mean "current approach"?
>
>
> On Thu, May 22, 2014 at 4:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote:
>
>> I think from e2e timestamp project points of view, current approach is
>> better, i.e. it allows us to adjust the start_tick so that we can keep the
>> timestamp monotone increasing when the clock shift. Plus in the case of the
>> remote video, it gives us a chance to adjust the estimation.
>>
>>
>> On Thu, May 22, 2014 at 4:21 PM, <fischman@chromium.org> wrote:
>>
>>> Alpha & I spoke f2f; summary:
>>> - we like that streams have a TimeTicks start and frames have a
>>> TimeDelta offset
>>> within the stream
>>> - the disconnect on the one-vs-multiple callbacks issue was rooted in a
>>> desire
>>> to keep media/cast's API unchanged.
>>>
>>> A way to reconcile this disconnect is to have media/cast/ consider
>>> stream start
>>> ticks and frame offset delta as well (instead of trying to present
>>> frames with
>>> capture ticks).  We should do this if we believe that this is the right
>>> abstraction for all the use-cases (cast, stream-forwarding, and abs
>>> time).
>>> Alpha was going to update this CL to reflect that (he thought it would be
>>> straightforward).
>>>
>>> Ronghua/Per/Yuri: would you like to weigh in with assent or concerns?
>>>
>>> I think if we have buy-in from libjingle that stream start time should be
>>> separate from frame offset delta then Alpha's update will be the way to
>>> go.  If
>>> we don't have buy-in from libjingle then I think it's fine to go with the
>>> current approach until there is a proposal on the table for something
>>> better.
>>>
>>> https://codereview.chromium.org/287313002/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-23 01:07:34]

Huh.  What do you envision should happen when the clock shifts or
estimation adjusts?  Would you say send a new/different start_time
TimeTicks with each frame?  (seems like that would be the opposite of
guaranteeing monotonicity!)

-a

On Thu, May 22, 2014 at 4:53 PM, Ronghua Wu <ronghuawu@chromium.org> wrote:

> Sorry I meant passing start_tick for each frame.
>
>
> On Thu, May 22, 2014 at 4:45 PM, Ami Fischman <fischman@chromium.org>wrote:
>
>> Sorry, what do you mean "current approach"?
>>
>>
>> On Thu, May 22, 2014 at 4:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote:
>>
>>> I think from e2e timestamp project points of view, current approach is
>>> better, i.e. it allows us to adjust the start_tick so that we can keep the
>>> timestamp monotone increasing when the clock shift. Plus in the case of the
>>> remote video, it gives us a chance to adjust the estimation.
>>>
>>>
>>> On Thu, May 22, 2014 at 4:21 PM, <fischman@chromium.org> wrote:
>>>
>>>> Alpha & I spoke f2f; summary:
>>>> - we like that streams have a TimeTicks start and frames have a
>>>> TimeDelta offset
>>>> within the stream
>>>> - the disconnect on the one-vs-multiple callbacks issue was rooted in a
>>>> desire
>>>> to keep media/cast's API unchanged.
>>>>
>>>> A way to reconcile this disconnect is to have media/cast/ consider
>>>> stream start
>>>> ticks and frame offset delta as well (instead of trying to present
>>>> frames with
>>>> capture ticks).  We should do this if we believe that this is the right
>>>> abstraction for all the use-cases (cast, stream-forwarding, and abs
>>>> time).
>>>> Alpha was going to update this CL to reflect that (he thought it would
>>>> be
>>>> straightforward).
>>>>
>>>> Ronghua/Per/Yuri: would you like to weigh in with assent or concerns?
>>>>
>>>> I think if we have buy-in from libjingle that stream start time should
>>>> be
>>>> separate from frame offset delta then Alpha's update will be the way to
>>>> go.  If
>>>> we don't have buy-in from libjingle then I think it's fine to go with
>>>> the
>>>> current approach until there is a proposal on the table for something
>>>> better.
>>>>
>>>> https://codereview.chromium.org/287313002/
>>>>
>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ronghua Wu (Left Chromium), 2014-05-23 04:08:15]

Yes, a different start_time will be send.
1) timestamp is monotonicity
2) start_time is not
3) so that (current_frame_timestamp - start_frame_timestamp) + start_time =
current_frame_capture_time


On Thu, May 22, 2014 at 6:07 PM, Ami Fischman <fischman@chromium.org> wrote:

> Huh.  What do you envision should happen when the clock shifts or
> estimation adjusts?  Would you say send a new/different start_time
> TimeTicks with each frame?  (seems like that would be the opposite of
> guaranteeing monotonicity!)
>
> -a
>
> On Thu, May 22, 2014 at 4:53 PM, Ronghua Wu <ronghuawu@chromium.org>wrote:
>
>> Sorry I meant passing start_tick for each frame.
>>
>>
>> On Thu, May 22, 2014 at 4:45 PM, Ami Fischman <fischman@chromium.org>wrote:
>>
>>> Sorry, what do you mean "current approach"?
>>>
>>>
>>> On Thu, May 22, 2014 at 4:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote:
>>>
>>>> I think from e2e timestamp project points of view, current approach is
>>>> better, i.e. it allows us to adjust the start_tick so that we can keep the
>>>> timestamp monotone increasing when the clock shift. Plus in the case of
the
>>>> remote video, it gives us a chance to adjust the estimation.
>>>>
>>>>
>>>> On Thu, May 22, 2014 at 4:21 PM, <fischman@chromium.org> wrote:
>>>>
>>>>> Alpha & I spoke f2f; summary:
>>>>> - we like that streams have a TimeTicks start and frames have a
>>>>> TimeDelta offset
>>>>> within the stream
>>>>> - the disconnect on the one-vs-multiple callbacks issue was rooted in
>>>>> a desire
>>>>> to keep media/cast's API unchanged.
>>>>>
>>>>> A way to reconcile this disconnect is to have media/cast/ consider
>>>>> stream start
>>>>> ticks and frame offset delta as well (instead of trying to present
>>>>> frames with
>>>>> capture ticks).  We should do this if we believe that this is the right
>>>>> abstraction for all the use-cases (cast, stream-forwarding, and abs
>>>>> time).
>>>>> Alpha was going to update this CL to reflect that (he thought it would
>>>>> be
>>>>> straightforward).
>>>>>
>>>>> Ronghua/Per/Yuri: would you like to weigh in with assent or concerns?
>>>>>
>>>>> I think if we have buy-in from libjingle that stream start time should
>>>>> be
>>>>> separate from frame offset delta then Alpha's update will be the way
>>>>> to go.  If
>>>>> we don't have buy-in from libjingle then I think it's fine to go with
>>>>> the
>>>>> current approach until there is a proposal on the table for something
>>>>> better.
>>>>>
>>>>> https://codereview.chromium.org/287313002/
>>>>>
>>>>
>>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-23 18:22:09]

Perhaps we're failing to communicate :)
Both the current CL's approach and the potential update keep stream start
time (TimeTicks) and frame timestamp (TimeDelta) separate (i.e. neither is
proposing to mash them into a single value and lose information).
The distinction is whether stream start time should be repeated on a
per-frame basis, or whether it should be reported via a separate callback.

Are you seeing value in tying reporting of stream start time to frame
delivery specifically?


On Thu, May 22, 2014 at 9:07 PM, Ronghua Wu <ronghuawu@chromium.org> wrote:

> Yes, a different start_time will be send.
> 1) timestamp is monotonicity
> 2) start_time is not
> 3) so that (current_frame_timestamp - start_frame_timestamp) + start_time
> = current_frame_capture_time
>
>
> On Thu, May 22, 2014 at 6:07 PM, Ami Fischman <fischman@chromium.org>wrote:
>
>> Huh.  What do you envision should happen when the clock shifts or
>> estimation adjusts?  Would you say send a new/different start_time
>> TimeTicks with each frame?  (seems like that would be the opposite of
>> guaranteeing monotonicity!)
>>
>> -a
>>
>> On Thu, May 22, 2014 at 4:53 PM, Ronghua Wu <ronghuawu@chromium.org>wrote:
>>
>>> Sorry I meant passing start_tick for each frame.
>>>
>>>
>>> On Thu, May 22, 2014 at 4:45 PM, Ami Fischman <fischman@chromium.org>wrote:
>>>
>>>> Sorry, what do you mean "current approach"?
>>>>
>>>>
>>>> On Thu, May 22, 2014 at 4:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote:
>>>>
>>>>> I think from e2e timestamp project points of view, current approach is
>>>>> better, i.e. it allows us to adjust the start_tick so that we can keep
the
>>>>> timestamp monotone increasing when the clock shift. Plus in the case of
the
>>>>> remote video, it gives us a chance to adjust the estimation.
>>>>>
>>>>>
>>>>> On Thu, May 22, 2014 at 4:21 PM, <fischman@chromium.org> wrote:
>>>>>
>>>>>> Alpha & I spoke f2f; summary:
>>>>>> - we like that streams have a TimeTicks start and frames have a
>>>>>> TimeDelta offset
>>>>>> within the stream
>>>>>> - the disconnect on the one-vs-multiple callbacks issue was rooted in
>>>>>> a desire
>>>>>> to keep media/cast's API unchanged.
>>>>>>
>>>>>> A way to reconcile this disconnect is to have media/cast/ consider
>>>>>> stream start
>>>>>> ticks and frame offset delta as well (instead of trying to present
>>>>>> frames with
>>>>>> capture ticks).  We should do this if we believe that this is the
>>>>>> right
>>>>>> abstraction for all the use-cases (cast, stream-forwarding, and abs
>>>>>> time).
>>>>>> Alpha was going to update this CL to reflect that (he thought it
>>>>>> would be
>>>>>> straightforward).
>>>>>>
>>>>>> Ronghua/Per/Yuri: would you like to weigh in with assent or concerns?
>>>>>>
>>>>>> I think if we have buy-in from libjingle that stream start time
>>>>>> should be
>>>>>> separate from frame offset delta then Alpha's update will be the way
>>>>>> to go.  If
>>>>>> we don't have buy-in from libjingle then I think it's fine to go with
>>>>>> the
>>>>>> current approach until there is a proposal on the table for something
>>>>>> better.
>>>>>>
>>>>>> https://codereview.chromium.org/287313002/
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alpha Left Google, 2014-05-27 19:01:01]

ronghua: Please comment on the issue of passing TimeTicks with frame delivery vs
callback for time information. Such that we can move forward with one way or the
other. Thanks.

[Ronghua Wu (Left Chromium), 2014-05-27 21:21:58]

I was chatting with Ami offline. My main point was the start time is per
frame basis at least in the case of the remote stream, where the start time
is estimated per frame. That's why I think reporting start time per frame
is better.

Ami's offline right now, so I don't know if we will reach to the same
conclusion. Will update the thread again after I chatted with him again.


On Tue, May 27, 2014 at 12:01 PM, <hclam@chromium.org> wrote:

> ronghua: Please comment on the issue of passing TimeTicks with frame
> delivery vs
> callback for time information. Such that we can move forward with one way
> or the
> other. Thanks.
>
>
> https://codereview.chromium.org/287313002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2014-05-27 22:14:31]

To re-state what Ronghua said (which was news to me): the estimating that
happens in the remote stream case is of the capture wall-time of each frame
(from which the frame's delta is subtracted to arrive at the per-frame estimate
of a stream start time).
That suggested to me that the right thing to report for each frame is the
estimated wall-clock time of the capture as well as the TimeDelta of the frame
within the stream.  He pointed out that getStartDate()
(http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element...)
makes it natural to track the stream start wall-clock instead of the frame
capture wall-clock.

It makes sense to me to track frame capture wall-time on a per-frame basis now.
It makes more sense to me to track this as a capture time than a start time
(capture time minus delta) but I don't feel strongly about this.

I guess this means the approach in this CL looks good to me modulo the previous
paragraph, which I'll leave up to Alpha/Ronghua to hash out.

[Alpha Left Google, 2014-05-28 01:14:14]

Thanks. Looks like we're getting a consensus. 

|start_ticks| was a compromise to avoid having duplicated information in each
frame delivery. I'm happy to change it to the estimated capture time with
comments to clarify. I'll update this code such that |start_ticks| becomes
|estimated_capture_time|. We'll have to document this very clearly why there's
two timing information passed with each frame.

[miu, 2014-05-28 02:11:50]

On 2014/05/28 01:14:14, Alpha wrote:
> Thanks. Looks like we're getting a consensus. 
> 
> |start_ticks| was a compromise to avoid having duplicated information in each
> frame delivery. I'm happy to change it to the estimated capture time with
> comments to clarify. I'll update this code such that |start_ticks| becomes
> |estimated_capture_time|. We'll have to document this very clearly why there's
> two timing information passed with each frame.

1. Given the use cases and semantics for MediaStreamVideoSink, IMHO, I *highly*
prefer making this capture_time instead of start_time (or
estimated_capture_time).  The code in cast_rtp_stream.cc would then not need to
calculate "start_time + delta" like in Patch Set 1.  And, IIUC, it seems this
would fit Ronghua's use cases well.

2. In the documentation, we should specify that the TimeDelta in the VideoFrame
represents the stream's presentation time (i.e., the time offset since the first
frame), whereas the "capture time" is being provided from a separate "reference"
wall clock.  These two timelines can drift with respect to one another, and the
purpose of the latter is to provide a reference timestamp to achieve A/V
synchronization downstream.  This is the *actual* scheme employed by both WebRTC
and media/cast: The TimeDelta in the VideoFrame would be the value in the RTP
time domain, versus the capture_time being in the NTP time domain.  A more
detailed explanation of all this can be found here:
http://www.ciscopress.com/articles/article.asp?p=705533&seqNum=6

[Ronghua Wu (Left Chromium), 2014-05-28 17:42:51]

Good summary Yuri.

Passing around the capture time instead of the start time SGTM.

One question though:
If the TimeDelta of the VideoFrame is defined as time offset since the
first frame, does it mean the first frame will always have a TimeDelta of
0?





On Tue, May 27, 2014 at 7:11 PM, <miu@chromium.org> wrote:

> On 2014/05/28 01:14:14, Alpha wrote:
>
>> Thanks. Looks like we're getting a consensus.
>>
>
>  |start_ticks| was a compromise to avoid having duplicated information in
>> each
>> frame delivery. I'm happy to change it to the estimated capture time with
>> comments to clarify. I'll update this code such that |start_ticks| becomes
>> |estimated_capture_time|. We'll have to document this very clearly why
>> there's
>> two timing information passed with each frame.
>>
>
> 1. Given the use cases and semantics for MediaStreamVideoSink, IMHO, I
> *highly*
> prefer making this capture_time instead of start_time (or
> estimated_capture_time).  The code in cast_rtp_stream.cc would then not
> need to
> calculate "start_time + delta" like in Patch Set 1.  And, IIUC, it seems
> this
> would fit Ronghua's use cases well.
>
> 2. In the documentation, we should specify that the TimeDelta in the
> VideoFrame
> represents the stream's presentation time (i.e., the time offset since the
> first
> frame), whereas the "capture time" is being provided from a separate
> "reference"
> wall clock.  These two timelines can drift with respect to one another,
> and the
> purpose of the latter is to provide a reference timestamp to achieve A/V
> synchronization downstream.  This is the *actual* scheme employed by both
> WebRTC
> and media/cast: The TimeDelta in the VideoFrame would be the value in the
> RTP
> time domain, versus the capture_time being in the NTP time domain.  A more
> detailed explanation of all this can be found here:
> http://www.ciscopress.com/articles/article.asp?p=705533&seqNum=6
>
>
> https://codereview.chromium.org/287313002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alpha Left Google, 2014-05-28 17:44:58]

On 2014/05/28 17:42:51, Ronghua Wu wrote:
> Good summary Yuri.
> 
> Passing around the capture time instead of the start time SGTM.
> 
> One question though:
> If the TimeDelta of the VideoFrame is defined as time offset since the
> first frame, does it mean the first frame will always have a TimeDelta of
> 0?
> 

That's correct. However a MediaStream can start before a sink uses it so the
first frame seen by a sink does not necessarily has timestamp() of 0.

[Ronghua Wu (Left Chromium), 2014-05-28 17:57:22]

On Wed, May 28, 2014 at 10:44 AM, <hclam@chromium.org> wrote:

> On 2014/05/28 17:42:51, Ronghua Wu wrote:
>
>> Good summary Yuri.
>>
>
>  Passing around the capture time instead of the start time SGTM.
>>
>
>  One question though:
>> If the TimeDelta of the VideoFrame is defined as time offset since the
>> first frame, does it mean the first frame will always have a TimeDelta of
>> 0?
>>
>
>
> That's correct. However a MediaStream can start before a sink uses it so
> the
> first frame seen by a sink does not necessarily has timestamp() of 0.
>
Make sense.

1) I wonder how will this impact how <video> tag's currentTime attribute is
computed.
Currently, the currentTime = current_frame_time_delta -
first_frame_time_delta. The "first frame" is the first frame that reaches
the <video> tag. So looks like we don't want to change this even with the
new definition of the TimeDelta of the VideoFrame? I.e. <video> tag's
currentTime should start from 0.

2) Also note that the remote video frame's TimeDelta is currently the
absolute value of the rtp timestamp, i.e. not the offset since the first
frame. So you will need to do some converting.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...


>
> https://codereview.chromium.org/287313002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alpha Left Google, 2014-05-29 01:51:10]

> 1) I wonder how will this impact how <video> tag's currentTime attribute is
> computed.
> Currently, the currentTime = current_frame_time_delta -
> first_frame_time_delta. The "first frame" is the first frame that reaches
> the <video> tag. So looks like we don't want to change this even with the
> new definition of the TimeDelta of the VideoFrame? I.e. <video> tag's
> currentTime should start from 0.

I don't think I know the answer to this question. Adding
|estimated_capture_time| will not affect TimeDelta of the video frame.

> 2) Also note that the remote video frame's TimeDelta is currently the
> absolute value of the rtp timestamp, i.e. not the offset since the first
> frame. So you will need to do some converting.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

I'm not planning to change it in this CL. This change is purely for adding
estimated_capture_time.

[Ronghua Wu (Left Chromium), 2014-05-29 22:29:50]

SGTM. I have no more questions for now. I guess you will update your patch
to replace |start_time| with |capture_time|?


On Wed, May 28, 2014 at 6:51 PM, <hclam@chromium.org> wrote:

> 1) I wonder how will this impact how <video> tag's currentTime attribute is
>> computed.
>> Currently, the currentTime = current_frame_time_delta -
>> first_frame_time_delta. The "first frame" is the first frame that reaches
>> the <video> tag. So looks like we don't want to change this even with the
>> new definition of the TimeDelta of the VideoFrame? I.e. <video> tag's
>> currentTime should start from 0.
>>
>
> I don't think I know the answer to this question. Adding
> |estimated_capture_time| will not affect TimeDelta of the video frame.
>
>
>  2) Also note that the remote video frame's TimeDelta is currently the
>> absolute value of the rtp timestamp, i.e. not the offset since the first
>> frame. So you will need to do some converting.
>>
>
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/renderer/media/webrtc/media_stream_remote_video_source.cc&l=74
>
> I'm not planning to change it in this CL. This change is purely for adding
> estimated_capture_time.
>
>
>
> https://codereview.chromium.org/287313002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alpha Left Google, 2014-05-29 22:30:43]

That's right. Working on it. Will have it tomorrow.

[Alpha Left Google, 2014-05-31 00:34:50]

Code and CL description updated based on our discussion.

https://codereview.chromium.org/287313002/diff/1/chrome/renderer/media/cast_r...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/287313002/diff/1/chrome/renderer/media/cast_r...
chrome/renderer/media/cast_rtp_stream.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2014/05/19 19:01:46, Ami Fischman wrote:
> Are the perf #'s in the CL description mean & std-dev?  
> Should note that if they are.
> 
> I'm confused/distressed by std-dev being so high relative to the mean.  Are
> mean+stddev meaningful metrics for the distributions involved?

I have removed the stats in the change log description. It's not important to
include there. It is a definite benefit to eliminate jitter in timestamps caused
by IPC.

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
File content/common/media/video_capture.h (right):

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:35: 
I have moved the definition of this callback to media_stream_video_sink.h to
avoid duplicated definition.

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:38: // |format| - Format and additional
information such as frame rate.
On 2014/05/19 19:01:46, Ami Fischman wrote:
> neither of these 2 lines are adding value.

Done.

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:42: // this stream is generated. It is
possible for this value to be null if
On 2014/05/19 19:01:46, Ami Fischman wrote:
> s/stream is/stream was/

Done.

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:43: // this timing information cannot be
determined. There is no gurantee that
On 2014/05/19 19:01:46, Ami Fischman wrote:
> typo: gurantee

Done.

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:45: // can only provide this timing
information as an estimation.
On 2014/05/19 19:01:46, Ami Fischman wrote:
> estimation -> estimate

Done.

https://codereview.chromium.org/287313002/diff/1/content/common/media/video_c...
content/common/media/video_capture.h:47: // |video_frame->timestamp() +
start_ticks| will give an estimation in local
On 2014/05/19 19:01:46, Ami Fischman wrote:
> estimation -> estimate

Done.

https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
File content/public/renderer/media_stream_video_sink.h (right):

https://codereview.chromium.org/287313002/diff/1/content/public/renderer/medi...
content/public/renderer/media_stream_video_sink.h:30: VideoSinkDeliverFrameCB;
On 2014/05/19 22:14:16, Ami Fischman wrote:
> Is it easy to see why VideoSinkDeliverFrameCB and VideoCaptureDeliverFrameCB
> should not be the same typedef?

I have moved the VideoCaptureDeliverFrameCB to this file to eliminate the
duplication.

[miu, 2014-05-31 02:37:25]

lgtm

https://codereview.chromium.org/287313002/diff/80001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_rtp_stream.cc (right):

https://codereview.chromium.org/287313002/diff/80001/chrome/renderer/media/ca...
chrome/renderer/media/cast_rtp_stream.cc:242: base::TimeTicks timestamp =
base::TimeTicks::Now();
nit: Avoid unnecessary probing of system clock with:

base::TimeTicks timestamp;
if (estimated_capture_time.is_null())
  timestamp = base::TimeTicks::Now();
else
  timestamp = estimated_capture_time;

[Ami GONE FROM CHROMIUM, 2014-06-02 17:55:49]

LGTM % miu's nit; thanks for driving consensus on this hairy issue.

A suggestion for the future of this code: since you have fallback code for null
timestamps (which you must have to deal with missing remote estimates) and since
there are many many layers that the capture timestamp goes through between e.g.
desktop capture and media/cast code seeing it, if I were you I'd want a test
that asserts that the timestamp is not dropped by any of those layers.  IDK
cast's testing story but ISTM you should have some sort of end-to-end test that
asserts that the values that come out of the desktop capturer are equal to the
values that cast_rtp_stream.cc sees.

[Ronghua Wu (Left Chromium), 2014-06-02 18:09:08]

LGTM

one more besides miu and ami's comments.

https://codereview.chromium.org/287313002/diff/80001/content/renderer/media/w...
File content/renderer/media/webrtc/media_stream_remote_video_source.cc (right):

https://codereview.chromium.org/287313002/diff/80001/content/renderer/media/w...
content/renderer/media/webrtc/media_stream_remote_video_source.cc:124: // null
value to |estimated_capture_time|.
The comment is not accurate. We do have estimated capture time available in
underlying code. We just build an interface to expose it to here, e.g. via the
cricket::VideoFrame or and another filed to RenderFrame.

[Alpha Left Google, 2014-06-02 22:07:39]

+dmichael for pepper
+jam for content/public

[Alpha Left Google, 2014-06-02 22:08:43]

I have added a unit test to see the TimeTicks passes through the system. However
we don't have a test that can mock video capturer in the browser side so the
test only do MediaStreamVideoSource to MediaStreamVideoSink.

[dmichael (off chromium), 2014-06-02 22:43:59]

lgtm

[jam, 2014-06-03 16:39:54]

lgtm

[Alpha Left Google, 2014-06-03 18:07:37]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-06-03 18:08:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/287313002/100001

[commit-bot: I haz the power, 2014-06-03 22:54:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-03 22:57:36]

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

[commit-bot: I haz the power, 2014-06-03 22:57:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Alpha Left Google, 2014-06-03 23:05:07]

The CQ bit was checked by hclam@chromium.org

[commit-bot: I haz the power, 2014-06-03 23:05:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hclam@chromium.org/287313002/120001

[commit-bot: I haz the power, 2014-06-04 02:46:20]

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

[commit-bot: I haz the power, 2014-06-04 09:07:23]

Change committed as 274736