Issue 1655793003: Make cricket::VideoCapturer implement VideoSourceInterface

perkj_webrtc

Description was changed from ========== Make WebRtcVideoChannel2::WebRtcVideoSendStream implement rtc::VideoRendererInterface. BUG=webrtc:5426 patch from issue 1631703002 at ...

4 years, 10 months ago (2016-02-01 17:37:19 UTC) #1

pthatcher

pthatcher@google.com changed reviewers: + pthatcher@google.com

4 years, 10 months ago (2016-02-01 18:40:53 UTC) #2

pthatcher

Most of my comments are minor. I think the big issues left are: 1. There ...

4 years, 10 months ago (2016-02-01 18:40:54 UTC) #3

nisse-webrtc

nisse@webrtc.org changed reviewers: + nisse@webrtc.org

4 years, 10 months ago (2016-02-02 09:52:30 UTC) #4

nisse-webrtc

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosinkinterface.h File webrtc/media/base/videosinkinterface.h (right): https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosinkinterface.h#newcode24 webrtc/media/base/videosinkinterface.h:24: virtual VideoSinkCapabilities getCapabilities() { return VideoSinkCapabilities(); } On 2016/02/01 ...

4 years, 10 months ago (2016-02-02 09:52:30 UTC) #5

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
File webrtc/media/base/videosinkinterface.h (right):

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
webrtc/media/base/videosinkinterface.h:24: virtual VideoSinkCapabilities
getCapabilities() { return  VideoSinkCapabilities(); }
On 2016/02/01 18:40:54, pthatcher wrote:
> These needs a SignalCapabilitiesChanged signal to indicate when it changes.  

I'm not sure what you mean by "signal" here. We really should keep this as
simple as possible. I'd really prefer not to add a separate callback function
for it. Some possibilities:

1. Let the sink just call RemoveSink and AddSink. Potential drawback is that we
might miss one frame if OnFrame is called on a separate thread. If we add the
capabilities as an argument to AddSink, we don't even need the getCapabilities
method.
 
2. Add a method which is equivalent to RemoveSink and AddSink, except that it
doesn't modify the list, avoiding the above race. Could be called
SignalCapabilitiesChanged, or ReAddSink, or whatever. Should take capabilities
as an argument, so still no need for getCapabilities.

3. Give OnFrame a return value, where true indicates changed capabilities. The
idea is that the source shouldn't care about capabilities when it's not
generating any frames. Potential drawback is that the new capabilities will not
be taken into consideration until the next frame, but that's probably no big
deal. Has the advantage over (1) and (2) that the sink doesn't need to keep any
backpointer to its source. Which makes things a little more flexible, the
responsibility for linking source and sink together (calling AddSink and
RemoveSink at the right time can be given to some other object). We could
consider having the new capabilities returned at the same time, via a pointer
argument to OnFrame.

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
webrtc/media/base/videosinkinterface.h:24: virtual VideoSinkCapabilities
getCapabilities() { return  VideoSinkCapabilities(); }
Will all flags really be capabilities, or is it better to just call them
properties? We were just discussing how to do android-specific hacks, where we
may need to pass things like texture references up and down the pipeline.

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:38: void DeliverFrame(const
VideoFrameT*);
I'm not entirely sure we should have a general mux object here. We have muxes,
e.g., VideoTrack ought to ingerit both VideoSourceInterface and
VideoSinkInterface.

I think we should do one of the following:

1. Let VideoSourceBase maintain a protected: sink list (should it be std::list
or std::vector, btw?), implement AddSink and RemoveSink, and nothing more.

2. Write a VideoMux class which implements both VideoSourceInterface and
VideoSinkInterface. It should also be responsible for merging of capabilities.
VideoTrack would inherit this class, and only add the enabled state and mute
logic (by overriding OnFrame).

3. Do no generic implementation now, just implement what's needed right now in
each VideoSource classes.

My gut feeling is that (1) is mostly useless abstraction, it will be just a
std::vector with renamed methods (this depends also on whether or not we include
the capabilities with the AddSink method), while (2) makes sense but I think
it's a bit premature to do now, let's do this abstraction after we have two
places which needs it.

By the way, do you think we will need a "capability" saying that the sink is
muted, i.e., currently not interested in any frames? May be useful for a
disabled VideoTrack, or for a general mux with no downstream sinks (I guess your
thinking of something similar with the HasSinks method?).

perkj_webrtc

Thanks for all your feedback. I am sorry if the cl was bit pre-mature when ...

4 years, 10 months ago (2016-02-02 16:24:00 UTC) #6

Thanks for all your feedback. I am sorry if the cl was bit pre-mature when you
looked at it. I sent the mail yesterday mostly to say that I had started and
though we should take a look at this before continuing on niels cl where he made
the WebRtcVideoChannel2::SentStream implement the sink interface.

Anyway - I have spend most of the day on this - can you take another look?
(Android still need to be fixed)

https://codereview.webrtc.org/1655793003/diff/20001/talk/media/base/videocapt...
File talk/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1655793003/diff/20001/talk/media/base/videocapt...
talk/media/base/videocapturer.cc:537: SignalVideoFrame(this,
adapted_frame.get());
On 2016/02/01 18:40:54, pthatcher wrote:
> We should leave a comment about how we fire both while we're transitioning
from
> one to the other (from SignalVideoFrame to DeliverFrame).

SignalVideoFrame now removed. I was just not done yesterday.

https://codereview.webrtc.org/1655793003/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (left):

https://codereview.webrtc.org/1655793003/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1653: }
On 2016/02/01 18:40:54, pthatcher wrote:
> It can't be muted any more?

Muted as was moved to the track enabled/disabled by niels. muted_ was/ is
controlled by setting the track to disabled... So this will create a black frame
out of a black frame....

Anyway lets keep this for the moment and focus on the source/sink refactoring in
this cl.

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
File webrtc/media/base/videosinkinterface.h (right):

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
webrtc/media/base/videosinkinterface.h:17: bool can_apply_rotation_ = false;
On 2016/02/01 18:40:54, pthatcher wrote:
> Shouldn't that be can_apply_rotation (no "_" on the end)?

Done.

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
webrtc/media/base/videosinkinterface.h:24: virtual VideoSinkCapabilities
getCapabilities() { return  VideoSinkCapabilities(); }
On 2016/02/02 09:52:30, nisse-webrtc wrote:
> On 2016/02/01 18:40:54, pthatcher wrote:
> > These needs a SignalCapabilitiesChanged signal to indicate when it changes. 

> 
> I'm not sure what you mean by "signal" here. We really should keep this as
> simple as possible. I'd really prefer not to add a separate callback function
> for it. Some possibilities:
> 
> 1. Let the sink just call RemoveSink and AddSink. Potential drawback is that
we
> might miss one frame if OnFrame is called on a separate thread. If we add the
> capabilities as an argument to AddSink, we don't even need the getCapabilities
> method.
>  
> 2. Add a method which is equivalent to RemoveSink and AddSink, except that it
> doesn't modify the list, avoiding the above race. Could be called
> SignalCapabilitiesChanged, or ReAddSink, or whatever. Should take capabilities
> as an argument, so still no need for getCapabilities.
> 
> 3. Give OnFrame a return value, where true indicates changed capabilities. The
> idea is that the source shouldn't care about capabilities when it's not
> generating any frames. Potential drawback is that the new capabilities will
not
> be taken into consideration until the next frame, but that's probably no big
> deal. Has the advantage over (1) and (2) that the sink doesn't need to keep
any
> backpointer to its source. Which makes things a little more flexible, the
> responsibility for linking source and sink together (calling AddSink and
> RemoveSink at the right time can be given to some other object). We could
> consider having the new capabilities returned at the same time, via a pointer
> argument to OnFrame.

I had not got that far yesterday but I would really like to avoid sigslots and
Signals in an interface. 
I went with nisses suggestion 1 here.

