Split IncomingVideoStream into two implementations, with smoothing and without.

Split IncomingVideoStream into two implementations, with smoothing and without.

This CL fixes an issue with the non-smoothing implementation where frames were delivered on the decoder thread.  No-smoothing is now done in a separate class that uses a TaskQueue.  The implementation may drop frames if the renderer doesn't keep up and it doesn't block the decoder thread.

Further work done:

* I added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 5 locks.

* I removed the Start/Stop methods from the IncomingVideoStream implementations.  Now, when an instance is created, it should be considered to be "running" and when it is deleted, it's "not running".  This saves on resources and also reduces the amount of locking required and I could remove one critical section altogether.

* I changed the VideoStreamDecoder class to not depend on IncomingVideoStream but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface.  This means that any implementation of that interface can be used and the decoder can be made to  just use the 'renderer' from the config.  Once we do that, we can decouple the IncomingVideoStream implementations from the decoder and VideoReceiveStream implementations and leave it up to the application for how to do smoothing.  The app can choose to use the Incoming* classes or roll its own (which may be preferable since applications often have their own scheduling mechanisms).

* The non-smoothing IncomingVideoStream implementation currently allows only 1 outstanding pending frame.  If we exceed that, the current frame won't be delivered to the renderer and instead we deliver the next one (since when this happens, the renderer is falling behind).

* The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in VideoReceiveStream and not all of the lifetime of VideoReceiveStream.

* Fixed VideoStreamDecoder to unregister callbacks in the dtor that were registered in the ctor. (this was open to a use-after-free regression)

* Delay and callback pointers are now passed via the ctors to the IncomingVideoStream classes.  The thread primitives in the IncomingVideoStream classes are also constructed/destructed at the same time as the owning object, which allowed me to remove one more lock.

* Removed code in the VideoStreamDecoder that could overwrite the VideoReceiveStream render delay with a fixed value of 10ms on construction.  This wasn't a problem with the previous implementation (it would be now though) but seemed to me like the wrong place to be setting that value.

* Made the render delay value in VideoRenderFrames, const.

BUG=

Committed: https://crrev.com/1c7eef652b0aa22d8ebb0bfe2b547094a794be22
Cr-Commit-Position: refs/heads/master@{#13129}

[tommi, 2016-06-03 21:15:45]

Update build files

[tommi, 2016-06-03 21:19:08]

Fix GN

[tommi, 2016-06-03 21:26:50]

Try non-smoothing

[tommi, 2016-06-03 21:34:00]

Back to default

[tommi, 2016-06-04 17:02:16]

Add TODOs with info on locking and assume we always have a renderer

[tommi, 2016-06-05 09:47:53]

Allow maximum 1 frame to be pending at any given time

[tommi, 2016-06-05 10:49:38]

Remove base class, start/stop, RAII, remove 'running_', remove one critsec

[tommi, 2016-06-05 11:02:52]

Delete unused variable

[tommi, 2016-06-05 12:49:21]

Make lifetime of video_stream_decoder_ match Start/Stop

[tommi, 2016-06-05 14:16:18]

Change delay expectations

[tommi, 2016-06-05 14:48:43]

Fix VideoStreamDecoder to unregister callbacks in dtor, restore e2e
expectations, set separate render callback

[tommi, 2016-06-05 19:04:31]

Rename IncomingVideoStreamImpl back to IncomingVideoStream

[tommi, 2016-06-05 19:19:05]

Remove SetRenderDelay from VideoRenderFrames and pass it via the ctor instead
(member becomes const)

[tommi, 2016-06-06 09:28:35]

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

[commit-bot: I haz the power, 2016-06-06 09:28:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035173002/280001

[commit-bot: I haz the power, 2016-06-06 09:32:43]

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

[commit-bot: I haz the power, 2016-06-06 09:32:43]

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

[tommi, 2016-06-06 19:46:38]

Remove one more lock in IncomingVideoStream

[tommi, 2016-06-06 22:34:06]

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

[commit-bot: I haz the power, 2016-06-06 22:34:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035173002/290001

[commit-bot: I haz the power, 2016-06-06 22:35:26]

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

[commit-bot: I haz the power, 2016-06-06 22:35:28]

Dry run: Try jobs failed on following builders:
  android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  android_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...)
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/8091)
  ios64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/340)
  ios64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/341)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/8091)
  ios_api_framework on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10412)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10338)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/15633)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14281)
  linux_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/15325)
  linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/12773)
  mac_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/11947)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/3782)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/9999)

