Use TaskQueue in IncomingVideoStream

Use TaskQueue in IncomingVideoStream instead of the PlatformThread + event timer approach.

BUG=webrtc:7219, webrtc:7253
Review-Url: https://codereview.webrtc.org/2716473002
Cr-Commit-Position: refs/heads/master@{#16860}
Committed: https://chromium.googlesource.com/external/webrtc/+/e2d1d6429557af4560a97581a5e282b54c742173

[tommi, 2017-02-22 21:53:56]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 21:54:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-22 22:00:13]

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

[commit-bot: I haz the power, 2017-02-22 22:00:14]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13905)

[tommi, 2017-02-22 22:21:12]

cleanup

[tommi, 2017-02-22 22:26:37]

add comment

[tommi, 2017-02-22 22:40:57]

use lambda

[tommi, 2017-02-22 22:41:02]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 22:41:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-22 22:47:43]

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

[commit-bot: I haz the power, 2017-02-22 22:47:44]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/13909)

[tommi, 2017-02-22 23:55:24]

Simplify

[tommi, 2017-02-23 00:11:37]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 00:11:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[tommi, 2017-02-23 00:15:46]

Fix constants, move to cc

[tommi, 2017-02-23 08:49:11]

Rebase

[tommi, 2017-02-23 11:17:44]

rebase and squash

[tommi, 2017-02-23 11:32:02]

Description was changed from

==========
WIP

BUG=
==========

to

==========
Use TaskQueue in IncomingVideoStream instead of the PlatformThread + event timer
approach.

BUG=webrtc:7219
==========

[tommi, 2017-02-23 11:32:02]

tommi@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[tommi, 2017-02-23 11:32:14]

Patchset #1 (id:1) has been deleted

[tommi, 2017-02-23 11:32:23]

Patchset #1 (id:20001) has been deleted

[tommi, 2017-02-23 11:32:29]

Patchset #1 (id:40001) has been deleted

[tommi, 2017-02-23 11:32:37]

Patchset #1 (id:60001) has been deleted

[tommi, 2017-02-23 11:32:48]

Patchset #1 (id:80001) has been deleted

[tommi, 2017-02-23 11:32:56]

Patchset #1 (id:100001) has been deleted

[tommi, 2017-02-23 11:33:04]

Patchset #1 (id:120001) has been deleted

[tommi, 2017-02-23 11:33:25]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 11:33:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-23 12:20:49]

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

[commit-bot: I haz the power, 2017-02-23 12:20:50]

Dry run: This issue passed the CQ dry run.

[mflodman, 2017-02-23 15:23:25]

A few comments / questions, then LGTM.

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/incl...
File webrtc/common_video/include/incoming_video_stream.h (left):

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/incl...
webrtc/common_video/include/incoming_video_stream.h:44: rtc::CriticalSection
buffer_critsect_;
Very nice to get rid of both this critsect and the thread below!

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
File webrtc/common_video/incoming_video_stream.cc (right):

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:37: if
(stream_->render_buffers_.AddFrame(std::move(frame_)) == 1)
There is no check that this happens on the right thread, 'render_buffers_' have
no thread/queue checks, should we add something here? Dequeue is covered below.

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:72:
incoming_render_queue_.PostDelayedTask([this]() { Dequeue(); }, wait_time);
'wait_time' can in theory be '0, e.g. if 'OnFrame' above takes a long time, but
I assume that is ok. Right? There is no unittest for that case in TaskQueue.

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/vide...
File webrtc/common_video/video_render_frames.cc (right):

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.cc:65:
incoming_frames_.emplace_back(std::move(new_frame));
Looking through the code, I can't find a check that the render time of a new
frame always is higher than existing frames. Maybe we should add a check for
this?

I can' fix that in a follow up otherwise,

[mflodman, 2017-02-23 15:29:20]

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/vide...
File webrtc/common_video/video_render_frames.cc (right):

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.cc:65:
incoming_frames_.emplace_back(std::move(new_frame));
On 2017/02/23 15:23:25, mflodman wrote:
> Looking through the code, I can't find a check that the render time of a new
> frame always is higher than existing frames. Maybe we should add a check for
> this?
> 
> I can' fix that in a follow up otherwise, 