Another think- we can remove VideoSinkInterface all together and just register a
callback object (Functor in bind.h) to receive VideoFrames through the new
VideoSourceInterface::AddSink.  Then no one have to implement
VideoSinkInterface. I don't have a strong opinion but the it would mean that all
current places where we receive video frames does not have to be renamed.

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:29: class VideoSourceBase : public
VideoSourceInterface<VideoFrameT> {
On 2016/02/01 18:40:54, pthatcher wrote:
> Where is the impl of this?

Sorry - maybe I was unclear yesterday- the cl was not done. It was more a
statement that I had started on this and wanted no let you and nisse know so he
did not spend time on updating all unittests to pass his attempt.

Not this is moved to talk/media/base since I have to add the cc-file to a gyp
file.

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:38: void DeliverFrame(const
VideoFrameT*);
On 2016/02/01 18:40:54, pthatcher wrote:
> Can you call this DeliverFrameToSinks, or better, SendFrameToSinks?

Done.

https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
webrtc/media/base/videosourceinterface.h:41: VideoSinkCapabilities
capabilities_;
On 2016/02/01 18:40:54, pthatcher wrote:
> Is this a merged capabilities?  Is it worth storing?  Could you not just have
a
> capabilities() method that computes it on the fly?

First attempt I would just like this to handle the case where all sinks have the
same capabilities. Ie- the way we use it today. I use this to DCHECK that they
actually are the same.

perkj_webrtc

On 2016/02/02 16:24:00, perkj_webrtc wrote: > Thanks for all your feedback. I am sorry if ...

4 years, 10 months ago (2016-02-02 16:34:47 UTC) #7

On 2016/02/02 16:24:00, perkj_webrtc wrote:
> Thanks for all your feedback. I am sorry if the cl was bit pre-mature when you
> looked at it. I sent the mail yesterday mostly to say that I had started and
> though we should take a look at this before continuing on niels cl where he
made
> the WebRtcVideoChannel2::SentStream implement the sink interface.
> 
> 
> Anyway - I have spend most of the day on this - can you take another look?
> (Android still need to be fixed)
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/talk/media/base/videocapt...
> File talk/media/base/videocapturer.cc (right):
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/talk/media/base/videocapt...
> talk/media/base/videocapturer.cc:537: SignalVideoFrame(this,
> adapted_frame.get());
> On 2016/02/01 18:40:54, pthatcher wrote:
> > We should leave a comment about how we fire both while we're transitioning
> from
> > one to the other (from SignalVideoFrame to DeliverFrame).
> 
> SignalVideoFrame now removed. I was just not done yesterday.
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvideoengine2.cc (left):
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvideoengine2.cc:1653: }
> On 2016/02/01 18:40:54, pthatcher wrote:
> > It can't be muted any more?
> 
> Muted as was moved to the track enabled/disabled by niels. muted_ was/ is
> controlled by setting the track to disabled... So this will create a black
frame
> out of a black frame....
> 
> Anyway lets keep this for the moment and focus on the source/sink refactoring
in
> this cl.
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
> File webrtc/media/base/videosinkinterface.h (right):
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
> webrtc/media/base/videosinkinterface.h:17: bool can_apply_rotation_ = false;
> On 2016/02/01 18:40:54, pthatcher wrote:
> > Shouldn't that be can_apply_rotation (no "_" on the end)?
> 
> Done.
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
> webrtc/media/base/videosinkinterface.h:24: virtual VideoSinkCapabilities
> getCapabilities() { return  VideoSinkCapabilities(); }
> On 2016/02/02 09:52:30, nisse-webrtc wrote:
> > On 2016/02/01 18:40:54, pthatcher wrote:
> > > These needs a SignalCapabilitiesChanged signal to indicate when it
changes. 
> 
> > 
> > I'm not sure what you mean by "signal" here. We really should keep this as
> > simple as possible. I'd really prefer not to add a separate callback
function
> > for it. Some possibilities:
> > 
> > 1. Let the sink just call RemoveSink and AddSink. Potential drawback is that
> we
> > might miss one frame if OnFrame is called on a separate thread. If we add
the
> > capabilities as an argument to AddSink, we don't even need the
getCapabilities
> > method.
> >  
> > 2. Add a method which is equivalent to RemoveSink and AddSink, except that
it
> > doesn't modify the list, avoiding the above race. Could be called
> > SignalCapabilitiesChanged, or ReAddSink, or whatever. Should take
capabilities
> > as an argument, so still no need for getCapabilities.
> > 
> > 3. Give OnFrame a return value, where true indicates changed capabilities.
The
> > idea is that the source shouldn't care about capabilities when it's not
> > generating any frames. Potential drawback is that the new capabilities will
> not
> > be taken into consideration until the next frame, but that's probably no big
> > deal. Has the advantage over (1) and (2) that the sink doesn't need to keep
> any
> > backpointer to its source. Which makes things a little more flexible, the
> > responsibility for linking source and sink together (calling AddSink and
> > RemoveSink at the right time can be given to some other object). We could
> > consider having the new capabilities returned at the same time, via a
pointer
> > argument to OnFrame.
> 
> I had not got that far yesterday but I would really like to avoid sigslots and
> Signals in an interface. 
> I went with nisses suggestion 1 here.
> 
> 
> Another think- we can remove VideoSinkInterface all together and just register
a
> callback object (Functor in bind.h) to receive VideoFrames through the new
> VideoSourceInterface::AddSink.  Then no one have to implement
> VideoSinkInterface. I don't have a strong opinion but the it would mean that
all
> current places where we receive video frames does not have to be renamed.
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
> File webrtc/media/base/videosourceinterface.h (right):
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
> webrtc/media/base/videosourceinterface.h:29: class VideoSourceBase : public
> VideoSourceInterface<VideoFrameT> {
> On 2016/02/01 18:40:54, pthatcher wrote:
> > Where is the impl of this?
> 
> Sorry - maybe I was unclear yesterday- the cl was not done. It was more a
> statement that I had started on this and wanted no let you and nisse know so
he
> did not spend time on updating all unittests to pass his attempt.
> 
> Not this is moved to talk/media/base since I have to add the cc-file to a gyp
> file.
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
> webrtc/media/base/videosourceinterface.h:38: void DeliverFrame(const
> VideoFrameT*);
> On 2016/02/01 18:40:54, pthatcher wrote:
> > Can you call this DeliverFrameToSinks, or better, SendFrameToSinks?
> 
> Done.
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
> webrtc/media/base/videosourceinterface.h:41: VideoSinkCapabilities
> capabilities_;
> On 2016/02/01 18:40:54, pthatcher wrote:
> > Is this a merged capabilities?  Is it worth storing?  Could you not just
have
> a
> > capabilities() method that computes it on the fly?
> 
> First attempt I would just like this to handle the case where all sinks have
the
> same capabilities. Ie- the way we use it today. I use this to DCHECK that they
> actually are the same.

And Android should now be fixed... A lock order inversion to look into left or
remove the lock....

pthatcher1

On 2016/02/02 09:52:30, nisse-webrtc wrote: > https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosinkinterface.h > File webrtc/media/base/videosinkinterface.h (right): > > https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosinkinterface.h#newcode24 > ...

4 years, 10 months ago (2016-02-02 19:29:00 UTC) #8

On 2016/02/02 09:52:30, nisse-webrtc wrote:
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
> File webrtc/media/base/videosinkinterface.h (right):
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
> webrtc/media/base/videosinkinterface.h:24: virtual VideoSinkCapabilities
> getCapabilities() { return  VideoSinkCapabilities(); }
> On 2016/02/01 18:40:54, pthatcher wrote:
> > These needs a SignalCapabilitiesChanged signal to indicate when it changes. 

> 
> I'm not sure what you mean by "signal" here. 

Just like all the other signals in our code base:

sigslot::signal1<VideoSink*> SignalCapabilitiesChanged;

> We really should keep this as
> simple as possible. I'd really prefer not to add a separate callback function
> for it. Some possibilities:
> 
> 1. Let the sink just call RemoveSink and AddSink. Potential drawback is that
we
> might miss one frame if OnFrame is called on a separate thread. If we add the
> capabilities as an argument to AddSink, we don't even need the getCapabilities
> method.

I think the need for the sink to have a reference to the source kills options #1
and #2.

>  
> 2. Add a method which is equivalent to RemoveSink and AddSink, except that it
> doesn't modify the list, avoiding the above race. Could be called
> SignalCapabilitiesChanged, or ReAddSink, or whatever. Should take capabilities
> as an argument, so still no need for getCapabilities.
> 

> 3. Give OnFrame a return value, where true indicates changed capabilities. The
> idea is that the source shouldn't care about capabilities when it's not
> generating any frames. Potential drawback is that the new capabilities will
not
> be taken into consideration until the next frame, but that's probably no big
> deal.  Has the advantage over (1) and (2) that the sink doesn't need to keep
any
> backpointer to its source. Which makes things a little more flexible, the
> responsibility for linking source and sink together (calling AddSink and
> RemoveSink at the right time can be given to some other object). We could
> consider having the new capabilities returned at the same time, via a pointer
> argument to OnFrame.
> 

That's the equivalent of having the VideoSource call capabilities() every time
before it calls OnFrame.  Which might be a reasonable thing to do, except for
having too call paths called for each frame, which may or may not be a
performance problems (we'd have to measure :).

Here's an alternative version of #3 that might work, call it option #4:

Instead of having capabilities() and SignalCapabilitiesChanged, we have:

virtual FrameResult OnFrame(VideoFrameT frame, FrameResultDetails* details);

enum FrameResult {
  FRAME_CONSUMED
  FRAME_EXCEEDS_CAPABILITIES
}

struct FrameResultDetails {
  bool can_apply_rotation;
}

If the source gets back FRAME_EXCEEDS_CAPABILITIES, it knows to check the
capabilities and re-send the frame and future frames in a different way
(rotated).  If the source gets back FRAME_CONSUMED, it can still check the
capabilities to see if it can stop rotating the frame (see if it changed).  

>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videosi...
> webrtc/media/base/videosinkinterface.h:24: virtual VideoSinkCapabilities
> getCapabilities() { return  VideoSinkCapabilities(); }
> Will all flags really be capabilities, or is it better to just call them
> properties? We were just discussing how to do android-specific hacks, where we
> may need to pass things like texture references up and down the pipeline.

I don't want to get too generic with things like "properties".  But perhaps
calling it something more like "desired frames" or "requests" might make more
sense than just "capabilities".  Perhaps if we made a list of things that we'd
actually use this for, we'd have a better idea of what to call them.  So far, we
have: can_apply_rotation and wants_frame.   I can also imagine
max_usable_resolution.  Anything more?  Something about textures?

> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
> File webrtc/media/base/videosourceinterface.h (right):
> 
>
https://codereview.webrtc.org/1655793003/diff/20001/webrtc/media/base/videoso...
> webrtc/media/base/videosourceinterface.h:38: void DeliverFrame(const
> VideoFrameT*);
> I'm not entirely sure we should have a general mux object here. We have muxes,
> e.g., VideoTrack ought to ingerit both VideoSourceInterface and
> VideoSinkInterface.
> 
> I think we should do one of the following:
> 
> 1. Let VideoSourceBase maintain a protected: sink list (should it be std::list
> or std::vector, btw?), implement AddSink and RemoveSink, and nothing more.
> 
> 2. Write a VideoMux class which implements both VideoSourceInterface and
> VideoSinkInterface. It should also be responsible for merging of capabilities.
> VideoTrack would inherit this class, and only add the enabled state and mute
> logic (by overriding OnFrame).
> 
> 3. Do no generic implementation now, just implement what's needed right now in
> each VideoSource classes.

I think we should have one place where we have muxing (not #3).  I would prefer
to have it not be a base class (not #1).  I like the idea of having one class
that is both a source and a sink.  And if you want to be a source to multiple
sinks, you "have a" (not "is a") one of these things.

My question is:  is VideoTrack the only place where want one of these?  An
decoder goes into a VideoTrack, and a VideoTrack out to many sinks.  An encoder
is one of many sinks attached to a video track.  I think option #4 might be: 
implementing muxing in VideoTrack and not anywhere else.  Which may be is a
limited version of #3.

> 
> My gut feeling is that (1) is mostly useless abstraction, it will be just a
> std::vector with renamed methods (this depends also on whether or not we
include
> the capabilities with the AddSink method), while (2) makes sense but I think
> it's a bit premature to do now, let's do this abstraction after we have two
> places which needs it.
> 
> By the way, do you think we will need a "capability" saying that the sink is
> muted, i.e., currently not interested in any frames? May be useful for a
> disabled VideoTrack, or for a general mux with no downstream sinks (I guess
your
> thinking of something similar with the HasSinks method?).

Yes, I think so.  This is similar to frame decimation, except for a very long
time.  The problem is that if we don't have a signal for "I'm ready for a frame
now", then how does the source know the sink is ready for another frame without
attempting to give it a frame or polling capabilities()?

In fact, a SignalFrameNeeded is something we need to fix a bug we have where the
source doesn't output frames for a long time (a screencast where nothing
changes), and a sink is attached later, but never gets a frame because nothing
changes.  A similar issue could happen where a sink is unmuted but then a new
frame never comes out.  That could be fixed by a combination of AddSink always
re-sending the last frame sent and unmute always re-consuming the last frame
sent, but a SignalFrameNeeded might be more explicit and clear, and useful for
frame decimation as well.

nisse-webrtc

On 2016/02/02 19:29:00, pthatcher1 wrote: > I think the need for the sink to have ...

4 years, 10 months ago (2016-02-03 08:20:55 UTC) #9

On 2016/02/02 19:29:00, pthatcher1 wrote:
> I think the need for the sink to have a reference to the source kills options
#1
> and #2.

I agree it's a disadvantage, but I'm not sure it's a show stopper. *Someone*
must keep a reference to the source to be able to call RemoveSink. If that's the
sink itself, it already has that reference. And if it's some other object, it
may be reasonable to let that other object be in charge of capability changes
too.

> Here's an alternative version of #3 that might work, call it option #4:
> 
> Instead of having capabilities() and SignalCapabilitiesChanged, we have:
> 
> 
> virtual FrameResult OnFrame(VideoFrameT frame, FrameResultDetails* details);
> 
> enum FrameResult {
>   FRAME_CONSUMED
>   FRAME_EXCEEDS_CAPABILITIES
> }
> 
> struct FrameResultDetails {
>   bool can_apply_rotation;
> }

Do make this practical, the immediate receiver has to know what to return.
Earlier, we agreed that it is desirable that OnFrame is a "fire-and-forget"
call. On a more practical level, in case the sink needs a thread jump or the
like, it seems undesirable to have the OnFrame method wait on the other thread
to do its work before returning. Of course, this drawback applies also to option
#3 (except that for #3, OnFrame doesn't need to know the details).

> If the source gets back FRAME_EXCEEDS_CAPABILITIES, it knows to check the
> capabilities and re-send the frame and future frames in a different way
> (rotated).

Do we really need to re-send the current frame? Then a mux would need to compute
the new combined capabilities, return EXCEEDS_CAPABILITIES upstream, hope for a
resend from upstream, and remember which of its downstream sinks the re-send
should be directed to. It would be nice if we could get away without that
complexity.

> I don't want to get too generic with things like "properties".  But perhaps
> calling it something more like "desired frames" or "requests" might make more
> sense than just "capabilities".

What about "constraints"? It's used elsewhere in webrtc, but I admit I don't
quite understand how it is used.

> My question is:  is VideoTrack the only place where want one of these?

The capturer (maybe as a temporary hack?) seems to need a list of sinks, but it
isn't a mux, since it isn't a sink itself.

> I think option #4 might be: 
> implementing muxing in VideoTrack and not anywhere else.  Which may be is a
> limited version of #3.

To me, it makes a lot of sense to start like that. If/when we find a need for
additional muxes, it's no big deal to create a new class (used as base class or
delegate) and move the code from VideoTrack to the new class.

> Yes, I think so.  This is similar to frame decimation, except for a very long
> time.  The problem is that if we don't have a signal for "I'm ready for a
frame
> now", then how does the source know the sink is ready for another frame
without
> attempting to give it a frame or polling capabilities()?

Maybe it's simpler to just unregister and reregister in this case. For muting,
it's perhaps more useful to say "I don't need more than 2 fps" anyway.

> In fact, a SignalFrameNeeded is something we need to fix a bug we have where
the
> source doesn't output frames for a long time (a screencast where nothing
> changes).

Now I'm guessing about things in the code I haven't looked at at all, but
wouldn't it be simpler to say that the screencast ought to produce a minimum of
1 or 2 fps? Unchanged frames should compress well.

nisse-webrtc

I think this is great progress. I have a couple of comments, we can discuss ...

4 years, 10 months ago (2016-02-03 09:16:34 UTC) #10

pthatcher1

On 2016/02/03 08:20:55, nisse-webrtc wrote: > On 2016/02/02 19:29:00, pthatcher1 wrote: > > I think ...

4 years, 10 months ago (2016-02-03 14:52:41 UTC) #11

On 2016/02/03 08:20:55, nisse-webrtc wrote:
> On 2016/02/02 19:29:00, pthatcher1 wrote:
> > I think the need for the sink to have a reference to the source kills
options
> #1
> > and #2.
> 
> I agree it's a disadvantage, but I'm not sure it's a show stopper. *Someone*
> must keep a reference to the source to be able to call RemoveSink. If that's
the
> sink itself, it already has that reference. And if it's some other object, it
> may be reasonable to let that other object be in charge of capability changes
> too.

The way we handle this in many other places in the WebRTC code base is to have a
SignalDestroyed.  In this case, the sink would fire SignalDestroyed when it's
destroyed, and the source would listen to that and remove it.  That way, the
sink does not have a reference to the source, except for an indirect one through
the signal, which is a common pattern in our code.

Having direct references between sink and source still seems very wrong to me. 
We have to pick one to be in control and let it move the other one around. 
Historically, that's been the sink being in control of the source (capturer) and
the capturer only knows about the sink indirectly via a signal.  Now we're
flipping it around, which I think is good.  But I don't think we can have it
both ways.

> 
> > Here's an alternative version of #3 that might work, call it option #4:
> > 
> > Instead of having capabilities() and SignalCapabilitiesChanged, we have:
> > 
> > 
> > virtual FrameResult OnFrame(VideoFrameT frame, FrameResultDetails* details);
> > 
> > enum FrameResult {
> >   FRAME_CONSUMED
> >   FRAME_EXCEEDS_CAPABILITIES
> > }
> > 
> > struct FrameResultDetails {
> >   bool can_apply_rotation;
> > }
> 
> Do make this practical, the immediate receiver has to know what to return.
> Earlier, we agreed that it is desirable that OnFrame is a "fire-and-forget"
> call. On a more practical level, in case the sink needs a thread jump or the
> like, it seems undesirable to have the OnFrame method wait on the other thread
> to do its work before returning. Of course, this drawback applies also to
option
> #3 (except that for #3, OnFrame doesn't need to know the details).
>   
> > If the source gets back FRAME_EXCEEDS_CAPABILITIES, it knows to check the
> > capabilities and re-send the frame and future frames in a different way
> > (rotated).
> 
> Do we really need to re-send the current frame? Then a mux would need to
compute
> the new combined capabilities, return EXCEEDS_CAPABILITIES upstream, hope for
a
> resend from upstream, and remember which of its downstream sinks the re-send
> should be directed to. It would be nice if we could get away without that
> complexity.

Yes, that's a good point.  Fire-and-forget OnFrame is very desireable, and
merging return values would be difficult for an aggregate source.  Let's give up
on this design :).