[tommi, 2016-06-06 22:35:39]

Remove one more lock in IncomingVideoStream

[tommi, 2016-06-06 22:36:13]

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

[commit-bot: I haz the power, 2016-06-06 22:36:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035173002/310001

[commit-bot: I haz the power, 2016-06-06 23:07:40]

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

[commit-bot: I haz the power, 2016-06-06 23:07:41]

Dry run: This issue passed the CQ dry run.

[tommi, 2016-06-08 10:08:05]

Description was changed from

==========
Split IncomingVideoStream into two implementations.
One with smoothing and another without.
Fixing an issue with the non-smoothing implementation in the former
implementation where frames were delivered on the decoder thread.

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

to

==========
Split IncomingVideoStream into two implementations, with smoothing and without.

This CL fixes an issue with the non-smoothing implementation where frames were
delivered on the decoder thread.  No-smoothing is now done in a separate class
that uses a TaskQueue.  The implementation may drop frames if the renderer
doesn't keep up and it doesn't block the decoder thread.

Further work done:

* I added TODOs and documentation for VideoReceiveStream::OnFrame, where we
today grab 5 locks.

* I removed the Start/Stop methods from the IncomingVideoStream implementations.
 Now, when an instance is created, it should be considered to be "running" and
when it is deleted, it's "not running".  This saves on resources and also
reduces the amount of locking required and I could remove one critical section
altogether.

* I changed the VideoStreamDecoder class to not depend on IncomingVideoStream
but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface.  This
means that any implementation of that interface can be used and the decoder can
be made to  just use the 'renderer' from the config.  Once we do that, we can
decouple the IncomingVideoStream implementations from the decoder and
VideoReceiveStream implementations and leave it up to the application for how to
do smoothing.  The app can choose to use the Incoming* classes or roll its own
(which may be preferable since applications often have their own scheduling
mechanisms).

* The non-smoothing IncomingVideoStream implementation currently allows only 1
outstanding pending frame.  If we exceed that, the current frame won't be
delivered to the renderer and instead we deliver the next one (since when this
happens, the renderer is falling behind).

* The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in
VideoReceiveStream and not all of the lifetime of VideoReceiveStream.

* Fixed VideoStreamDecoder to unregister callbacks in the dtor that were
registered in the ctor. (this was open to a use-after-free regression)

* Delay and callback pointers are now passed via the ctors to the
IncomingVideoStream classes.  The thread primitives in the IncomingVideoStream
classes are also constructed/destructed at the same time as the owning object,
which allowed me to remove one more lock.

* Removed code in the VideoStreamDecoder that could overwrite the
VideoReceiveStream render delay with a fixed value of 10ms on construction. 
This wasn't a problem with the previous implementation (it would be now though)
but seemed to me like the wrong place to be setting that value.

* Made the render delay value in VideoRenderFrames, const.

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

[tommi, 2016-06-08 10:08:05]

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

[tommi, 2016-06-10 14:26:45]

Rebase

[tommi, 2016-06-10 15:35:20]

Add a couple of tests for the non-smoothing incoming stream implementation

[tommi, 2016-06-10 15:37:31]

I've added a couple of tests for the non-smoothing implementation, which is the
one that's used by Chrome, but so far hasn't had any tests running against it.

I think the default implementation should have some tests against it as well, in
particular it would be nice to have some quality tests for how smooth the timers
are, but I figured that this CL is not about that, so it would be simpler to
tackle that separately.

[tommi, 2016-06-10 15:37:39]

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

[commit-bot: I haz the power, 2016-06-10 15:37:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035173002/350001

[commit-bot: I haz the power, 2016-06-10 15:39:35]

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

[commit-bot: I haz the power, 2016-06-10 15:39:37]

Dry run: Try jobs failed on following builders:
  win_drmemory_light on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...)
  win_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_dbg/builds/1...)
  win_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/9962)
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/15400)

[tommi, 2016-06-10 15:52:35]

Rebase

[tommi, 2016-06-10 15:57:18]

Fix merge

[tommi, 2016-06-10 16:02:10]

Rebase again

[tommi, 2016-06-10 16:08:00]

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

