Reland of Split IncomingVideoStream into two implementations, with smoothing and without.

Reland of IncomingVideoStream refactoring.
This reland does not contain the non-smoothing part of the original implementation.  Instead, when smoothing is turned off, frame callbacks run on the decoder thread, as they did before.  This code path is used in Chrome.  As far as Chrome goes, the difference now is that there won't be an instance of IncomingVideoStream in between the decoder and the callback (i.e. fewer locks).  Other than that, no change for Chrome.

Original issue's description (with non-smoothing references removed):

Split IncomingVideoStream into two implementations, with smoothing and without.

* Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today grab 6 locks.

* 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.

* 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 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=chromium:620232
R=mflodman@webrtc.org, nisse@webrtc.org

Committed: https://crrev.com/884c336c345d988974c2a69cea402b0fb8b07a63
Cr-Commit-Position: refs/heads/master@{#13219}

[tommi, 2016-06-17 08:00:06]

Created Reland of Split IncomingVideoStream into two implementations, with
smoothing and without.

[tommi, 2016-06-17 08:00:38]

Description was changed from

==========
Reland of Split IncomingVideoStream into two implementations, with smoothing and
without. (patchset #1 id:1 of https://codereview.webrtc.org/2071093002/ )

Reason for revert:
Relanding without the non-smoothing implementation.

Original issue's description:
> Revert of Split IncomingVideoStream into two implementations, with smoothing
and without. (patchset #4 id:290001 of https://codereview.webrtc.org/2071473002/
)
> 
> Reason for revert:
> Reverting again.  The perf regression does not seem to be related to dropping
frames.
> 
> Original issue's description:
> > Reland of Split IncomingVideoStream into two implementations, with smoothing
and without.
> >
> > Original issue's description:
> >
> > 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=chromium:620232
> > TBR=mflodman
> >
> > Committed: https://crrev.com/e03f8787377bbc03a4e00184bb14b7561b108cbb
> > Cr-Commit-Position: refs/heads/master@{#13175}
> 
> TBR=mflodman@webrtc.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=chromium:620232
> 
> Committed: https://crrev.com/8e8222d0d249813b000cf4ee3ede1a6699c0d534
> Cr-Commit-Position: refs/heads/master@{#13176}

TBR=mflodman@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=chromium:620232
==========

to

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

Original issue's description:

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=chromium:620232
TBR=mflodman

Committed: https://crrev.com/e03f8787377bbc03a4e00184bb14b7561b108cbb
Cr-Commit-Position: refs/heads/master@{#13175}
==========

[tommi, 2016-06-17 08:00:58]

Description was changed from

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

Original issue's description:

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=chromium:620232
TBR=mflodman

Committed: https://crrev.com/e03f8787377bbc03a4e00184bb14b7561b108cbb
Cr-Commit-Position: refs/heads/master@{#13175}
==========

to

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

Original issue's description:

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=chromium:620232
==========

[tommi, 2016-06-17 08:15:13]

Remove test code for non-smoothing implementation

[tommi, 2016-06-17 08:25:33]

Remove non-smoothing implementation

[tommi, 2016-06-17 08:26:01]

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

[commit-bot: I haz the power, 2016-06-17 08:26:05]

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

[tommi, 2016-06-17 08:31:39]

Description was changed from

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

Original issue's description:

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=chromium:620232
==========

to

==========
Reland of IncomingVideoStream refactoring.
This reland does not contain the non-smoothing part of the original
implementation.  Instead, when smoothing is turned off, frame callbacks run on
the decoder thread, as they did before.  This code path is used in Chrome.  As
far as Chrome goes, the difference now is that there won't be an instance of
IncomingVideoStream in between the decoder and the callback (i.e. fewer locks). 
Other than that, no change for Chrome.

Original issue's description (with non-smoothing references removed):

Split IncomingVideoStream into two implementations, with smoothing and without.

* Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today
grab 6 locks.

* 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.

* 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 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=chromium:620232
==========

[tommi, 2016-06-17 08:33:38]

Hej Magnus - yet another attempt at landing.  This time around, I've removed the
non smoothing class that was using the task queue.  I'll tackle the particular
problem we were seeing on the Win10 bot, separately.  The change now is simpler
than it was before, but still has functional benefits as well as simplifying and
detangling the affected classes.

[commit-bot: I haz the power, 2016-06-17 08:44:25]

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

[commit-bot: I haz the power, 2016-06-17 08:44:26]

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

[tommi, 2016-06-17 10:09:28]

Fix last minute gaffe

[tommi, 2016-06-17 11:47:12]

Patchset #4 (id:300001) has been deleted

[tommi, 2016-06-17 11:48:04]

Fix renderer pointer

[tommi, 2016-06-17 11:56:48]

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

[commit-bot: I haz the power, 2016-06-17 11:56:56]

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

[commit-bot: I haz the power, 2016-06-17 13:51:47]

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

[commit-bot: I haz the power, 2016-06-17 13:51:48]

Dry run: This issue passed the CQ dry run.

[tommi, 2016-06-20 12:49:58]

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

[tommi, 2016-06-20 12:49:58]

mflodman - I'm asking nisse to take a look since you're very busy.  Moved you to
cc.

[nisse-webrtc, 2016-06-20 13:47:55]

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

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/common_video/incl...
webrtc/common_video/include/incoming_video_stream.h:41: void OnFrame(const
VideoFrame& video_frame) override;
This is a public method on the VideoSinkInterface, so it looks a bit strange to
make it private here. If really intended, maybe make the inheritance non-public
instead?

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

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:13: #include
"webrtc/base/atomicops.h"
Unused.

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/end_to_end_...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/end_to_end_...
webrtc/video/end_to_end_tests.cc:2047: void OnFrame(const VideoFrame&
video_frame) override {}
Why private? (Also applies to ReceiveStreamRenderer::OnFrame, below).

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

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:249: incoming_video_stream_.reset(
This is the only assignment, and it's a private member, so there should be no
harm in a using a more specific type.

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:99:
std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>> incoming_video_stream_;
I think ownership here is a bit unfortunate. The grand plan is to move ownership
to whoever creates the webrtc::VideoReceivStream::Config (which uses an un-owned
VideoSinkInterface pointer)?

Then I think it makes sense to change this back to an IncomingVideoStream
pointer, and think about it not as the a pointer to the sink to use, but a
pointer to an object we own and have to clean up. And let the destructor of the
abstract VideoSinkInterface stay non-public.

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_strea...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_strea...
webrtc/video/video_stream_decoder.h:101: // We shouldn't need to support both.
Like the similar pre_encode_callback in VideoSendStream, it's only used by
tests. Both should be eliminated, by letting tests register as additional sinks
at the appropriate frame source. Not clear to me how that works on the receive
side, though.

For typing, you can probably replace it by VideoSinkInterface right away,
I420FrameCallback is nothing more than an alias for
VideoSinkInterface<webrtc::VideoFrame>. And delete I420FrameCallback if this is
the last use.

[tommi, 2016-06-20 14:24:49]

Remove unused header

[tommi, 2016-06-20 14:24:52]

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

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/common_video/incl...
webrtc/common_video/include/incoming_video_stream.h:41: void OnFrame(const
VideoFrame& video_frame) override;
On 2016/06/20 13:47:55, nisse-webrtc wrote:
> This is a public method on the VideoSinkInterface, so it looks a bit strange
to
> make it private here. If really intended, maybe make the inheritance
non-public
> instead?

As discussed, this is a commonly used technique in webrtc, chromium and google,
which makes the public interface of the class that implements the interface,
more focused.  An instance of the class can still be cast into the interface it
implements, but doing so needs to be done explicitly, which helps.
With private inheritance, casting to the interface wouldn't be allowed at all
from outside the class.

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

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/common_video/inco...
webrtc/common_video/incoming_video_stream.cc:13: #include
"webrtc/base/atomicops.h"
On 2016/06/20 13:47:55, nisse-webrtc wrote:
> Unused.

Done.

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

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.cc:249: incoming_video_stream_.reset(
On 2016/06/20 13:47:55, nisse-webrtc wrote:
> This is the only assignment, and it's a private member, so there should be no
> harm in a using a more specific type.

as discussed, there's little harm besides the additional dependencies in the
header, but there aren't benefits either to depending on the more fully declared
type.  Since we're moving towards having only the config's renderer pointer,
this is more future proof and discourages regressions (e.g. if someone adds a
method to IncomingVideoStream, they'll have to change the type of this pointer
before calling).

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:99:
std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>> incoming_video_stream_;
On 2016/06/20 13:47:55, nisse-webrtc wrote:
> I think ownership here is a bit unfortunate. The grand plan is to move
ownership
> to whoever creates the webrtc::VideoReceivStream::Config (which uses an
un-owned
> VideoSinkInterface pointer)?

Yes (or something along those lines)

> Then I think it makes sense to change this back to an IncomingVideoStream
> pointer, and think about it not as the a pointer to the sink to use, but a
> pointer to an object we own and have to clean up. And let the destructor of
the
> abstract VideoSinkInterface stay non-public.

I think making the dtor of VideoSinkInterface public, is still a good thing and
required for the above grand plan.  There's no refcounting going on, so the
public dtor should be fine.

The interface that's needed here, is a VideoSinkInterface, and not specifically
IncomingVideoStream.  We only need to call the ctor of IncomingVideoStream and
once we've got that, we don't need more.  This also makes the class' header
dependencies one class less and the same as for the decoder (in all cases, not
just with smoothing enabled).

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_strea...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_strea...
webrtc/video/video_stream_decoder.h:101: // We shouldn't need to support both.
On 2016/06/20 13:47:55, nisse-webrtc wrote:
> Like the similar pre_encode_callback in VideoSendStream, it's only used by
> tests. Both should be eliminated, by letting tests register as additional
sinks
> at the appropriate frame source. Not clear to me how that works on the receive
> side, though.
> 
> For typing, you can probably replace it by VideoSinkInterface right away,
> I420FrameCallback is nothing more than an alias for
> VideoSinkInterface<webrtc::VideoFrame>. And delete I420FrameCallback if this
is
> the last use. 

looked into it and found the type is still different.  Hope to get rid of it
soon enough though.

[nisse-webrtc, 2016-06-20 14:51:40]

lgtm

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

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/common_video/incl...
webrtc/common_video/include/incoming_video_stream.h:41: void OnFrame(const
VideoFrame& video_frame) override;
On 2016/06/20 14:24:51, tommi-webrtc wrote:
> On 2016/06/20 13:47:55, nisse-webrtc wrote:
> > This is a public method on the VideoSinkInterface, so it looks a bit strange
> to
> > make it private here. If really intended, maybe make the inheritance
> non-public
> > instead?
> 
> As discussed, this is a commonly used technique in webrtc, chromium and
google,
> which makes the public interface of the class that implements the interface,
> more focused.  An instance of the class can still be cast into the interface
it
> implements, but doing so needs to be done explicitly, which helps.
> With private inheritance, casting to the interface wouldn't be allowed at all
> from outside the class.

Thanks for the explanation. So private inheritance might work in some cases, but
a private overidden method is the chromium convention.

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:99:
std::unique_ptr<rtc::VideoSinkInterface<VideoFrame>> incoming_video_stream_;
On 2016/06/20 14:24:51, tommi-webrtc wrote:
> On 2016/06/20 13:47:55, nisse-webrtc wrote:
> > I think ownership here is a bit unfortunate. The grand plan is to move
> ownership
> > to whoever creates the webrtc::VideoReceivStream::Config (which uses an
> un-owned
> > VideoSinkInterface pointer)?
> 
> Yes (or something along those lines)
> 
> > Then I think it makes sense to change this back to an IncomingVideoStream
> > pointer, and think about it not as the a pointer to the sink to use, but a
> > pointer to an object we own and have to clean up. And let the destructor of
> the
> > abstract VideoSinkInterface stay non-public.
> 
> I think making the dtor of VideoSinkInterface public, is still a good thing
and
> required for the above grand plan.  There's no refcounting going on, so the
> public dtor should be fine.
> 
> The interface that's needed here, is a VideoSinkInterface, and not
specifically
> IncomingVideoStream.  We only need to call the ctor of IncomingVideoStream and
> once we've got that, we don't need more.  This also makes the class' header
> dependencies one class less and the same as for the decoder (in all cases, not
> just with smoothing enabled).

Acknowledged.

When we introduced this interface some months ago after extensive design
discussions, we might have thought that it would be used by refcounted objects.
If that's not the case, a public constructor makes a lot more sense.

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_strea...
File webrtc/video/video_stream_decoder.h (right):

https://codereview.webrtc.org/2078873002/diff/320001/webrtc/video/video_strea...
webrtc/video/video_stream_decoder.h:101: // We shouldn't need to support both.
On 2016/06/20 14:24:51, tommi-webrtc wrote:
> On 2016/06/20 13:47:55, nisse-webrtc wrote:
> > Like the similar pre_encode_callback in VideoSendStream, it's only used by
> > tests. Both should be eliminated, by letting tests register as additional
> sinks
> > at the appropriate frame source. Not clear to me how that works on the
receive
> > side, though.
> > 
> > For typing, you can probably replace it by VideoSinkInterface right away,
> > I420FrameCallback is nothing more than an alias for
> > VideoSinkInterface<webrtc::VideoFrame>. And delete I420FrameCallback if this
> is
> > the last use. 
> 
> looked into it and found the type is still different.  Hope to get rid of it
> soon enough though.

For reference, the sender-side change from I420FrameCallback to
VideoSinkInterface was done in cl https://codereview.webrtc.org/1891733002/

The version of frame_callback.h I had lying around must have been a remnant of
some intermediate state.

[mflodman, 2016-06-20 15:20:50]

mflodman@webrtc.org changed reviewers:
+ mflodman@webrtc.org, philipel@webrtc.org

[mflodman, 2016-06-20 15:20:51]

LGTM

But,
Philip encountered crashes in Chromium due do to the video sink interface being
removed, but then the crash went away again. One of the changes in this CL is
moving the IncomingVideoStream creation / destruction to
VideoStreamReceiver::[Start,Stop], could that be what causes the crash?

Philip,
What do you think?

[tommi, 2016-06-20 15:36:21]

On 2016/06/20 15:20:51, mflodman wrote:
> LGTM
> 
> But,
> Philip encountered crashes in Chromium due do to the video sink interface
being
> removed, but then the crash went away again. One of the changes in this CL is
> moving the IncomingVideoStream creation / destruction to
> VideoStreamReceiver::[Start,Stop], could that be what causes the crash?
> 
> Philip,
> What do you think?

Do you have more information about that crash?  I am reducing the scope of a few
things to Start/Stop.  This includes incoming video stream, but also the
VideoStreamDecoder.  In the VSD, I found bugs that I'm fixing whereby we
registered for callbacks in the ctor but didn't unregister.  This wasn't a
problem since the objects that provided the callbacks, ended up with overlapping
only with the creation time of the decoder instance, but not its destruction
(asymmetric).  Now that lifetime is symmetric, I discovered those issues, but I
think they've been fixed.

[mflodman, 2016-06-20 15:53:37]

On 2016/06/20 15:36:21, tommi-webrtc wrote:
> On 2016/06/20 15:20:51, mflodman wrote:
> > LGTM
> > 
> > But,
> > Philip encountered crashes in Chromium due do to the video sink interface
> being
> > removed, but then the crash went away again. One of the changes in this CL
is
> > moving the IncomingVideoStream creation / destruction to
> > VideoStreamReceiver::[Start,Stop], could that be what causes the crash?
> > 
> > Philip,
> > What do you think?
> 
> Do you have more information about that crash?  I am reducing the scope of a
few
> things to Start/Stop.  This includes incoming video stream, but also the
> VideoStreamDecoder.  In the VSD, I found bugs that I'm fixing whereby we
> registered for callbacks in the ctor but didn't unregister.  This wasn't a
> problem since the objects that provided the callbacks, ended up with
overlapping
> only with the creation time of the decoder instance, but not its destruction
> (asymmetric).  Now that lifetime is symmetric, I discovered those issues, but
I
> think they've been fixed.

That could absolutely be it in this case, but I don't really know the details of
the crash Philip encountered.

[philipel, 2016-06-20 15:54:47]

On 2016/06/20 15:36:21, tommi-webrtc wrote:
> On 2016/06/20 15:20:51, mflodman wrote:
> > LGTM
> > 
> > But,
> > Philip encountered crashes in Chromium due do to the video sink interface
> being
> > removed, but then the crash went away again. One of the changes in this CL
is
> > moving the IncomingVideoStream creation / destruction to
> > VideoStreamReceiver::[Start,Stop], could that be what causes the crash?
> > 
> > Philip,
> > What do you think?
> 
> Do you have more information about that crash?  I am reducing the scope of a
few
> things to Start/Stop.  This includes incoming video stream, but also the
> VideoStreamDecoder.  In the VSD, I found bugs that I'm fixing whereby we
> registered for callbacks in the ctor but didn't unregister.  This wasn't a
> problem since the objects that provided the callbacks, ended up with
overlapping
> only with the creation time of the decoder instance, but not its destruction
> (asymmetric).  Now that lifetime is symmetric, I discovered those issues, but
I
> think they've been fixed.

The crashes were not due to the video sink being null, it was due to other
reasons (not de-registering a module, packet buffer being destroyed before frame
objects) . But a thing I did notice though was that in the case of hangouts, it
takes a long time for the video sink to register which cause many frames (>100)
to be dropped before anything is rendered. Not sure if that is relevant to this
CL though...

[tommi, 2016-06-20 17:43:14]

Description was changed from

==========
Reland of IncomingVideoStream refactoring.
This reland does not contain the non-smoothing part of the original
implementation.  Instead, when smoothing is turned off, frame callbacks run on
the decoder thread, as they did before.  This code path is used in Chrome.  As
far as Chrome goes, the difference now is that there won't be an instance of
IncomingVideoStream in between the decoder and the callback (i.e. fewer locks). 
Other than that, no change for Chrome.

Original issue's description (with non-smoothing references removed):

Split IncomingVideoStream into two implementations, with smoothing and without.

* Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today
grab 6 locks.

* 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.

* 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 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=chromium:620232
==========

to

==========
Reland of IncomingVideoStream refactoring.
This reland does not contain the non-smoothing part of the original
implementation.  Instead, when smoothing is turned off, frame callbacks run on
the decoder thread, as they did before.  This code path is used in Chrome.  As
far as Chrome goes, the difference now is that there won't be an instance of
IncomingVideoStream in between the decoder and the callback (i.e. fewer locks). 
Other than that, no change for Chrome.

Original issue's description (with non-smoothing references removed):

Split IncomingVideoStream into two implementations, with smoothing and without.

* Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today
grab 6 locks.

* 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.

* 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 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=chromium:620232
R=mflodman@webrtc.org, nisse@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/884c336c345d988974c2a69ce...
==========

[tommi, 2016-06-20 17:43:17]

Committed patchset #5 (id:340001) manually as
884c336c345d988974c2a69cea402b0fb8b07a63 (presubmit successful).

[commit-bot: I haz the power, 2016-06-20 17:43:18]

Description was changed from

==========
Reland of IncomingVideoStream refactoring.
This reland does not contain the non-smoothing part of the original
implementation.  Instead, when smoothing is turned off, frame callbacks run on
the decoder thread, as they did before.  This code path is used in Chrome.  As
far as Chrome goes, the difference now is that there won't be an instance of
IncomingVideoStream in between the decoder and the callback (i.e. fewer locks). 
Other than that, no change for Chrome.

Original issue's description (with non-smoothing references removed):

Split IncomingVideoStream into two implementations, with smoothing and without.

* Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today
grab 6 locks.

* 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.

* 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 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=chromium:620232
R=mflodman@webrtc.org, nisse@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/884c336c345d988974c2a69ce...
==========

to

==========
Reland of IncomingVideoStream refactoring.
This reland does not contain the non-smoothing part of the original
implementation.  Instead, when smoothing is turned off, frame callbacks run on
the decoder thread, as they did before.  This code path is used in Chrome.  As
far as Chrome goes, the difference now is that there won't be an instance of
IncomingVideoStream in between the decoder and the callback (i.e. fewer locks). 
Other than that, no change for Chrome.

Original issue's description (with non-smoothing references removed):

Split IncomingVideoStream into two implementations, with smoothing and without.

* Added TODOs and documentation for VideoReceiveStream::OnFrame, where we today
grab 6 locks.

* 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.

* 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 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=chromium:620232
R=mflodman@webrtc.org, nisse@webrtc.org

Committed: https://crrev.com/884c336c345d988974c2a69cea402b0fb8b07a63
Cr-Commit-Position: refs/heads/master@{#13219}
==========

[sakal, 2016-06-21 07:08:33]

A revert of this CL (patchset #5 id:340001) has been created in
https://codereview.webrtc.org/2084873002/ by sakal@webrtc.org.

The reason for reverting is: Breaks chromium.webrtc.fyi

https://uberchromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Win7%20Te...
https://uberchromegw.corp.google.com/i/chromium.webrtc.fyi/builders/Win10%20T....