> 
> > I don't want to get too generic with things like "properties".  But perhaps
> > calling it something more like "desired frames" or "requests" might make
more
> > sense than just "capabilities".
> 
> What about "constraints"? It's used elsewhere in webrtc, but I admit I don't
> quite understand how it is used.

"Constraints" are a very complex part of getUserMedia.  I think we should avoid
using that term for anything else.  It will just lead to lots of confusion.

> 
> > My question is:  is VideoTrack the only place where want one of these?
> 
> The capturer (maybe as a temporary hack?) seems to need a list of sinks, but
it
> isn't a mux, since it isn't a sink itself.
> 
> > I think option #4 might be: 
> > implementing muxing in VideoTrack and not anywhere else.  Which may be is a
> > limited version of #3.
> 
> To me, it makes a lot of sense to start like that. If/when we find a need for
> additional muxes, it's no big deal to create a new class (used as base class
or
> delegate) and move the code from VideoTrack to the new class.
> 

Actually, yesterday I was reading how the AudioTrack and AudioSource and
AudioSinks work, and it makes sense to me now:

- AudioTrack has an AudioSourceInterface
- AudioSource is an AudioSinkInterface and an AudioSouceInterface
- AudioSource muxes many AudioSinkInterfaces.

In other words, AudioSource is what I thought AudioTrack would be (the one
muxer), and AudioTrack uses one of those.  That makes a lot of sense to me now
that I see it.  I think we should do exactly the same thing for video.  It's
more like a mix of your #2 and #3 than my #4.

> > Yes, I think so.  This is similar to frame decimation, except for a very
long
> > time.  The problem is that if we don't have a signal for "I'm ready for a
> frame
> > now", then how does the source know the sink is ready for another frame
> without
> > attempting to give it a frame or polling capabilities()?
> 
> Maybe it's simpler to just unregister and reregister in this case. For muting,
> it's perhaps more useful to say "I don't need more than 2 fps" anyway.
>  

Muting == 0 fps makes a lot of sense, and maybe so does having the source do the
decimation.  Although, a muxer will want to do different decimation for each
sink. 

I still don't like the idea of unregister+register.  The point in time and the
place in code where you know to register may be far away (in time and space)
from where you know the capabilities (or muted state or desired fps) are.  They
just don't seem to go well together.  I still think a SignalCapabilitiesChanged
or SignalDesiresChanged makes more sense.  It would go well with
SignalDestroyed, too.  

> > In fact, a SignalFrameNeeded is something we need to fix a bug we have where
> the
> > source doesn't output frames for a long time (a screencast where nothing
> > changes).
> 
> Now I'm guessing about things in the code I haven't looked at at all, but
> wouldn't it be simpler to say that the screencast ought to produce a minimum
of
> 1 or 2 fps? Unchanged frames should compress well.

That's possible, but there are two downsides:

1.  You have to go fix all the sources, and make sure all new sources know to do
this (I'm sure it will be easy for future ones to forget, like ones that read
from a canvas or something)
2.  There's still an extra 0.5-1 second delay when you see something.

If we already need a SignalDesiresChanged and SignalDestroyed anyway, then
putting in a SignalFrameNeeded or capabilities.needs_frame_now or equivalent
would fit in nicely.

perkj_webrtc

On 2016/02/03 14:52:41, pthatcher1 wrote: > On 2016/02/03 08:20:55, nisse-webrtc wrote: > > On 2016/02/02 ...

4 years, 10 months ago (2016-02-03 15:31:51 UTC) #12

On 2016/02/03 14:52:41, pthatcher1 wrote:
> On 2016/02/03 08:20:55, nisse-webrtc wrote:
> > On 2016/02/02 19:29:00, pthatcher1 wrote:
> > > I think the need for the sink to have a reference to the source kills
> options
> > #1
> > > and #2.
> > 
> > I agree it's a disadvantage, but I'm not sure it's a show stopper. *Someone*
> > must keep a reference to the source to be able to call RemoveSink. If that's
> the
> > sink itself, it already has that reference. And if it's some other object,
it
> > may be reasonable to let that other object be in charge of capability
changes
> > too.
> 
> The way we handle this in many other places in the WebRTC code base is to have
a
> SignalDestroyed.  In this case, the sink would fire SignalDestroyed when it's
> destroyed, and the source would listen to that and remove it.  That way, the
> sink does not have a reference to the source, except for an indirect one
through
> the signal, which is a common pattern in our code.
> 
> Having direct references between sink and source still seems very wrong to me.

> We have to pick one to be in control and let it move the other one around. 
> Historically, that's been the sink being in control of the source (capturer)
and
> the capturer only knows about the sink indirectly via a signal.  Now we're
> flipping it around, which I think is good.  But I don't think we can have it
> both ways.
> 
> > 
> > > Here's an alternative version of #3 that might work, call it option #4:
> > > 
> > > Instead of having capabilities() and SignalCapabilitiesChanged, we have:
> > > 
> > > 
> > > virtual FrameResult OnFrame(VideoFrameT frame, FrameResultDetails*
details);
> > > 
> > > enum FrameResult {
> > >   FRAME_CONSUMED
> > >   FRAME_EXCEEDS_CAPABILITIES
> > > }
> > > 
> > > struct FrameResultDetails {
> > >   bool can_apply_rotation;
> > > }
> > 
> > Do make this practical, the immediate receiver has to know what to return.
> > Earlier, we agreed that it is desirable that OnFrame is a "fire-and-forget"
> > call. On a more practical level, in case the sink needs a thread jump or the
> > like, it seems undesirable to have the OnFrame method wait on the other
thread
> > to do its work before returning. Of course, this drawback applies also to
> option
> > #3 (except that for #3, OnFrame doesn't need to know the details).
> >   
> > > If the source gets back FRAME_EXCEEDS_CAPABILITIES, it knows to check the
> > > capabilities and re-send the frame and future frames in a different way
> > > (rotated).
> > 
> > Do we really need to re-send the current frame? Then a mux would need to
> compute
> > the new combined capabilities, return EXCEEDS_CAPABILITIES upstream, hope
for
> a
> > resend from upstream, and remember which of its downstream sinks the re-send
> > should be directed to. It would be nice if we could get away without that
> > complexity.
> 
> Yes, that's a good point.  Fire-and-forget OnFrame is very desireable, and
> merging return values would be difficult for an aggregate source.  Let's give
up
> on this design :).
> 
> > 
> > > I don't want to get too generic with things like "properties".  But
perhaps
> > > calling it something more like "desired frames" or "requests" might make
> more
> > > sense than just "capabilities".
> > 
> > What about "constraints"? It's used elsewhere in webrtc, but I admit I don't
> > quite understand how it is used.
> 
> "Constraints" are a very complex part of getUserMedia.  I think we should
avoid
> using that term for anything else.  It will just lead to lots of confusion.
> 
> > 
> > > My question is:  is VideoTrack the only place where want one of these?
> > 
> > The capturer (maybe as a temporary hack?) seems to need a list of sinks, but
> it
> > isn't a mux, since it isn't a sink itself.
> > 
> > > I think option #4 might be: 
> > > implementing muxing in VideoTrack and not anywhere else.  Which may be is
a
> > > limited version of #3.
> > 
> > To me, it makes a lot of sense to start like that. If/when we find a need
for
> > additional muxes, it's no big deal to create a new class (used as base class
> or
> > delegate) and move the code from VideoTrack to the new class.
> > 
> 
> Actually, yesterday I was reading how the AudioTrack and AudioSource and
> AudioSinks work, and it makes sense to me now:
> 
> - AudioTrack has an AudioSourceInterface
> - AudioSource is an AudioSinkInterface and an AudioSouceInterface
> - AudioSource muxes many AudioSinkInterfaces.
> 

