Win Video Capture: Sending the timestamps provided by the driver to the capture pipeline

WebRTC render smoothness test shows on windows platform the timestamps on the video frames are quite irregular. The reason is that we read the system clock time, when the frame is available to chromium, but the system clock may not be high definition.

In this CL, we changed the behavior to reading timestamp from the capture driver.

BUG=529949
TEST=all unit test and browser test passing, apprtc working as before.

Committed: https://crrev.com/b22216f19a3864c98588f5008a112fb9fc715371
Cr-Commit-Position: refs/heads/master@{#348291}

[qiangchen, 2015-09-09 22:18:05]

Patchset #1 (id:1) has been deleted

[qiangchen, 2015-09-09 22:50:16]

qiangchen@chromium.org changed reviewers:
+ mcasas@chromium.org

[mcasas, 2015-09-09 23:33:27]

LGTM with:

- Run more bots more often. Let them work for you :)
- Add a more extended description f.i. "Win Video Capture: ..."
"Sending the timestamp provided by the OS to the 
capture pipeline", and update TEST= line or specify how
this should affect a tester.
- Be more descriptive in the bug, just "...issue" is not
enough, consider e.g. 
"Address Windows video capture timestamp(s) irregularities"

https://codereview.chromium.org/1324683004/diff/20001/media/capture/video/win...
File media/capture/video/win/sink_input_pin_win.cc (right):

https://codereview.chromium.org/1324683004/diff/20001/media/capture/video/win...
media/capture/video/win/sink_input_pin_win.cc:188: 
I deeply distrust all this code. Would you mind adding
a DCHECK to certify that |start_time| <= |end_time| 
and |start_time| is non negative?

https://codereview.chromium.org/1324683004/diff/20001/media/capture/video/win...
File media/capture/video/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/1324683004/diff/20001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:459: base::TimeTicks() +
Funny you need to add a dummy ctor to add a TimeDelta. Hmm.

[qiangchen, 2015-09-10 17:07:09]

Patchset #2 (id:40001) has been deleted

[qiangchen, 2015-09-10 18:03:01]

Thanks. Two things remain:
1. For TEST= line, do you mean to write a unittest? Currently, I did not find a
simple way to view the effect of this CL.

2. Many trybots failed at step compile. But the point where the compilation
failed is not the code I changed. Any idea?

https://codereview.chromium.org/1324683004/diff/20001/media/capture/video/win...
File media/capture/video/win/sink_input_pin_win.cc (right):

https://codereview.chromium.org/1324683004/diff/20001/media/capture/video/win...
media/capture/video/win/sink_input_pin_win.cc:188: 
On 2015/09/09 23:33:26, mcasas wrote:
> I deeply distrust all this code. Would you mind adding
> a DCHECK to certify that |start_time| <= |end_time| 
> and |start_time| is non negative?

Done.

Added a fallback to old code path if the driver does not provide a timestamp.

https://codereview.chromium.org/1324683004/diff/20001/media/capture/video/win...
File media/capture/video/win/video_capture_device_win.cc (right):

https://codereview.chromium.org/1324683004/diff/20001/media/capture/video/win...
media/capture/video/win/video_capture_device_win.cc:459: base::TimeTicks() +
On 2015/09/09 23:33:27, mcasas wrote:
> Funny you need to add a dummy ctor to add a TimeDelta. Hmm.

TimeTicks has only static function FromInternalValue which can return a non-zero
instance. 

In my previous CL, Dale Curtis thought it was unsafe to use that function, in
case that in the future the unit for internal value would change.

[qiangchen, 2015-09-10 18:58:06]

Patchset #2 (id:60001) has been deleted

[mcasas, 2015-09-10 20:03:20]

About TEST, I mean at least a human description of perceived
changes (or lack thereof) after your test, e.g. "all unit test and
browser test passing, apprtc working as before"

Of course if you can write a unittest, even better! :)

https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
File media/capture/video/win/sink_input_pin_win.cc (right):

https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
media/capture/video/win/sink_input_pin_win.cc:186: 
no blank line

https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
media/capture/video/win/sink_input_pin_win.cc:189: end_time = -1;
Hmmm, the assignment and the use of |start_time| are a bit far now 
and that dilutes your original intention.

I suggest you change the interface signature of FrameReceived() to
 
void FrameReceived(const uint8* , int , base::TimeTicks)

and do all the calculations here:

REFERENCE_TIME start_time, end_time;
base::TimeTicks capture_timestamp;
if (SUCCEEDED(sample->GetTime(&start_time, &end_time))) {
  DCHECK_LE(start_time, end_time);
  capture_timestamp += base::TimeDelta::FromMicroseconds(start_time / 10);
} else {
  capture_timestamp = base::TimeTicks::Now();
}
observer_->FrameReceived(buffer, length, capture_timestamp);

Like this is clearer what is sent as timestamp and why.
WDYT?

[qiangchen, 2015-09-10 21:37:45]

Added the TEST line. 

Let's make the unittest in another CL. As it would involving implementation of
IMediaSample etc. Thus I think the unittest is big enough for a separate CL.

https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
File media/capture/video/win/sink_input_pin_win.cc (right):

https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
media/capture/video/win/sink_input_pin_win.cc:186: 
On 2015/09/10 20:03:20, mcasas wrote:
> no blank line

Done.

https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
media/capture/video/win/sink_input_pin_win.cc:189: end_time = -1;
On 2015/09/10 20:03:20, mcasas wrote:
> Hmmm, the assignment and the use of |start_time| are a bit far now 
> and that dilutes your original intention.
> 
> I suggest you change the interface signature of FrameReceived() to
>  
> void FrameReceived(const uint8* , int , base::TimeTicks)
> 
> and do all the calculations here:
> 
> REFERENCE_TIME start_time, end_time;
> base::TimeTicks capture_timestamp;
> if (SUCCEEDED(sample->GetTime(&start_time, &end_time))) {
>   DCHECK_LE(start_time, end_time);
>   capture_timestamp += base::TimeDelta::FromMicroseconds(start_time / 10);
> } else {
>   capture_timestamp = base::TimeTicks::Now();
> }
> observer_->FrameReceived(buffer, length, capture_timestamp);
> 
> Like this is clearer what is sent as timestamp and why.
> WDYT?

Done.

[mcasas, 2015-09-10 23:56:38]

On 2015/09/10 21:37:45, qiangchenC wrote:
> Added the TEST line. 
> 
> Let's make the unittest in another CL. As it would involving implementation of
> IMediaSample etc. Thus I think the unittest is big enough for a separate CL.
> 
>
https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
> File media/capture/video/win/sink_input_pin_win.cc (right):
> 
>
https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
> media/capture/video/win/sink_input_pin_win.cc:186: 
> On 2015/09/10 20:03:20, mcasas wrote:
> > no blank line
> 
> Done.
> 
>
https://codereview.chromium.org/1324683004/diff/80001/media/capture/video/win...
> media/capture/video/win/sink_input_pin_win.cc:189: end_time = -1;
> On 2015/09/10 20:03:20, mcasas wrote:
> > Hmmm, the assignment and the use of |start_time| are a bit far now 
> > and that dilutes your original intention.
> > 
> > I suggest you change the interface signature of FrameReceived() to
> >  
> > void FrameReceived(const uint8* , int , base::TimeTicks)
> > 
> > and do all the calculations here:
> > 
> > REFERENCE_TIME start_time, end_time;
> > base::TimeTicks capture_timestamp;
> > if (SUCCEEDED(sample->GetTime(&start_time, &end_time))) {
> >   DCHECK_LE(start_time, end_time);
> >   capture_timestamp += base::TimeDelta::FromMicroseconds(start_time / 10);
> > } else {
> >   capture_timestamp = base::TimeTicks::Now();
> > }
> > observer_->FrameReceived(buffer, length, capture_timestamp);
> > 
> > Like this is clearer what is sent as timestamp and why.
> > WDYT?
> 
> Done.

still LGTM

[qiangchen, 2015-09-10 23:58:17]

The CQ bit was checked by qiangchen@chromium.org

[commit-bot: I haz the power, 2015-09-10 23:58:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1324683004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1324683004/100001

[commit-bot: I haz the power, 2015-09-11 00:04:19]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2015-09-11 00:05:15]

Patchset 3 (id:??) landed as
https://crrev.com/b22216f19a3864c98588f5008a112fb9fc715371
Cr-Commit-Position: refs/heads/master@{#348291}

[commit-bot: I haz the power, 2015-09-23 12:16:41]

Patchset 3 (id:??) landed as
https://crrev.com/b22216f19a3864c98588f5008a112fb9fc715371
Cr-Commit-Position: refs/heads/master@{#348291}