And that would be as a dcheck and not for release builds...

[tommi, 2017-02-23 18:59:56]

Address comments

[tommi, 2017-02-23 19:00:09]

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/incl...
File webrtc/common_video/include/incoming_video_stream.h (left):

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/incl...
webrtc/common_video/include/incoming_video_stream.h:44: rtc::CriticalSection
buffer_critsect_;
On 2017/02/23 15:23:25, mflodman wrote:
> Very nice to get rid of both this critsect and the thread below!

hopefully there will be some performance benefits

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
File webrtc/common_video/incoming_video_stream.cc (right):

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:37: if
(stream_->render_buffers_.AddFrame(std::move(frame_)) == 1)
On 2017/02/23 15:23:25, mflodman wrote:
> There is no check that this happens on the right thread, 'render_buffers_'
have
> no thread/queue checks, should we add something here? Dequeue is covered
below.

Check added.  Dequeue would have caught it, but one more check doesn't hurt.

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:72:
incoming_render_queue_.PostDelayedTask([this]() { Dequeue(); }, wait_time);
On 2017/02/23 15:23:25, mflodman wrote:
> 'wait_time' can in theory be '0, e.g. if 'OnFrame' above takes a long time,
but
> I assume that is ok. Right? There is no unittest for that case in TaskQueue.

That's ok.  Also added test for TaskQueue.

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/vide...
File webrtc/common_video/video_render_frames.cc (right):

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.cc:65:
incoming_frames_.emplace_back(std::move(new_frame));
On 2017/02/23 15:23:25, mflodman wrote:
> Looking through the code, I can't find a check that the render time of a new
> frame always is higher than existing frames. Maybe we should add a check for
> this?
> 
> I can' fix that in a follow up otherwise, 

Done.

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.cc:65:
incoming_frames_.emplace_back(std::move(new_frame));
On 2017/02/23 15:29:20, mflodman wrote:
> On 2017/02/23 15:23:25, mflodman wrote:
> > Looking through the code, I can't find a check that the render time of a new
> > frame always is higher than existing frames. Maybe we should add a check for
> > this?
> > 
> > I can' fix that in a follow up otherwise, 
> 
> And that would be as a dcheck and not for release builds...

Acknowledged.

[tommi, 2017-02-23 19:00:17]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 19:00:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-23 19:09:06]

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

[commit-bot: I haz the power, 2017-02-23 19:09:06]

Dry run: Try jobs failed on following builders:
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23149)

[mflodman, 2017-02-24 15:29:58]

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
File webrtc/common_video/incoming_video_stream.cc (right):

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:37: if
(stream_->render_buffers_.AddFrame(std::move(frame_)) == 1)
On 2017/02/23 19:00:08, tommi (webrtc) wrote:
> On 2017/02/23 15:23:25, mflodman wrote:
> > There is no check that this happens on the right thread, 'render_buffers_'
> have
> > no thread/queue checks, should we add something here? Dequeue is covered
> below.
> 
> Check added.  Dequeue would have caught it, but one more check doesn't hurt.

If render_buffers_ were empty yes, but not otherwise. But I agree that would
probably be enough anyway.

https://codereview.webrtc.org/2716473002/diff/140001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:72:
incoming_render_queue_.PostDelayedTask([this]() { Dequeue(); }, wait_time);
On 2017/02/23 19:00:08, tommi (webrtc) wrote:
> On 2017/02/23 15:23:25, mflodman wrote:
> > 'wait_time' can in theory be '0, e.g. if 'OnFrame' above takes a long time,
> but
> > I assume that is ok. Right? There is no unittest for that case in TaskQueue.
> 
> That's ok.  Also added test for TaskQueue.

Thanks

https://codereview.webrtc.org/2716473002/diff/160001/webrtc/common_video/vide...
File webrtc/common_video/video_render_frames.cc (right):