It make sense - but then we need to keep the mute logic in webrtcvideoengine2 or
let change the source be able to produce black frames for only those sinks
(VideoTracks) that are disabled.
I think audio handles disabled tracks in the engine? (
provider_->SetAudioSend(ssrc_, track_->enabled(), options, renderer);



> In other words, AudioSource is what I thought AudioTrack would be (the one
> muxer), and AudioTrack uses one of those.  That makes a lot of sense to me now
> that I see it.  I think we should do exactly the same thing for video.  It's
> more like a mix of your #2 and #3 than my #4.
> 
> > > Yes, I think so.  This is similar to frame decimation, except for a very
> long
> > > time.  The problem is that if we don't have a signal for "I'm ready for a
> > frame
> > > now", then how does the source know the sink is ready for another frame
> > without
> > > attempting to give it a frame or polling capabilities()?
> > 
> > Maybe it's simpler to just unregister and reregister in this case. For
muting,
> > it's perhaps more useful to say "I don't need more than 2 fps" anyway.
> >  
> 
> Muting == 0 fps makes a lot of sense, and maybe so does having the source do
the
> decimation.  Although, a muxer will want to do different decimation for each
> sink. 
> 
> I still don't like the idea of unregister+register.  The point in time and the
> place in code where you know to register may be far away (in time and space)
> from where you know the capabilities (or muted state or desired fps) are. 
They
> just don't seem to go well together.  I still think a
SignalCapabilitiesChanged
> or SignalDesiresChanged makes more sense.  It would go well with
> SignalDestroyed, too.  
> 
> > > In fact, a SignalFrameNeeded is something we need to fix a bug we have
where
> > the
> > > source doesn't output frames for a long time (a screencast where nothing
> > > changes).
> > 
> > Now I'm guessing about things in the code I haven't looked at at all, but
> > wouldn't it be simpler to say that the screencast ought to produce a minimum
> of
> > 1 or 2 fps? Unchanged frames should compress well.
> 
> That's possible, but there are two downsides:
> 
> 1.  You have to go fix all the sources, and make sure all new sources know to
do
> this (I'm sure it will be easy for future ones to forget, like ones that read
> from a canvas or something)
> 2.  There's still an extra 0.5-1 second delay when you see something.
> 
> If we already need a SignalDesiresChanged and SignalDestroyed anyway, then
> putting in a SignalFrameNeeded or capabilities.needs_frame_now or equivalent
> would fit in nicely.

Would you be ok with avoiding the Signal in the interface by using a Callback?
 
template <typename VideoFrameT>
class VideoSinkInterface {
 public:
  virtual void OnFrame(const VideoFrameT& frame) = 0;
  virtual VideoSinkHints GetHints() const { return VideoSinkHints(); }
 protected:
  virtual ~VideoSinkInterface() {}
};



template <typename VideoFrameT>
class VideoSourceInterface {
 public:
  using SinkHintsChangedCB = Callback0<void>;
  virtual SinkHintsChangedCB AddSink(VideoSinkInterface<VideoFrameT>* sink) = 0;
  virtual void RemoveSink(VideoSinkInterface<VideoFrameT>* sink) = 0;

 protected:
  virtual ~VideoSourceInterface() {}
};

SinkHintsChangedCB can be empty so you don't need to check if your sink is
registered if you have it as a class member in a sink implementation. 
See the use in the latest patchset.

pthatcher1

pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org

4 years, 10 months ago (2016-02-03 15:38:35 UTC) #13

pthatcher1

It's looking good. The only thing I don't like is the need to remove and ...

4 years, 10 months ago (2016-02-03 15:38:36 UTC) #14

It's looking good.

The only thing I don't like is the need to remove and readd a sink in order to
change capabilities.  I still think having a capabilities() method and a
SignalCapabilitiesChanged would be be more clear.  