[commit-bot: I haz the power, 2016-06-10 16:08:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035173002/410001

[commit-bot: I haz the power, 2016-06-10 16:13:35]

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

[commit-bot: I haz the power, 2016-06-10 16:13:36]

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

[mflodman, 2016-06-13 18:39:24]

Nice to use the sink interface instead!

A few comments.

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/incl...
File webrtc/common_video/include/incoming_video_stream.h (right):

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/incl...
webrtc/common_video/include/incoming_video_stream.h:29: explicit
IncomingVideoStream(int32_t delay_ms,
Remove explicit.

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

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:105:
rtc::AtomicOps::Increment(&queued_frames_);
Can you add a thread check here?

RTC_DCHECK_RUN_ON(&decoder_thread_checker_);

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/vide...
File webrtc/common_video/video_render_frames.h (right):

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.h:36: int32_t ReleaseAllFrames();
We can remove this method.

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:316: stats_proxy_.OnDecodedFrame();
This is not the right place to log this stat, since we might want to log decode
events even without a renderer. I'll take a note about this and follow up.

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:331: // TODO(tommi): OnRenderFrame grabs a
lock too.
Any thoughts around how to get rid of that lock, and other, in the stats proxy?

[tommi, 2016-06-14 08:11:09]

Address comments

[tommi, 2016-06-14 08:11:28]

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/incl...
File webrtc/common_video/include/incoming_video_stream.h (right):

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/incl...
webrtc/common_video/include/incoming_video_stream.h:29: explicit
IncomingVideoStream(int32_t delay_ms,
On 2016/06/13 18:39:24, mflodman wrote:
> Remove explicit.

Done.

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

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:105:
rtc::AtomicOps::Increment(&queued_frames_);
On 2016/06/13 18:39:24, mflodman wrote:
> Can you add a thread check here?
> 
> RTC_DCHECK_RUN_ON(&decoder_thread_checker_);

Done.

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/vide...
File webrtc/common_video/video_render_frames.h (right):

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.h:36: int32_t ReleaseAllFrames();
On 2016/06/13 18:39:24, mflodman wrote:
> We can remove this method.

Done.

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:316: stats_proxy_.OnDecodedFrame();
On 2016/06/13 18:39:24, mflodman wrote:
> This is not the right place to log this stat, since we might want to log
decode
> events even without a renderer. I'll take a note about this and follow up.

OK.  Move into the decoder somehow?

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:331: // TODO(tommi): OnRenderFrame grabs a
lock too.
On 2016/06/13 18:39:24, mflodman wrote:
> Any thoughts around how to get rid of that lock, and other, in the stats
proxy?

I'd start by seeing if we can update the stats in one call.

A clock is being used by some of these methods to calculate an offset from 'now'
vs something else.  That could be done once at the top (and possibly once more
at the end of this function if necessary), and the calculations done on this
thread. Once all the stats have been calculated, they can be pushed to the stats
proxy and we'd grab a lock once and check the clock fewer times.

[mflodman, 2016-06-14 08:59:26]

LGTM

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/vide...
File webrtc/common_video/video_render_frames.h (right):

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/common_video/vide...
webrtc/common_video/video_render_frames.h:36: int32_t ReleaseAllFrames();
On 2016/06/14 08:11:27, tommi-webrtc wrote:
> On 2016/06/13 18:39:24, mflodman wrote:
> > We can remove this method.
> 
> Done.

Thanks!

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.cc (right):

https://codereview.webrtc.org/2035173002/diff/410001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:316: stats_proxy_.OnDecodedFrame();
On 2016/06/14 08:11:28, tommi-webrtc wrote:
> On 2016/06/13 18:39:24, mflodman wrote:
> > This is not the right place to log this stat, since we might want to log
> decode
> > events even without a renderer. I'll take a note about this and follow up.
> 
> OK.  Move into the decoder somehow?

Yes, to VideoStremDecoder ot VideoReciecveStream. There's no obvious place
currently, but I'll figure something out in another CL.

[tommi, 2016-06-14 09:07:54]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2016-06-14 09:08:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2035173002/430001

[commit-bot: I haz the power, 2016-06-14 11:38:42]

Description was changed from

==========
Split IncomingVideoStream into two implementations, with smoothing and without.

This CL fixes an issue with the non-smoothing implementation where frames were
delivered on the decoder thread.  No-smoothing is now done in a separate class
that uses a TaskQueue.  The implementation may drop frames if the renderer
doesn't keep up and it doesn't block the decoder thread.

Further work done:

* I added TODOs and documentation for VideoReceiveStream::OnFrame, where we
today grab 5 locks.

* I removed the Start/Stop methods from the IncomingVideoStream implementations.
 Now, when an instance is created, it should be considered to be "running" and
when it is deleted, it's "not running".  This saves on resources and also
reduces the amount of locking required and I could remove one critical section
altogether.

* I changed the VideoStreamDecoder class to not depend on IncomingVideoStream
but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface.  This
means that any implementation of that interface can be used and the decoder can
be made to  just use the 'renderer' from the config.  Once we do that, we can
decouple the IncomingVideoStream implementations from the decoder and
VideoReceiveStream implementations and leave it up to the application for how to
do smoothing.  The app can choose to use the Incoming* classes or roll its own
(which may be preferable since applications often have their own scheduling
mechanisms).

* The non-smoothing IncomingVideoStream implementation currently allows only 1
outstanding pending frame.  If we exceed that, the current frame won't be
delivered to the renderer and instead we deliver the next one (since when this
happens, the renderer is falling behind).

* The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in
VideoReceiveStream and not all of the lifetime of VideoReceiveStream.

* Fixed VideoStreamDecoder to unregister callbacks in the dtor that were
registered in the ctor. (this was open to a use-after-free regression)

* Delay and callback pointers are now passed via the ctors to the
IncomingVideoStream classes.  The thread primitives in the IncomingVideoStream
classes are also constructed/destructed at the same time as the owning object,
which allowed me to remove one more lock.

* Removed code in the VideoStreamDecoder that could overwrite the
VideoReceiveStream render delay with a fixed value of 10ms on construction. 
This wasn't a problem with the previous implementation (it would be now though)
but seemed to me like the wrong place to be setting that value.

* Made the render delay value in VideoRenderFrames, const.

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

to

==========
Split IncomingVideoStream into two implementations, with smoothing and without.

This CL fixes an issue with the non-smoothing implementation where frames were
delivered on the decoder thread.  No-smoothing is now done in a separate class
that uses a TaskQueue.  The implementation may drop frames if the renderer
doesn't keep up and it doesn't block the decoder thread.

Further work done:

* I added TODOs and documentation for VideoReceiveStream::OnFrame, where we
today grab 5 locks.

* I removed the Start/Stop methods from the IncomingVideoStream implementations.
 Now, when an instance is created, it should be considered to be "running" and
when it is deleted, it's "not running".  This saves on resources and also
reduces the amount of locking required and I could remove one critical section
altogether.

* I changed the VideoStreamDecoder class to not depend on IncomingVideoStream
but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface.  This
means that any implementation of that interface can be used and the decoder can
be made to  just use the 'renderer' from the config.  Once we do that, we can
decouple the IncomingVideoStream implementations from the decoder and
VideoReceiveStream implementations and leave it up to the application for how to
do smoothing.  The app can choose to use the Incoming* classes or roll its own
(which may be preferable since applications often have their own scheduling
mechanisms).

* The non-smoothing IncomingVideoStream implementation currently allows only 1
outstanding pending frame.  If we exceed that, the current frame won't be
delivered to the renderer and instead we deliver the next one (since when this
happens, the renderer is falling behind).

* The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in
VideoReceiveStream and not all of the lifetime of VideoReceiveStream.

* Fixed VideoStreamDecoder to unregister callbacks in the dtor that were
registered in the ctor. (this was open to a use-after-free regression)

* Delay and callback pointers are now passed via the ctors to the
IncomingVideoStream classes.  The thread primitives in the IncomingVideoStream
classes are also constructed/destructed at the same time as the owning object,
which allowed me to remove one more lock.

* Removed code in the VideoStreamDecoder that could overwrite the
VideoReceiveStream render delay with a fixed value of 10ms on construction. 
This wasn't a problem with the previous implementation (it would be now though)
but seemed to me like the wrong place to be setting that value.

* Made the render delay value in VideoRenderFrames, const.

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

[commit-bot: I haz the power, 2016-06-14 11:38:43]

Committed patchset #23 (id:430001)

[commit-bot: I haz the power, 2016-06-14 11:38:46]

Description was changed from

==========
Split IncomingVideoStream into two implementations, with smoothing and without.

This CL fixes an issue with the non-smoothing implementation where frames were
delivered on the decoder thread.  No-smoothing is now done in a separate class
that uses a TaskQueue.  The implementation may drop frames if the renderer
doesn't keep up and it doesn't block the decoder thread.

Further work done:

* I added TODOs and documentation for VideoReceiveStream::OnFrame, where we
today grab 5 locks.

* I removed the Start/Stop methods from the IncomingVideoStream implementations.
 Now, when an instance is created, it should be considered to be "running" and
when it is deleted, it's "not running".  This saves on resources and also
reduces the amount of locking required and I could remove one critical section
altogether.

* I changed the VideoStreamDecoder class to not depend on IncomingVideoStream
but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface.  This
means that any implementation of that interface can be used and the decoder can
be made to  just use the 'renderer' from the config.  Once we do that, we can
decouple the IncomingVideoStream implementations from the decoder and
VideoReceiveStream implementations and leave it up to the application for how to
do smoothing.  The app can choose to use the Incoming* classes or roll its own
(which may be preferable since applications often have their own scheduling
mechanisms).

* The non-smoothing IncomingVideoStream implementation currently allows only 1
outstanding pending frame.  If we exceed that, the current frame won't be
delivered to the renderer and instead we deliver the next one (since when this
happens, the renderer is falling behind).

* The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in
VideoReceiveStream and not all of the lifetime of VideoReceiveStream.

* Fixed VideoStreamDecoder to unregister callbacks in the dtor that were
registered in the ctor. (this was open to a use-after-free regression)

* Delay and callback pointers are now passed via the ctors to the
IncomingVideoStream classes.  The thread primitives in the IncomingVideoStream
classes are also constructed/destructed at the same time as the owning object,
which allowed me to remove one more lock.

* Removed code in the VideoStreamDecoder that could overwrite the
VideoReceiveStream render delay with a fixed value of 10ms on construction. 
This wasn't a problem with the previous implementation (it would be now though)
but seemed to me like the wrong place to be setting that value.

* Made the render delay value in VideoRenderFrames, const.

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

to

==========
Split IncomingVideoStream into two implementations, with smoothing and without.

This CL fixes an issue with the non-smoothing implementation where frames were
delivered on the decoder thread.  No-smoothing is now done in a separate class
that uses a TaskQueue.  The implementation may drop frames if the renderer
doesn't keep up and it doesn't block the decoder thread.

Further work done:

* I added TODOs and documentation for VideoReceiveStream::OnFrame, where we
today grab 5 locks.

* I removed the Start/Stop methods from the IncomingVideoStream implementations.
 Now, when an instance is created, it should be considered to be "running" and
when it is deleted, it's "not running".  This saves on resources and also
reduces the amount of locking required and I could remove one critical section
altogether.

* I changed the VideoStreamDecoder class to not depend on IncomingVideoStream
but rather use the generic rtc::VideoSinkInterface<VideoFrame> interface.  This
means that any implementation of that interface can be used and the decoder can
be made to  just use the 'renderer' from the config.  Once we do that, we can
decouple the IncomingVideoStream implementations from the decoder and
VideoReceiveStream implementations and leave it up to the application for how to
do smoothing.  The app can choose to use the Incoming* classes or roll its own
(which may be preferable since applications often have their own scheduling
mechanisms).

* The non-smoothing IncomingVideoStream implementation currently allows only 1
outstanding pending frame.  If we exceed that, the current frame won't be
delivered to the renderer and instead we deliver the next one (since when this
happens, the renderer is falling behind).

* The lifetime of the VideoStreamDecoder instance is now bound to Start/Stop in
VideoReceiveStream and not all of the lifetime of VideoReceiveStream.

* Fixed VideoStreamDecoder to unregister callbacks in the dtor that were
registered in the ctor. (this was open to a use-after-free regression)

* Delay and callback pointers are now passed via the ctors to the
IncomingVideoStream classes.  The thread primitives in the IncomingVideoStream
classes are also constructed/destructed at the same time as the owning object,
which allowed me to remove one more lock.

* Removed code in the VideoStreamDecoder that could overwrite the
VideoReceiveStream render delay with a fixed value of 10ms on construction. 
This wasn't a problem with the previous implementation (it would be now though)
but seemed to me like the wrong place to be setting that value.

* Made the render delay value in VideoRenderFrames, const.

BUG=

Committed: https://crrev.com/1c7eef652b0aa22d8ebb0bfe2b547094a794be22
Cr-Commit-Position: refs/heads/master@{#13129}
==========

[commit-bot: I haz the power, 2016-06-14 11:38:48]

Patchset 23 (id:??) landed as
https://crrev.com/1c7eef652b0aa22d8ebb0bfe2b547094a794be22
Cr-Commit-Position: refs/heads/master@{#13129}

[tommi, 2016-06-14 23:04:22]

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

The reason for reverting is: Reverting while we track down the issue on the
Win10 bot..