https://codereview.webrtc.org/2716473002/diff/160001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.cc:69: // correctly ordered.
I was thinking something easier to avoid looping over the list for each frame,
something like this but before line 65:

if (!incoming_frames_.empty())
  RTC_DCHECK_GE(new_frame.render_time_ms(),
incoming_frames_.back().render_time_ms());

[tommi, 2017-02-24 20:41:16]

Address comments

[tommi, 2017-02-24 20:41:53]

https://codereview.webrtc.org/2716473002/diff/160001/webrtc/common_video/vide...
File webrtc/common_video/video_render_frames.cc (right):

https://codereview.webrtc.org/2716473002/diff/160001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.cc:69: // correctly ordered.
On 2017/02/24 15:29:58, mflodman wrote:
> I was thinking something easier to avoid looping over the list for each frame,
> something like this but before line 65:
> 
> if (!incoming_frames_.empty())
>   RTC_DCHECK_GE(new_frame.render_time_ms(),
> incoming_frames_.back().render_time_ms());

ah, of course. I changed the code and ran a bunch of unit tests but didn't find
a single one where the list of frames contains more than one frame :-/  So, I
changed the check so that in DCHECK builds, we store the last time stamp and
DCHECK that new frames have a timestamp that's >= the last one.

[tommi, 2017-02-24 20:44:23]

Rebase

[tommi, 2017-02-24 20:45:17]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2017-02-24 20:45:17]

The patchset sent to the CQ was uploaded after l-g-t-m from mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2716473002/#ps200001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-02-24 20:45:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-24 20:56:52]

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

[commit-bot: I haz the power, 2017-02-24 20:56:53]

Try jobs failed on following builders:
  mac_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/22361)

[tommi, 2017-02-24 21:40:05]

Looks like this is indeed happening in a few tests and it looks kind of random
on top of that:

win_x64_dbg
P2PTestConductor.GetGcmRecv
P2PTestConductor.IceRestart
P2PTestConductor.LocalP2PTestWithSpecCompliantMaxBundleOffer

win_dbg
P2PTestConductor.LocalP2PTestRtpDataChannel
P2PTestConductor.IceRenominationEnabled

mac_asan
TestWithNewVideoJitterBuffer/EndToEndTest.SendsAndReceivesVP9/0

android_rel
org.appspot.apprtc.test.PeerConnectionClientTest#testCameraSwitch

linux_memcheck
TestWithNewVideoJitterBuffer/EndToEndTest.SendsAndReceivesVP8/0
TestWithNewVideoJitterBuffer/EndToEndTest.SendsAndReceivesVP9/1
TestWithNewVideoJitterBuffer/EndToEndTest.SendsAndReceivesVP9VideoRotation9

[tommi, 2017-02-24 21:43:05]

Ignore frames from the past

[tommi, 2017-02-24 21:43:17]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 21:43:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-24 22:47:41]

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

[commit-bot: I haz the power, 2017-02-24 22:47:42]

Dry run: This issue passed the CQ dry run.

[tommi, 2017-02-24 22:47:53]

On 2017/02/24 21:43:05, tommi (webrtc) wrote:
> Ignore frames from the past

Magnus - can you take another look at the time stamp check?  It does appear that
we're getting frames from the past in some tests.  There are two patches, one
that has a DCHECK and another that logs a warning and drops them.  The second
one is the latest patch and the trybots aren't complaining fwiw.

[mflodman, 2017-02-25 17:10:42]

On 2017/02/24 22:47:53, tommi (webrtc) wrote:
> On 2017/02/24 21:43:05, tommi (webrtc) wrote:
> > Ignore frames from the past
> 
> Magnus - can you take another look at the time stamp check?  It does appear
that
> we're getting frames from the past in some tests.  There are two patches, one
> that has a DCHECK and another that logs a warning and drops them.  The second
> one is the latest patch and the trybots aren't complaining fwiw.

Hmm, that is weird. I'll take a look at it but given the travel I'm not sure
exactly when that might be. I might ask for assistance if it won't happen soon.