If you want to try the route where we don't have a SIgnalCapabilitiesChanged,
but instead call capabilities() before every call to OnFrame, I think that could
work also.  But I don't like the model where the sink has to manually tell the
source that its capabilities changed via two separate method calls (add and
remove).  Even an explicit call to Source::OnCapabilitiesChanged would be an
improvement over that.

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/capturere...
File talk/media/base/capturerenderadapter.h (right):

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/capturere...
talk/media/base/capturerenderadapter.h:48: class CaptureRenderAdapter : public
rtc::VideoSinkInterface<VideoFrame> {
On 2016/02/03 09:16:34, nisse-webrtc wrote:
> After these changes, it looks like this class is 
> 
> 1. Functionally equivalent to a VideoTrack without the muting logic. I.e, a
> plain mux.
> 
> 2. Redundant, any code calling AddSink on this object could just as well call
> AddSink directly on the VideoCapturer.
> 
> Correct?

I'd like it if we can just remove this class altogether.   It's only used in one
place:  VideoCapturerState, which only uses it for 2 things:

1.  To help implement CaptureManager::AddVideoSink and
CaptureManager::RemoveVideoSink
2.  To store a video capturer to be used in
CaptureManager::UnregisterVideoCapturer.

For #1, it could probably use our VideoMux (whatever we end up calling it)
instead of CaptureRendererAdapter.  Our we could go one step further: 
CaptureManager::AddVideoSink and CaptureManager::RemoveVideoSink are only used
to implement VideoSource::AddSink and VideoSource::RemoveSink, so maybe we could
just skip using the CaptureManager at all for this.

For #2, the VideoCapturerState could just contain the VideoCapturer directly
instead of having the CaptureRendererAdapter do it.

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videocapt...
File talk/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videocapt...
talk/media/base/videocapturer.cc:248: apply_rotation_ =
!capabilities.can_apply_rotation;
I like this a lot.

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videocapt...
talk/media/base/videocapturer.cc:351: if (!HasSinks()) {
We could make this more generic as something like FrameWorthDelivering().  Then,
in the future when all the sinks are muted in some way (have maxFps = 0), then
we wouldn't produce a frame.

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videocapt...
File talk/media/base/videocapturer.h (right):

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videocapt...
talk/media/base/videocapturer.h:132: public rtc::VideoSourceBase {
Can we try making this "has a muxer" rather than "is a muxer"?  In other words,
instead of inheriting from VideoSourceBase, containing a VideoBrodacaster (or
whatever we call it).

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videosour...
File talk/media/base/videosourcebase.cc (right):

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videosour...
talk/media/base/videosourcebase.cc:38: RTC_DCHECK(sinks_.find(sink) ==
sinks_.end());
On 2016/02/03 09:16:34, nisse-webrtc wrote:
> This seems pointless when using a set.

And even if we change to a vector, it should probably just return early (no-op).

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videosour...
File talk/media/base/videosourcebase.h (right):

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videosour...
talk/media/base/videosourcebase.h:39: class VideoSourceBase : public
VideoSourceInterface<cricket::VideoFrame> {
Can we call this thing something more explicit, like VideoBroadcaster?

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videosour...
talk/media/base/videosourcebase.h:42: const VideoSinkCapabilities& capabilities)
override;
Why does AddSink take the capabilities instead of calling sink->capabilities()?

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videosour...
talk/media/base/videosourcebase.h:47: void DeliverFrameToSinks(const
cricket::VideoFrame& frame);
On 2016/02/03 09:16:34, nisse-webrtc wrote:
> Is there any obvious drawback to rename this OnFrame, and make the class also
> inherit VideoSinkInterface?

Yes, I was thinking the same thing.  I think that makes sense.  

Of course, then you have to implement capability merging (assuming we stick with
that design), but I think that makes sense as well.

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videosour...
talk/media/base/videosourcebase.h:56: CriticalSection sink_lock_;
On 2016/02/03 09:16:34, nisse-webrtc wrote:
> I guess this makes sense, if used as a base class. If instead used as a
member,
> I think it would make more sense to have the using class be responsible for
> locking.

As mentioned earlier, I'd like to see us try making it a member instead of a
base class.

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/base/videosour...
talk/media/base/videosourcebase.h:58:
std::unordered_set<VideoSinkInterface<cricket::VideoFrame>*> sinks_;
On 2016/02/03 09:16:34, nisse-webrtc wrote:
> I think Tommi objected to using std::set earlier, since it implies unneeded
> overhead. If we're not expecting more than a dozen sinks, std::vector or
> std::list should be good enough, right?

Throughout our code base, we almost alway use std::vector.  That's what I'd
prefer here.

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1701: capturer_->AddSink(this,
sink_capabilities_);
I think it would be better to add capabilities() to the VideoSinkInterface and
then have this implementation implement it (instead of storing it) and then call
capturer_->AddSink(this) without the capabilities passed in.

https://codereview.webrtc.org/1655793003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1896: capturer_->AddSink(this,
sink_capabilities_);
Why not just fire SignalCapabilitiesChanged()?

https://codereview.webrtc.org/1655793003/diff/60001/webrtc/media/base/videosi...
File webrtc/media/base/videosinkinterface.h (right):

https://codereview.webrtc.org/1655793003/diff/60001/webrtc/media/base/videosi...
webrtc/media/base/videosinkinterface.h:19: }
Why not use operator== ?

pthatcher1

> > It make sense - but then we need to keep the mute logic ...

4 years, 10 months ago (2016-02-03 16:06:20 UTC) #15

> 
> It make sense - but then we need to keep the mute logic in webrtcvideoengine2
or
> let change the source be able to produce black frames for only those sinks
> (VideoTracks) that are disabled.
> I think audio handles disabled tracks in the engine? (
> provider_->SetAudioSend(ssrc_, track_->enabled(), options, renderer);

That brings up a good point: there are actually two different meanings of
"muted", and in previous emails I was getting them confused:
A.  The sink says "I can't use a frame rate now anyway, so don't bother sending
them".  That's equivalent to fps == 0.
B.  The sink says "I want frames, but I want them all black.  They don't need to
be 30fps, but they do need to be > 0 fps".  

"muted" in webrtcvideoengine2 is type B. 
"muted" for a renderer that isn't on screen at the moment is type A.

Type A muted makes sense to implement in a common place
(VideoMuxer/VideoBroadcaster/VideoWhateverWeCallIt would make sense).  
Type B I could see us only having in the webrtcvideoengine2.   Basically, it
could do this:

1.  Acts as a Type A muted sink (it says "I don't want any frames).
2.  It generates black frames at Nfps (1fps?) and injects those into itself.  In
fact, it could do this by creating a BlackFrameSource and then hooking itself up
to that whenever it decided it wanted black frames.  

The other option is to have the VideoBroadcaster implement the "produce black
frames" logic.  But then a VideoBroadcaster would need to have a
VideoFrameFactory for black frames, and that would probably be getting to
complex for something that is really only needed in one place.

What do you think?




> 
> 
> > In other words, AudioSource is what I thought AudioTrack would be (the one
> > muxer), and AudioTrack uses one of those.  That makes a lot of sense to me
now
> > that I see it.  I think we should do exactly the same thing for video.  It's
> > more like a mix of your #2 and #3 than my #4.
> > 
> > > > Yes, I think so.  This is similar to frame decimation, except for a very
> > long
> > > > time.  The problem is that if we don't have a signal for "I'm ready for
a
> > > frame
> > > > now", then how does the source know the sink is ready for another frame
> > > without
> > > > attempting to give it a frame or polling capabilities()?
> > > 
> > > Maybe it's simpler to just unregister and reregister in this case. For
> muting,
> > > it's perhaps more useful to say "I don't need more than 2 fps" anyway.
> > >  
> > 
> > Muting == 0 fps makes a lot of sense, and maybe so does having the source do
> the
> > decimation.  Although, a muxer will want to do different decimation for each
> > sink. 
> > 
> > I still don't like the idea of unregister+register.  The point in time and
the
> > place in code where you know to register may be far away (in time and space)
> > from where you know the capabilities (or muted state or desired fps) are. 
> They
> > just don't seem to go well together.  I still think a
> SignalCapabilitiesChanged
> > or SignalDesiresChanged makes more sense.  It would go well with
> > SignalDestroyed, too.  
> > 
> > > > In fact, a SignalFrameNeeded is something we need to fix a bug we have
> where
> > > the
> > > > source doesn't output frames for a long time (a screencast where nothing
> > > > changes).
> > > 
> > > Now I'm guessing about things in the code I haven't looked at at all, but
> > > wouldn't it be simpler to say that the screencast ought to produce a
minimum
> > of
> > > 1 or 2 fps? Unchanged frames should compress well.
> > 
> > That's possible, but there are two downsides:
> > 
> > 1.  You have to go fix all the sources, and make sure all new sources know
to
> do
> > this (I'm sure it will be easy for future ones to forget, like ones that
read
> > from a canvas or something)
> > 2.  There's still an extra 0.5-1 second delay when you see something.
> > 
> > If we already need a SignalDesiresChanged and SignalDestroyed anyway, then
> > putting in a SignalFrameNeeded or capabilities.needs_frame_now or equivalent
> > would fit in nicely.
> 
> Would you be ok with avoiding the Signal in the interface by using a Callback?
>  
> template <typename VideoFrameT>
> class VideoSinkInterface {
>  public:
>   virtual void OnFrame(const VideoFrameT& frame) = 0;
>   virtual VideoSinkHints GetHints() const { return VideoSinkHints(); }
>  protected:
>   virtual ~VideoSinkInterface() {}
> };
> 

So far all this does is rename "capabilities" to "hints", right? Oh, I guess you
added the reader, which is good.

By the way, there are lots of words we could use for this: wants, needs,
desires, capabilities, properties, hints, requests.  Currently, I'm thinking:
,
wants > needs > capabilities > desires > requests > hints > properties

> 
> 
> template <typename VideoFrameT>
> class VideoSourceInterface {
>  public:
>   using SinkHintsChangedCB = Callback0<void>;
>   virtual SinkHintsChangedCB AddSink(VideoSinkInterface<VideoFrameT>* sink) =
0;
>   virtual void RemoveSink(VideoSinkInterface<VideoFrameT>* sink) = 0;
> 
>  protected:
>   virtual ~VideoSourceInterface() {}
> };
> 
> SinkHintsChangedCB can be empty so you don't need to check if your sink is
> registered if you have it as a class member in a sink implementation. 
> See the use in the latest patchset.

And this is just adding a return value to AddSink which is "fire this when you
change your capabilities"?

So the use of it would be something like this?

VideoSinkInterface<T>* sink = ...;  // Gotten somehow
VideoSouceInterface<T>* source = ... // Gotten somehow
auto signal_capabilities_changed() = souce->AddSink(sink);
...
signal_capabilities_changed()


So if I have a sink from somewhere, like from
WebRtcVideoMediaChannel2::GetSink(ssrc), and then I also have a source from
somewhere, like from RtpSender::SetTrack, and then I combine them, what do I do
with the CB returned?  How do I know when the sink returned from GetSink()
changes its capabilities?  Do I somehow have to pass the CB into
WebRtcVideoMediaChannel2 so that it can call it when header extensions change? 
This doesn't seem much better than having an explicit
VideoSourceInterface::OnSinkCapabilitiesChanged(), which has the same problem. 
The crux of the problem is:

1.  WebRtcVideoMediaChannel2 knows when the capabilities changed.
2.  The caller of WebRtcVideoMediaChannel2::GetSink(ssrc) doesn't.
3.  The VideoSourceInterface needs to know when the capabilities changed.

Somehow the WebRtcVideoMediaChannel2 needs to tell the VideoSourceInterface.  We
currently give a source to WebRtcVideoMediaChannel2::SetCapturer, but we're
moving away from that.  

The only viable solutions I see are:

A.  The sink returned from WebRtcVideoMediaChannel2::GetSink(ssrc) fires a
signal 
B.  The source reads VideoSinkInterface::capabilities() before every call to
OnFrame.

I'm worried about the performance of B, and I think A makes sense.

I really don't understand the objection to SignalCapabilitiesChanged.  Could you
please explain it again to me?

perkj_webrtc

On 2016/02/03 16:06:20, pthatcher1 wrote: > > > > It make sense - but then ...

4 years, 10 months ago (2016-02-03 16:29:09 UTC) #16

On 2016/02/03 16:06:20, pthatcher1 wrote:
> > 
> > It make sense - but then we need to keep the mute logic in
webrtcvideoengine2
> or
> > let change the source be able to produce black frames for only those sinks
> > (VideoTracks) that are disabled.
> > I think audio handles disabled tracks in the engine? (
> > provider_->SetAudioSend(ssrc_, track_->enabled(), options, renderer);
> 
> That brings up a good point: there are actually two different meanings of
> "muted", and in previous emails I was getting them confused:
> A.  The sink says "I can't use a frame rate now anyway, so don't bother
sending
> them".  That's equivalent to fps == 0.
> B.  The sink says "I want frames, but I want them all black.  They don't need
to
> be 30fps, but they do need to be > 0 fps".  
> 
> "muted" in webrtcvideoengine2 is type B. 
> "muted" for a renderer that isn't on screen at the moment is type A.
> 
> Type A muted makes sense to implement in a common place
> (VideoMuxer/VideoBroadcaster/VideoWhateverWeCallIt would make sense).  
> Type B I could see us only having in the webrtcvideoengine2.   Basically, it
> could do this:
> 
> 1.  Acts as a Type A muted sink (it says "I don't want any frames).
> 2.  It generates black frames at Nfps (1fps?) and injects those into itself. 
In
> fact, it could do this by creating a BlackFrameSource and then hooking itself
up
> to that whenever it decided it wanted black frames.  
> 
> The other option is to have the VideoBroadcaster implement the "produce black
> frames" logic.  But then a VideoBroadcaster would need to have a
> VideoFrameFactory for black frames, and that would probably be getting to
> complex for something that is really only needed in one place.
> 
> What do you think?
> 
> 
> 
> 
> > 
> > 
> > > In other words, AudioSource is what I thought AudioTrack would be (the one
> > > muxer), and AudioTrack uses one of those.  That makes a lot of sense to me
> now
> > > that I see it.  I think we should do exactly the same thing for video. 
It's
> > > more like a mix of your #2 and #3 than my #4.
> > > 
> > > > > Yes, I think so.  This is similar to frame decimation, except for a
very
> > > long
> > > > > time.  The problem is that if we don't have a signal for "I'm ready
for
> a
> > > > frame
> > > > > now", then how does the source know the sink is ready for another
frame
> > > > without
> > > > > attempting to give it a frame or polling capabilities()?
> > > > 
> > > > Maybe it's simpler to just unregister and reregister in this case. For
> > muting,
> > > > it's perhaps more useful to say "I don't need more than 2 fps" anyway.
> > > >  
> > > 
> > > Muting == 0 fps makes a lot of sense, and maybe so does having the source
do
> > the
> > > decimation.  Although, a muxer will want to do different decimation for
each
> > > sink. 
> > > 
> > > I still don't like the idea of unregister+register.  The point in time and
> the
> > > place in code where you know to register may be far away (in time and
space)
> > > from where you know the capabilities (or muted state or desired fps) are. 
> > They
> > > just don't seem to go well together.  I still think a
> > SignalCapabilitiesChanged
> > > or SignalDesiresChanged makes more sense.  It would go well with
> > > SignalDestroyed, too.  
> > > 
> > > > > In fact, a SignalFrameNeeded is something we need to fix a bug we have
> > where
> > > > the
> > > > > source doesn't output frames for a long time (a screencast where
nothing
> > > > > changes).
> > > > 
> > > > Now I'm guessing about things in the code I haven't looked at at all,
but
> > > > wouldn't it be simpler to say that the screencast ought to produce a
> minimum
> > > of
> > > > 1 or 2 fps? Unchanged frames should compress well.
> > > 
> > > That's possible, but there are two downsides:
> > > 
> > > 1.  You have to go fix all the sources, and make sure all new sources know
> to
> > do
> > > this (I'm sure it will be easy for future ones to forget, like ones that
> read
> > > from a canvas or something)
> > > 2.  There's still an extra 0.5-1 second delay when you see something.
> > > 
> > > If we already need a SignalDesiresChanged and SignalDestroyed anyway, then
> > > putting in a SignalFrameNeeded or capabilities.needs_frame_now or
equivalent
> > > would fit in nicely.
> > 
> > Would you be ok with avoiding the Signal in the interface by using a
Callback?
> >  
> > template <typename VideoFrameT>
> > class VideoSinkInterface {
> >  public:
> >   virtual void OnFrame(const VideoFrameT& frame) = 0;
> >   virtual VideoSinkHints GetHints() const { return VideoSinkHints(); }
> >  protected:
> >   virtual ~VideoSinkInterface() {}
> > };
> > 
> 
> So far all this does is rename "capabilities" to "hints", right? Oh, I guess
you
> added the reader, which is good.
> 
> By the way, there are lots of words we could use for this: wants, needs,
> desires, capabilities, properties, hints, requests.  Currently, I'm thinking:
> ,
> wants > needs > capabilities > desires > requests > hints > properties
> 
> > 
> > 
> > template <typename VideoFrameT>
> > class VideoSourceInterface {
> >  public:
> >   using SinkHintsChangedCB = Callback0<void>;
> >   virtual SinkHintsChangedCB AddSink(VideoSinkInterface<VideoFrameT>* sink)
=
> 0;
> >   virtual void RemoveSink(VideoSinkInterface<VideoFrameT>* sink) = 0;
> > 
> >  protected:
> >   virtual ~VideoSourceInterface() {}
> > };
> > 
> > SinkHintsChangedCB can be empty so you don't need to check if your sink is
> > registered if you have it as a class member in a sink implementation. 
> > See the use in the latest patchset.
> 
> And this is just adding a return value to AddSink which is "fire this when you
> change your capabilities"?
> 
> So the use of it would be something like this?
> 
> VideoSinkInterface<T>* sink = ...;  // Gotten somehow
> VideoSouceInterface<T>* source = ... // Gotten somehow
> auto signal_capabilities_changed() = souce->AddSink(sink);
> ...
> signal_capabilities_changed()
> 
> 
> So if I have a sink from somewhere, like from
> WebRtcVideoMediaChannel2::GetSink(ssrc), and then I also have a source from
> somewhere, like from RtpSender::SetTrack, and then I combine them, what do I
do
> with the CB returned?  How do I know when the sink returned from GetSink()
> changes its capabilities?  Do I somehow have to pass the CB into
> WebRtcVideoMediaChannel2 so that it can call it when header extensions change?

> This doesn't seem much better than having an explicit
> VideoSourceInterface::OnSinkCapabilitiesChanged(), which has the same problem.

> The crux of the problem is:
> 
> 1.  WebRtcVideoMediaChannel2 knows when the capabilities changed.
> 2.  The caller of WebRtcVideoMediaChannel2::GetSink(ssrc) doesn't.
> 3.  The VideoSourceInterface needs to know when the capabilities changed.
> 
> Somehow the WebRtcVideoMediaChannel2 needs to tell the VideoSourceInterface. 
We
> currently give a source to WebRtcVideoMediaChannel2::SetCapturer, but we're
> moving away from that.  
> 
> The only viable solutions I see are:
> 
> A.  The sink returned from WebRtcVideoMediaChannel2::GetSink(ssrc) fires a
> signal 
> B.  The source reads VideoSinkInterface::capabilities() before every call to
> OnFrame.
> 
> I'm worried about the performance of B, and I think A makes sense.
> 
> I really don't understand the objection to SignalCapabilitiesChanged.  Could
you
> please explain it again to me?

... Too bad...... - but why do we need to move away from
WebRtcVideoMediaChannel2::SendStream::SetCapturer? or SetSource and replace it
with GetSink? 
If the sink registers with the source you don't have the problem you describe.
The SendStream knows when the capabilities change and it has the callback
object. But yes, it is pretty much the same thing as calling a member function
on the Source except that you don't need to worry about if the sink has been
registered with the source or not yet.  

If we use a Signal in the interface I worry that it will be hard to implement
the SinkInterface in Chrome. There has been a lot of opinions about the use of
Signals and sigslots. Most vocal has been tommi and pbos - let me see if I can
get more help of specifying the problem. I know that thread safety and binary
size has been two issues mentioned.

pthatcher1

> > I really don't understand the objection to SignalCapabilitiesChanged. Could > you > > ...

4 years, 10 months ago (2016-02-03 17:33:56 UTC) #17

> > I really don't understand the objection to SignalCapabilitiesChanged.  Could
> you
> > please explain it again to me?
> 
> ... Too bad...... - but why do we need to move away from
> WebRtcVideoMediaChannel2::SendStream::SetCapturer? or SetSource and replace it
> with GetSink? 
> If the sink registers with the source you don't have the problem you describe.
>
> The SendStream knows when the capabilities change and it has the callback
> object. But yes, it is pretty much the same thing as calling a member function
> on the Source except that you don't need to worry about if the sink has been
> registered with the source or not yet.  


But that just kicks the can down the road.  The
WebRtcVideoMediaChannel2::SendStream isn't the end of the pipeline either,
because it will go further down, and if the capabilities of the thing further
down changes, then how will the WebRtcVideoMediaChannel2::SendStream know?   For
example, if the WebRtcVideoMediaChannel2::SendStream ends up passing things
further down to encoders that are also sinks, and the capabilities of those
encoders change, how will it know?

This is just one example of the fundamental problem.  I think this will come up
in other contexts also, such as local rendering.  We have sources that product
frames and sinks that have changing needs/wants/capabilities.  The sinks want to
know about the frames.  The sources want to know what the
needs/wants/capabilities are.   And we want to minimize how much the sources and
sinks know about each other.  The last part is key.  We want to minimize how
much the sources and sinks know about each other.

> 
> If we use a Signal in the interface I worry that it will be hard to implement
> the SinkInterface in Chrome. There has been a lot of opinions about the use of
> Signals and sigslots. Most vocal has been tommi and pbos - let me see if I can
> get more help of specifying the problem. I know that thread safety and binary
> size has been two issues mentioned.

We have signals in interfaces all over the place.  Take a look at
cricket::AsyncPacketSocket, which is implemented by Chrome.  It has 7 signals. 
I don't think is a reasonable short-term goal to remove the use of signals in
interfaces.  So I don't think we should be worried about binary size here,
considering that if it's a problem, it's way outside the scope here.

However, I do see your concern about thread safety.  But I don't think it's the
SignalCapabilitiesChanged that's the problem.  It's the GetCapabilities method
that's the problem.  If the handler of SignalCapabilitiesChanged uses the same
thread safety as AddSink and RemoveSink, then SignalCapabilitiesChanged can be
fired on any thread, and it's just as safe as what you have with
RemoveSink+AddSink.  So I don't think that's a problem.  But the
GetCapabilities() method is a bigger problem.  On what thread is it safe to call
*that*?  I think it would have to thread safe just like OnFrame, but it's harder
because it has a return value, whereas OnFrame is "fire and forget".  


So.... even though I just said we want to minimize how much sources and sinks
know about each other, what if tried something like this (ignoring templating
for a moment):

class VideoSourceInterface {
  // These must all be thread safe
  // AddSink should call VideoSinkInterface::SetSource(this)
  // RemoveSink should call VideoSinkInterface::ClearSource(this);
  // VideoSinkInterface::ClearSource(this) must be called when the
VideoSourceInterface is destroyed.
  void AddSink(VideoSinkInterface* sink, const VideoWants& wants);
  void RemoveSink(VideoSinkInterface* sink);
  void UpdateSink(VideoSinkInterface* sink, const VideoWants& wants);
}

class VideoSinkInterface {
  // These must be thread safe
  void OnFrame(VideoFrameT frame);
  // When the sink's wants change, it must call VideoSourceInterface::UpdateSink
  // When the sink is destroyed, it must call VideoSourceInterface::RemoveSink
  // A sink can only have one source at a time, but that's probably a good idea
anyway.
  void SetSource(VideoSourceInterface* source);
  // Clearing a source that isn't the current one is a no-op.
  void ClearSource(VideoSourceInterface* source);
}

That would be thread safe and avoid the use of signals and each method could be
implemented with a thread-hop without worrying about return values, and I don't
see a good reason why the sink can't know about the sources it's supposed to
update.    


What do you think?

pthatcher1

> > So.... even though I just said we want to minimize how much sources ...

4 years, 10 months ago (2016-02-03 17:59:12 UTC) #18

> 
> So.... even though I just said we want to minimize how much sources and sinks
> know about each other, what if tried something like this (ignoring templating
> for a moment):
> 
> class VideoSourceInterface {
>   // These must all be thread safe
>   // AddSink should call VideoSinkInterface::SetSource(this)
>   // RemoveSink should call VideoSinkInterface::ClearSource(this);
>   // VideoSinkInterface::ClearSource(this) must be called when the
> VideoSourceInterface is destroyed.
>   void AddSink(VideoSinkInterface* sink, const VideoWants& wants);
>   void RemoveSink(VideoSinkInterface* sink);
>   void UpdateSink(VideoSinkInterface* sink, const VideoWants& wants);
> }
> 
> class VideoSinkInterface {
>   // These must be thread safe
>   void OnFrame(VideoFrameT frame);
>   // When the sink's wants change, it must call
VideoSourceInterface::UpdateSink
>   // When the sink is destroyed, it must call VideoSourceInterface::RemoveSink
>   // A sink can only have one source at a time, but that's probably a good
idea
> anyway.
>   void SetSource(VideoSourceInterface* source);
>   // Clearing a source that isn't the current one is a no-op.
>   void ClearSource(VideoSourceInterface* source);
> }
> 
> That would be thread safe and avoid the use of signals and each method could
be
> implemented with a thread-hop without worrying about return values, and I
don't
> see a good reason why the sink can't know about the sources it's supposed to
> update.    
> 
> 
> What do you think?


Actually, I just thought of another tweak on this:


class VideoSourceInterface {
  // VideoSinkInterface::SetSource(this) should be called when a sink is added.
  // VideoSinkInterface::ClearSource(this) should be called when a sink is
removed.
  // VideoSinkInterface::ClearSource(this) must be called 
  // when the VideoSourceInterface is destroyed.
  // Adding a current sink is a no-op.
  void AddSink(VideoSinkInterface* sink);
  void RemoveSink(VideoSinkInterface* sink);
  void AddOrUpdateSink(VideoSinkInterface* sink, const VideoWants& wants);
}

class VideoSinkInterface {
  void OnFrame(VideoFrameT frame);
 
  // VideoSourceInterface::AddOrUpdateSink should be called 
  // when the source is changed or when the wants change.
  // VideoSourceInterface::RemoveSink should be called when 
  // the source is cleared.
  // VideoSouceInterface::RemoveSink must be called
  // when the VideoSinkInterface is destroyed.
  // Clearing a non-current source is a no-op.
  void SetSource(VideoSourceInterface* source);
  void ClearSource(VideoSourceInterface* source);
}

This would let you call either source->AddSink(sink) or sink->SetSource(source)
and everything would hookup with the latest capabilities/wants correctly.  And
it doesn't require a synchronous read of the capabilities from the source to add
the sink.  

But I did just realize one thing about thread safety:

1.  If the sink is destroyed on one thread and Source::RemoveSink does a thread
hop, but another thread is still using the now-destroyed sink, then it will
crash.  This implies RemoveSink needs to not do a thead-hop.

2.  Similarly, if a source is destroyed on one thread and Sink::ClearSource does
a thead hop, but another thread is still using the now-destroyed sink, then it
will crash.  This implies that ClearSource needs to not do a thread-hop.  


What do you think?


Of course, this is all just to avoid the synchronous capabilities() or wants()
call.  If we don't think that's a big deal, then all of this might be overkill.

perkj_webrtc

On 2016/02/03 17:59:12, pthatcher1 wrote: > > > > So.... even though I just said ...

4 years, 10 months ago (2016-02-03 21:06:57 UTC) #19

On 2016/02/03 17:59:12, pthatcher1 wrote:
> > 
> > So.... even though I just said we want to minimize how much sources and
sinks
> > know about each other, what if tried something like this (ignoring
templating
> > for a moment):
> > 
> > class VideoSourceInterface {
> >   // These must all be thread safe
> >   // AddSink should call VideoSinkInterface::SetSource(this)
> >   // RemoveSink should call VideoSinkInterface::ClearSource(this);
> >   // VideoSinkInterface::ClearSource(this) must be called when the
> > VideoSourceInterface is destroyed.
> >   void AddSink(VideoSinkInterface* sink, const VideoWants& wants);
> >   void RemoveSink(VideoSinkInterface* sink);
> >   void UpdateSink(VideoSinkInterface* sink, const VideoWants& wants);
> > }
> > 
> > class VideoSinkInterface {
> >   // These must be thread safe
> >   void OnFrame(VideoFrameT frame);
> >   // When the sink's wants change, it must call
> VideoSourceInterface::UpdateSink
> >   // When the sink is destroyed, it must call
VideoSourceInterface::RemoveSink
> >   // A sink can only have one source at a time, but that's probably a good
> idea
> > anyway.
> >   void SetSource(VideoSourceInterface* source);
> >   // Clearing a source that isn't the current one is a no-op.
> >   void ClearSource(VideoSourceInterface* source);
> > }
> > 
> > That would be thread safe and avoid the use of signals and each method could
> be
> > implemented with a thread-hop without worrying about return values, and I
> don't
> > see a good reason why the sink can't know about the sources it's supposed to
> > update.    
> > 
> > 
> > What do you think?
> 
> 
> Actually, I just thought of another tweak on this:
> 
> 
> class VideoSourceInterface {
>   // VideoSinkInterface::SetSource(this) should be called when a sink is
added.
>   // VideoSinkInterface::ClearSource(this) should be called when a sink is
> removed.
>   // VideoSinkInterface::ClearSource(this) must be called 
>   // when the VideoSourceInterface is destroyed.
>   // Adding a current sink is a no-op.
>   void AddSink(VideoSinkInterface* sink);
>   void RemoveSink(VideoSinkInterface* sink);
>   void AddOrUpdateSink(VideoSinkInterface* sink, const VideoWants& wants);
> }
> 
> class VideoSinkInterface {
>   void OnFrame(VideoFrameT frame);
>  
>   // VideoSourceInterface::AddOrUpdateSink should be called 
>   // when the source is changed or when the wants change.
>   // VideoSourceInterface::RemoveSink should be called when 
>   // the source is cleared.
>   // VideoSouceInterface::RemoveSink must be called
>   // when the VideoSinkInterface is destroyed.
>   // Clearing a non-current source is a no-op.
>   void SetSource(VideoSourceInterface* source);
>   void ClearSource(VideoSourceInterface* source);
> }
> 
> This would let you call either source->AddSink(sink) or
sink->SetSource(source)
> and everything would hookup with the latest capabilities/wants correctly.  And
> it doesn't require a synchronous read of the capabilities from the source to
add
> the sink.  
> 
> But I did just realize one thing about thread safety:
> 
> 1.  If the sink is destroyed on one thread and Source::RemoveSink does a
thread
> hop, but another thread is still using the now-destroyed sink, then it will
> crash.  This implies RemoveSink needs to not do a thead-hop.
> 
> 2.  Similarly, if a source is destroyed on one thread and Sink::ClearSource
does
> a thead hop, but another thread is still using the now-destroyed sink, then it
> will crash.  This implies that ClearSource needs to not do a thread-hop.  
> 
> 
> What do you think?
> 
> 
> Of course, this is all just to avoid the synchronous capabilities() or wants()
> call.  If we don't think that's a big deal, then all of this might be
overkill.

This seems like a good proposal to me. 

In Chrome we solved your last point on how to remove a sink thread safe by
making sure a  video frame callbacks are bound to a reference counted object.
See ex
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
and
MediaStreamVideoTrack::RemoveSink.https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media/media_stream_video_track.cc&l=258

Stripping away  chromes callbacks/bind magic and looking at it  in this context 
it  means that we 
1.  make VideoSinkInterFace ref counted.
2. VideoSourceInterFace::AddSink take  a temporary  reference to
VideoSinkinterface and post to the thread where  VideoSinkInterFace::OnFrame is
called on. On this thread - store a reference to the sink.
2. VideoSourceInterface::RemoveSink post to the thread that calls
VideoSinkInterFace::OnFrame,  here  the reference is removed and thus OnFrame
will not be called again on that sink.  Then post back to the thread where
AddSink/RemoveSink is called to make sure the last reference to the sink is
released on the correct thread.

Can we let VideoSinkinterface be reference counted? 
I think its easiest if we say that AddSink,  RemoveSink, SetSource, and
ClearSource are all called on one an only one thread. OnFrame may be called on
the same thread or another thread.
On what thread will we allow AddOrUpdateSink be called on?

pthatcher1

On 2016/02/03 21:06:57, perkj_webrtc wrote: > On 2016/02/03 17:59:12, pthatcher1 wrote: > > > > ...

4 years, 10 months ago (2016-02-03 22:23:55 UTC) #20

On 2016/02/03 21:06:57, perkj_webrtc wrote:
> On 2016/02/03 17:59:12, pthatcher1 wrote:
> > > 
> > > So.... even though I just said we want to minimize how much sources and
> sinks
> > > know about each other, what if tried something like this (ignoring
> templating
> > > for a moment):
> > > 
> > > class VideoSourceInterface {
> > >   // These must all be thread safe
> > >   // AddSink should call VideoSinkInterface::SetSource(this)
> > >   // RemoveSink should call VideoSinkInterface::ClearSource(this);
> > >   // VideoSinkInterface::ClearSource(this) must be called when the
> > > VideoSourceInterface is destroyed.
> > >   void AddSink(VideoSinkInterface* sink, const VideoWants& wants);
> > >   void RemoveSink(VideoSinkInterface* sink);
> > >   void UpdateSink(VideoSinkInterface* sink, const VideoWants& wants);
> > > }
> > > 
> > > class VideoSinkInterface {
> > >   // These must be thread safe
> > >   void OnFrame(VideoFrameT frame);
> > >   // When the sink's wants change, it must call
> > VideoSourceInterface::UpdateSink
> > >   // When the sink is destroyed, it must call
> VideoSourceInterface::RemoveSink
> > >   // A sink can only have one source at a time, but that's probably a good
> > idea
> > > anyway.
> > >   void SetSource(VideoSourceInterface* source);
> > >   // Clearing a source that isn't the current one is a no-op.
> > >   void ClearSource(VideoSourceInterface* source);
> > > }
> > > 
> > > That would be thread safe and avoid the use of signals and each method
could
> > be
> > > implemented with a thread-hop without worrying about return values, and I
> > don't
> > > see a good reason why the sink can't know about the sources it's supposed
to
> > > update.    
> > > 
> > > 
> > > What do you think?
> > 
> > 
> > Actually, I just thought of another tweak on this:
> > 
> > 
> > class VideoSourceInterface {
> >   // VideoSinkInterface::SetSource(this) should be called when a sink is
> added.
> >   // VideoSinkInterface::ClearSource(this) should be called when a sink is
> > removed.
> >   // VideoSinkInterface::ClearSource(this) must be called 
> >   // when the VideoSourceInterface is destroyed.
> >   // Adding a current sink is a no-op.
> >   void AddSink(VideoSinkInterface* sink);
> >   void RemoveSink(VideoSinkInterface* sink);
> >   void AddOrUpdateSink(VideoSinkInterface* sink, const VideoWants& wants);
> > }
> > 
> > class VideoSinkInterface {
> >   void OnFrame(VideoFrameT frame);
> >  
> >   // VideoSourceInterface::AddOrUpdateSink should be called 
> >   // when the source is changed or when the wants change.
> >   // VideoSourceInterface::RemoveSink should be called when 
> >   // the source is cleared.
> >   // VideoSouceInterface::RemoveSink must be called
> >   // when the VideoSinkInterface is destroyed.
> >   // Clearing a non-current source is a no-op.
> >   void SetSource(VideoSourceInterface* source);
> >   void ClearSource(VideoSourceInterface* source);
> > }
> > 
> > This would let you call either source->AddSink(sink) or
> sink->SetSource(source)
> > and everything would hookup with the latest capabilities/wants correctly. 
And
> > it doesn't require a synchronous read of the capabilities from the source to
> add
> > the sink.  
> > 
> > But I did just realize one thing about thread safety:
> > 
> > 1.  If the sink is destroyed on one thread and Source::RemoveSink does a
> thread
> > hop, but another thread is still using the now-destroyed sink, then it will
> > crash.  This implies RemoveSink needs to not do a thead-hop.
> > 
> > 2.  Similarly, if a source is destroyed on one thread and Sink::ClearSource
> does
> > a thead hop, but another thread is still using the now-destroyed sink, then
it
> > will crash.  This implies that ClearSource needs to not do a thread-hop.  
> > 
> > 
> > What do you think?
> > 
> > 
> > Of course, this is all just to avoid the synchronous capabilities() or
wants()
> > call.  If we don't think that's a big deal, then all of this might be
> overkill.
> 
> This seems like a good proposal to me. 
> 
> In Chrome we solved your last point on how to remove a sink thread safe by
> making sure a  video frame callbacks are bound to a reference counted object.
> See ex
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
> and
>
MediaStreamVideoTrack::RemoveSink.https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media/media_stream_video_track.cc&l=258
> 
> Stripping away  chromes callbacks/bind magic and looking at it  in this
context 
> it  means that we 
> 1.  make VideoSinkInterFace ref counted.
> 2. VideoSourceInterFace::AddSink take  a temporary  reference to
> VideoSinkinterface and post to the thread where  VideoSinkInterFace::OnFrame
is
> called on. On this thread - store a reference rethe sink.
> 2. VideoSourceInterface::RemoveSink post to the thread that calls
> VideoSinkInterFace::OnFrame,  here  the reference is removed and thus OnFrame
> will not be called again on that sink.  Then post back to the thread where
> AddSink/RemoveSink is called to make sure the last reference to the sink is
> released on the correct thread.
> 


I'm not too excited about making the sink ref-counted.  That would mean that if
we have encoders that are sinks, we'd have to have the encoders be refcounted. 
I'm not too excited about that. And there are probably other sinks where making
them refcounted would be a pain.   I think it would be better to just have
RemoveSink synchronously make sure OnFrame is never called on a removed sink. 
If we can't accomplish that, then we could make some kind of VideoSinkProxy
which tolerates receiving a frame until ClearSource is called, and then deletes
itself.  Only fancy thread-hopping sources would need to use a VideoSinkProxy. 
Everyone else could keep it simple.  In fact, what I've thought up here with
SetSource/ClearSource is already a bit too complicated, and I'd like to see it
be less complicated, not more.



> Can we let VideoSinkinterface be reference counted? 
> I think its easiest if we say that AddSink,  RemoveSink, SetSource, and
> ClearSource are all called on one an only one thread. OnFrame may be called on
> the same thread or another thread.
> On what thread will we allow AddOrUpdateSink be called on? 

Same as AddSink and RemoveSink: any thread.

nisse-webrtc

On 2016/02/03 17:59:12, pthatcher1 wrote: > 1. If the sink is destroyed on one thread ...

4 years, 10 months ago (2016-02-04 07:51:14 UTC) #21

pthatcher1

On 2016/02/04 07:51:14, nisse-webrtc wrote: > On 2016/02/03 17:59:12, pthatcher1 wrote: > > > 1. ...

4 years, 10 months ago (2016-02-04 17:10:58 UTC) #22

On 2016/02/04 07:51:14, nisse-webrtc wrote:
> On 2016/02/03 17:59:12, pthatcher1 wrote:
> 
> > 1.  If the sink is destroyed on one thread and Source::RemoveSink does a
> thread
> > hop, but another thread is still using the now-destroyed sink, then it will
> > crash.  This implies RemoveSink needs to not do a thead-hop.
> 
> I'd say it this way: RemoveSink must guarantee that at the time that method
> returns,
> there is no current and no future calls to OnFrame. So that the sink can be
> safely destroyed.
> 

Yes, that's a better way of putting it.  Exactly right.

> > Of course, this is all just to avoid the synchronous capabilities() or
wants()
> > call.  If we don't think that's a big deal, then all of this might be
> overkill.
> 
> If we go with Source::AddOrUpdateSink(sink, capabilities/hints), do we need 
> any other way to query for them?
> 

That's the whole point of AddOrUpdateSink: an alternative to capabilities().  We
need either one or the other, but not both.

> And I think the hints should be advisory for the source. The sink must be
> prepared 
> to handle whatever frames it gets, and changes to the capabilities/hints can't
> be 
> expected to take effect at precisely the right time. So in that sense,
> capabilities 
> are not synchronous.

We can think of them as "hints" or "wants", but we should realize that if the
sink gets something it doesn't want (like a frame without rotation applied when
it doesn't know how to rotate), then we may end up with frames looking wrong
(rotated the wrong way).  

> 
> Motivation: If a sink discovers a bottleneck in cpu or networking, it's too
late
> 
> to rely on the source alone to fix the problem. So the sink must be prepared
to
> drop 
> or scale down frames. Informing the source is a means to avoid unnecessary
work
> in the
> future, but it's not going to solve the sink's immediate problem.

That's a good point.  Either these are hard limits or soft ones.  If they are
soft ones like "I can't handle more than X fps, so don't bother", then the sink
still needs to do decimation.  If they are hard limits, like "you must not send
me more than X fps", then the sink doesn't, but it puts more burden on getting
this whole thing right.

In other words, are these "needs" (hard) or "wants" (soft)?  It sounds like
you're in favor of these being wants (soft) and having them just be
optimizations.  I was actually thinking of them more as needs (hard) so we don't
have to duplicate rotation/decimation/etc code in two places (the source and the
sink).  But I'm willing to be persuaded the other direction.

perkj_webrtc

Hi Can you PTAL? I have continued to call hints/wants... VideoSinkHints but I don't have ...

4 years, 10 months ago (2016-02-08 14:32:01 UTC) #23

nisse-webrtc

https://codereview.webrtc.org/1655793003/diff/160001/talk/app/webrtc/videosource.cc File talk/app/webrtc/videosource.cc (right): https://codereview.webrtc.org/1655793003/diff/160001/talk/app/webrtc/videosource.cc#newcode30 talk/app/webrtc/videosource.cc:30: #include <string> Left over change? https://codereview.webrtc.org/1655793003/diff/160001/webrtc/media/base/videobroadcaster.h File webrtc/media/base/videobroadcaster.h (right): ...

4 years, 10 months ago (2016-02-08 14:57:04 UTC) #24

pthatcher1

I like the latest changes. I still think I'd prefer "wants" over "hints". I think ...

4 years, 10 months ago (2016-02-08 20:59:40 UTC) #25

perkj_webrtc

Description was changed from ========== Patchset 1 contains Niels cl - Make WebRtcVideoChannel2::WebRtcVideoSendStream implement rtc::VideoRendererInterface. ...

4 years, 10 months ago (2016-02-09 13:23:44 UTC) #27

perkj_webrtc

PTAL https://codereview.webrtc.org/1655793003/diff/160001/talk/app/webrtc/videosource.cc File talk/app/webrtc/videosource.cc (right): https://codereview.webrtc.org/1655793003/diff/160001/talk/app/webrtc/videosource.cc#newcode30 talk/app/webrtc/videosource.cc:30: #include <string> On 2016/02/08 14:57:04, nisse-webrtc wrote: > ...

4 years, 10 months ago (2016-02-09 13:23:58 UTC) #28

nisse-webrtc

lgtm, with nits addressed. I've mainly looked at the new code, I don't really follow ...

4 years, 10 months ago (2016-02-09 14:10:55 UTC) #29

pthatcher1

Mostly more naming nits, but I do think the code around finding pairs could be ...

4 years, 10 months ago (2016-02-09 17:13:41 UTC) #30

perkj_webrtc

PTAL https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videobroadcaster.cc File webrtc/media/base/videobroadcaster.cc (right): https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videobroadcaster.cc#newcode18 webrtc/media/base/videobroadcaster.cc:18: thread_checker_.DetachFromThread(); On 2016/02/09 14:10:54, nisse-webrtc wrote: > Might ...

4 years, 10 months ago (2016-02-10 10:54:37 UTC) #32

PTAL

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
File webrtc/media/base/videobroadcaster.cc (right):

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.cc:18: thread_checker_.DetachFromThread();
On 2016/02/09 14:10:54, nisse-webrtc wrote:
> Might need a comment. From the docs, this seems to imply that the object may
be
> created on one thread, but exclusively used on a different thread. Why?

I think this is common enough to not do it. thread_checker_.DetachFromThread()
allows something to be created on one thread and later used on another. It is
fairly common in libjingle that an object is created on the signaling thread and
later used on the worker thread.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.cc:26: auto lambda = [sink](const SinkPair&
sink_pair) {
On 2016/02/09 14:10:54, nisse-webrtc wrote:
> If the function is named, please use a descriptive name. (I'm happy with
unnamed
> functions too, but I'm not used to it in C++).

Done.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.cc:26: auto lambda = [sink](const SinkPair&
sink_pair) {
On 2016/02/09 17:13:41, pthatcher1 wrote:
> On 2016/02/09 14:10:54, nisse-webrtc wrote:
> > If the function is named, please use a descriptive name. (I'm happy with
> unnamed
> > functions too, but I'm not used to it in C++).
> 
> Or just put it inline:
> 
> auto sink_pair_it = std::find_if(sinks_.begin(), sinks_.end(), [sink](const
> SinkPair& sink_pair) {
>   return sink_pair.first == sink;
> });

use FindSinkPair

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.cc:37: // Rotation must be applied by the
source if one sink request it.
On 2016/02/09 17:13:41, pthatcher1 wrote:
> request it => wants it

Done.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.cc:48: auto lambda = [sink](const SinkPair&
sink_pair) {
On 2016/02/09 17:13:41, pthatcher1 wrote:
> On 2016/02/09 14:10:54, nisse-webrtc wrote:
> > Same function here.
> 
> It might be worth making a private method like:
> 
> SinkPair* FindSinkPair(const VideoSinkInterface<cricket::VideoFrame>* sink);
> 
> That would remove duplication and let the caller just do if(pair) instead  of
if
> (pair != foo.end())

Good idea.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.cc:62: VideoWants
VideoBroadcaster::sink_wants() const {
On 2016/02/09 17:13:41, pthatcher1 wrote:
> Why not just wants()?  If not wants(), then at least sinks_wants() (since
there
> is more than one sink).

Done.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
File webrtc/media/base/videobroadcaster.h (right):

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.h:29: const VideoWants& hints) override;
On 2016/02/09 14:10:54, nisse-webrtc wrote:
> nit: Rename also the argument name hints -> wants.

Done.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.h:44: VideoWants current_hints_;
On 2016/02/09 17:13:41, pthatcher1 wrote:
> wants_?

Done.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videob...
webrtc/media/base/videobroadcaster.h:46:
std::pair<VideoSinkInterface<cricket::VideoFrame>*, VideoWants>;
On 2016/02/09 17:13:41, pthatcher1 wrote:
> If you make a simple struct, then you can use .sink and .wants instead of
.first
> and .second, which would be nicer to read:
> 
> struct SinkPair {
>   VideoSinkInterface<cricket::VideoFrame>* sink;
>   VideoSinkWants wants;
> }
> 
> Then later:
> 
> auto pair = FindPair(sink);
> if (pair) {
>   pair.wants = wants;
> } else {
>   pairs_.push(VideoPair(sink, wants)));
> }

Done.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videoc...
File webrtc/media/base/videocapturer.h (right):

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videoc...
webrtc/media/base/videocapturer.h:267: const rtc::VideoWants& hints) override;
On 2016/02/09 17:13:41, pthatcher1 wrote:
> Should the type be VideoWants or VideoSinkWants?  I'm thinking VideoSinkWants.

Done.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videoc...
webrtc/media/base/videocapturer.h:273: virtual void
OnSinkVideoWantsChanged(const rtc::VideoWants& wants);
On 2016/02/09 17:13:41, pthatcher1 wrote:
> OnVideoSinkWantsChanged would be a better order, but "video" is really clear
> already in this context, so just "OnSinkWantsChanged" would be even better.

Done.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videos...
File webrtc/media/base/videosourceinterface.h (right):

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videos...
webrtc/media/base/videosourceinterface.h:35: virtual void
AddOrUpdateSink(VideoSinkInterface<VideoFrameT>* sink,
On 2016/02/09 17:13:41, pthatcher1 wrote:
> On 2016/02/09 14:10:54, nisse-webrtc wrote:
> > I don't insist on this, but I think it will be prettier if this method is
> > renamed to just AddSink, together with a comment saying that the same method
> is
> > used to change the wants of an already registered sink.
> 
> It would be prettier.  The tradeoff is when you see code later that is:
> 
> source->AddSink(sink, wants)
> 
> for a sink that's already been added.  Reading that code, you'd be confused
> unless you want to read the comment.  
> 
> So, which is more important?  Pretty or readable?  
> 
> I could go either way.

I vote on AddOrUpdate... 

Guess what - we are more or less back on patchset 3(4) with the difference that
we don't have to call RemoveSink to update VideoWants + naming.

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/webrtc/webr...
File webrtc/media/webrtc/webrtcvideocapturer.h (left):

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/webrtc/webr...
webrtc/media/webrtc/webrtcvideocapturer.h:14: #ifdef HAVE_WEBRTC_VIDEO
On 2016/02/09 14:10:54, nisse-webrtc wrote:
> Why are these conditionals removed?

They are meaningless and should be removed. They  annoyed me since I cant use
eclipse for refactoring (because of a bug in eclipse). But the rest of them
should probably be removed in a separate cl...

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/webrtc/webr...
File webrtc/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/webrtc/webr...
webrtc/media/webrtc/webrtcvideoengine2.cc:1799: sink_wants_.rotation_applied =
!ContainsHeaderExtension(
On 2016/02/09 17:13:41, pthatcher1 wrote:
> I love how well "wants rotation applied" reads.

Acknowledged.

pthatcher1

lgtm https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videosourceinterface.h File webrtc/media/base/videosourceinterface.h (right): https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videosourceinterface.h#newcode35 webrtc/media/base/videosourceinterface.h:35: virtual void AddOrUpdateSink(VideoSinkInterface<VideoFrameT>* sink, On 2016/02/10 10:54:36, perkj_webrtc ...

4 years, 10 months ago (2016-02-10 19:29:19 UTC) #33

perkj_webrtc

On 2016/02/10 19:29:19, pthatcher1 wrote: > lgtm > > https://codereview.webrtc.org/1655793003/diff/200001/webrtc/media/base/videosourceinterface.h > File webrtc/media/base/videosourceinterface.h (right): > ...

4 years, 10 months ago (2016-02-10 22:02:43 UTC) #34

perkj_webrtc

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org, pthatcher@webrtc.org Link to ...

4 years, 10 months ago (2016-02-11 07:50:55 UTC) #36

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-11 07:51:11 UTC) #37

commit-bot: I haz the power

Description was changed from ========== This cl introduce a VideoSourceInterface and let cricket::VideoCapturer implement it. ...

4 years, 10 months ago (2016-02-11 07:55:25 UTC) #38

perkj_webrtc

A revert of this CL (patchset #12 id:260001) has been created in https://codereview.webrtc.org/1687273002/ by perkj@webrtc.org. ...

4 years, 10 months ago (2016-02-11 09:19:57 UTC) #40

perkj_webrtc

Description was changed from ========== This cl introduce a VideoSourceInterface and let cricket::VideoCapturer implement it. ...

4 years, 10 months ago (2016-02-11 09:55:12 UTC) #41

perkj_webrtc

Description was changed from ========== This cl introduce a VideoSourceInterface and let cricket::VideoCapturer implement it. ...

4 years, 10 months ago (2016-02-11 10:06:23 UTC) #42

perkj_webrtc

Committed patchset #14 (id:300001) manually as 4d19c5b010473615fa181afa84c6f4b3104e3171 (presubmit successful).

4 years, 10 months ago (2016-02-11 10:06:25 UTC) #43

commit-bot: I haz the power

Description was changed from ========== This cl introduce a VideoSourceInterface and let cricket::VideoCapturer implement it. ...

4 years, 10 months ago (2016-02-11 10:40:48 UTC) #44

commit-bot: I haz the power

Patchset 14 (id:??) landed as https://crrev.com/2f21789b4be6f17b0f39ba18b2ffc93608b0e789 Cr-Commit-Position: refs/heads/master@{#11563}

4 years, 10 months ago (2016-02-11 10:40:50 UTC) #45

commit-bot: I haz the power

Description was changed from ========== This cl introduce a VideoSourceInterface and let cricket::VideoCapturer implement it. ...

4 years, 10 months ago (2016-02-11 10:40:56 UTC) #46

commit-bot: I haz the power

Patchset 14 (id:??) landed as https://crrev.com/4d19c5b010473615fa181afa84c6f4b3104e3171 Cr-Commit-Position: refs/heads/master@{#11567}

4 years, 10 months ago (2016-02-11 10:40:57 UTC) #47

perkj_webrtc

4 years, 10 months ago (2016-02-11 10:56:02 UTC) #48

Message was sent while issue was closed.

A revert of this CL (patchset #14 id:300001) has been created in
https://codereview.webrtc.org/1690893002/ by perkj@webrtc.org.

The reason for reverting is: Needs to revert again unfortunately. 
There are multiple implementations in Chrome of cricket::VideoCapturer. 

One is ./../remoting/protocol/webrtc_video_capturer_adapter.cc...

https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build...


Fun times - I will have to modify this cl after trying it manually out in
Chrome..

Issue 1655793003: Make cricket::VideoCapturer implement VideoSourceInterface (Closed)

Description

Patch Set 1 : http://crrev.com/1631703002#ps140001 #

Patch Set 2 : Adding VideoSourceInterface and letting cricket::VideoCapturer implement it #

Patch Set 3 : Restarted... #

Patch Set 4 : Fixed Android #

Patch Set 5 : AddSink returns Callback #

Patch Set 6 : Rebased #

Patch Set 7 : Rebase only #

Patch Set 8 : Use AddOrUpdateSink #

Patch Set 9 : Addressed comments. #

Patch Set 10 : Addressed comments. Removed AddSink #

Patch Set 11 : Addressed comments #

Patch Set 12 : rebased #

Patch Set 13 : Revert of the Revert #

Patch Set 14 : Added dummy capturerenderadapter.cc #

Messages