Incorrect timestamps used by video analyzer with real camera

Incorrect timestamps used by video analyzer with real camera

VideoAnalyzer::IncomingCapturedFrame() takes a copy of a incoming frame
and stores it so it can be compared to the rendered output. It does
however take the copy before the timestamps have been correctly set if
a real camera is used (which typically has render time set but not ntp
time). This caused a lookup mismatch in RenderFrame() which results in
a crash.

BUG=webrtc:5348

[sprang_webrtc, 2015-12-18 16:13:15]

sprang@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2015-12-18 16:23:16]

https://codereview.webrtc.org/1534233002/diff/1/webrtc/video/video_capture_in...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1534233002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:98:
local_renderer_->RenderFrame(video_frame, 0);
Use this callback instead of wrapping input, but move this until after we set
the timestamp.

[sprang_webrtc, 2015-12-18 16:52:57]

https://codereview.webrtc.org/1534233002/diff/1/webrtc/video/video_capture_in...
File webrtc/video/video_capture_input.cc (right):

https://codereview.webrtc.org/1534233002/diff/1/webrtc/video/video_capture_in...
webrtc/video/video_capture_input.cc:98:
local_renderer_->RenderFrame(video_frame, 0);
On 2015/12/18 16:23:16, pbos-webrtc wrote:
> Use this callback instead of wrapping input, but move this until after we set
> the timestamp.

Done.

[pbos-webrtc, 2015-12-19 11:02:34]

lgtm

https://codereview.webrtc.org/1534233002/diff/20001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/1534233002/diff/20001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:170: void RenderFrame(const VideoFrame&
video_frame,
Put a comment on this function to document that this renderer is used both for
local rendering (on input) and rendering after frames have been decoded on the
remote side, otherwise the below block is confusing.

[pbos-webrtc, 2015-12-19 11:03:29]

On 2015/12/19 11:02:34, pbos-webrtc wrote:
> lgtm
> 
>
https://codereview.webrtc.org/1534233002/diff/20001/webrtc/video/video_qualit...
> File webrtc/video/video_quality_test.cc (right):
> 
>
https://codereview.webrtc.org/1534233002/diff/20001/webrtc/video/video_qualit...
> webrtc/video/video_quality_test.cc:170: void RenderFrame(const VideoFrame&
> video_frame,
> Put a comment on this function to document that this renderer is used both for
> local rendering (on input) and rendering after frames have been decoded on the
> remote side, otherwise the below block is confusing.

Actually let me take another look when you know why all perf bots are failing
(all of FullStackTest is broken). :)

[Dani Kirov, 2015-12-21 20:29:29]

On 2015/12/19 11:03:29, pbos-webrtc wrote:
> On 2015/12/19 11:02:34, pbos-webrtc wrote:
> > lgtm
> > 
> >
>
https://codereview.webrtc.org/1534233002/diff/20001/webrtc/video/video_qualit...
> > File webrtc/video/video_quality_test.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1534233002/diff/20001/webrtc/video/video_qualit...
> > webrtc/video/video_quality_test.cc:170: void RenderFrame(const VideoFrame&
> > video_frame,
> > Put a comment on this function to document that this renderer is used both
for
> > local rendering (on input) and rendering after frames have been decoded on
the
> > remote side, otherwise the below block is confusing.
> 
> Actually let me take another look when you know why all perf bots are failing
> (all of FullStackTest is broken). :)

I've applied the patch, but the code still crashes in exactly the same place. 
Line:188     while (frames_.front().timestamp() < send_timestamp) {

[sprang_webrtc, 2016-01-20 16:31:50]

Added an internal proxy class implementing VideoRenderer so that we don't need
to demultiplex sender and receiver frames based on timestamps. PTAL

[pbos-webrtc, 2016-01-20 20:11:43]

Dani can you check and see that this resolves your issues?

[sprang_webrtc, 2016-01-25 14:50:06]

Ping?

[sprang_webrtc, 2016-02-26 16:20:19]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-02-26 16:20:19]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1534233002/#ps40001 (title:
"Separate callback for send-side frames rather than ts demultiplexing")

[commit-bot: I haz the power, 2016-02-26 16:20:26]

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

[commit-bot: I haz the power, 2016-02-26 16:22:40]

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

[commit-bot: I haz the power, 2016-02-26 16:22:41]

Exceeded global retry quota

[sprang_webrtc, 2016-08-31 16:25:14]

sprang@webrtc.org changed reviewers:
+ stefan@webrtc.org
- pbos@webrtc.org

[sprang_webrtc, 2016-08-31 16:25:14]

-pbos +stefan
Resurrecting this, with updated interfaces. ptal

[nisse-webrtc, 2016-09-02 08:06:44]

On 2016/08/31 16:25:14, språng wrote:
> -pbos +stefan
> Resurrecting this, with updated interfaces. ptal

Just note that the intention (see cl https://codereview.webrtc.org/2282713002/)
is that render_time_ms will be a (scaled) alias if timestamp_us, which will
always use rtc::TimeMicros timescale. No matter if we're using supposedly more
accurate camera timestamps or not.

[stefan-webrtc, 2016-09-13 07:19:53]

https://codereview.webrtc.org/1534233002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/1534233002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:202: frames_.push_back(video_frame);
Why did we have to use a copy before but not now?

[sprang_webrtc, 2016-09-13 08:35:26]

https://codereview.webrtc.org/1534233002/diff/60001/webrtc/video/video_qualit...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/1534233002/diff/60001/webrtc/video/video_qualit...
webrtc/video/video_quality_test.cc:202: frames_.push_back(video_frame);
On 2016/09/13 07:19:53, stefan-webrtc (holmer) wrote:
> Why did we have to use a copy before but not now?

Because we now call this method after all the timestamps have all been properly
populated.
The old code relied upon ntp_time_ms being set, and that that was the timestamp
always used - which was the root cause of this bug.
Though I think this CL is now borked again, since things moved significantly
with the encoder task queue thingy :(

I'll ping you when this CL is relevant again.