[tommi, 2017-02-25 17:13:23]

On 2017/02/25 17:10:42, mflodman wrote:
> On 2017/02/24 22:47:53, tommi (webrtc) wrote:
> > On 2017/02/24 21:43:05, tommi (webrtc) wrote:
> > > Ignore frames from the past
> > 
> > Magnus - can you take another look at the time stamp check?  It does appear
> that
> > we're getting frames from the past in some tests.  There are two patches,
one
> > that has a DCHECK and another that logs a warning and drops them.  The
second
> > one is the latest patch and the trybots aren't complaining fwiw.
> 
> Hmm, that is weird. I'll take a look at it but given the travel I'm not sure
> exactly when that might be. I might ask for assistance if it won't happen
soon.

I'll file a bug for it with the above info and change the DCHECK to a log for
now (and not drop the frames).

[mflodman, 2017-02-25 17:16:42]

LGTM

[tommi, 2017-02-27 11:17:29]

Description was changed from

==========
Use TaskQueue in IncomingVideoStream instead of the PlatformThread + event timer
approach.

BUG=webrtc:7219
==========

to

==========
Use TaskQueue in IncomingVideoStream instead of the PlatformThread + event timer
approach.

BUG=webrtc:7219, 7253
==========

[tommi, 2017-02-27 11:17:41]

Description was changed from

==========
Use TaskQueue in IncomingVideoStream instead of the PlatformThread + event timer
approach.

BUG=webrtc:7219, 7253
==========

to

==========
Use TaskQueue in IncomingVideoStream instead of the PlatformThread + event timer
approach.

BUG=webrtc:7219, webrtc:7253
==========

[tommi, 2017-02-27 11:23:32]

remove return statement and add TODO

[tommi, 2017-02-27 11:26:50]

ugh, looks like a pebkac

[tommi, 2017-02-27 11:26:55]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 11:27:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-27 11:33:02]

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

[commit-bot: I haz the power, 2017-02-27 11:33:04]

Dry run: Try jobs failed on following builders:
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/23635)

[tommi, 2017-02-27 11:57:12]

test pebkac again

[tommi, 2017-02-27 11:57:19]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 11:57:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[tommi, 2017-02-27 12:02:29]

Remove DCHECK

[tommi, 2017-02-27 12:13:47]

Print render_time_ms instead of timestamp

[tommi, 2017-02-27 12:16:18]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2017-02-27 12:16:19]

The patchset sent to the CQ was uploaded after l-g-t-m from mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2716473002/#ps320001 (title:
"Print render_time_ms instead of timestamp")

[commit-bot: I haz the power, 2017-02-27 12:16:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-27 12:40:44]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1488197778471100,
"parent_rev": "21253fc2c8a06a4b144be9fa6823b063c2b2f1dc", "commit_rev":
"e2d1d6429557af4560a97581a5e282b54c742173"}

[commit-bot: I haz the power, 2017-02-27 12:40:47]

Description was changed from

==========
Use TaskQueue in IncomingVideoStream instead of the PlatformThread + event timer
approach.

BUG=webrtc:7219, webrtc:7253
==========

to

==========
Use TaskQueue in IncomingVideoStream instead of the PlatformThread + event timer
approach.

BUG=webrtc:7219, webrtc:7253

Review-Url: https://codereview.webrtc.org/2716473002
Cr-Commit-Position: refs/heads/master@{#16860}
Committed:
https://chromium.googlesource.com/external/webrtc/+/e2d1d6429557af4560a97581a...
==========

[commit-bot: I haz the power, 2017-02-27 12:40:48]

Committed patchset #10 (id:320001) as
https://chromium.googlesource.com/external/webrtc/+/e2d1d6429557af4560a97581a...

[tommi, 2017-02-27 13:10:29]

A revert of this CL (patchset #10 id:320001) has been created in
https://codereview.webrtc.org/2714393003/ by tommi@webrtc.org.

The reason for reverting is: Reverting while fixing build issue in Chromium..