New helper class AdaptedVideoSource.

New helper class AdaptedVideoSource.

Keeps a TimestampAligner, a VideoAdapter and VideoBroadcaster.
Intended to be used by all capturer-like VideoTrackSource:s, to avoid
duplication of common adaptation logic.

Superseded by https://codereview.webrtc.org/2328333002/

BUG=

[nisse-webrtc, 2016-09-08 12:41:19]

nisse@webrtc.org changed reviewers:
+ magjed@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org

[nisse-webrtc, 2016-09-08 12:41:19]

This is a request-for-comments cl.

Background is that we'd like to eliminate the use of cricket::VideoCapturer from
chrome, and there's a need for some VideoCapturer-lite class with common logic,
currently duplicated in cricket::VideoCapturer and AndroidVideoTrackSource, and
soon also for capturers in Chrome and iOS.

Some thoughts:

1. It probably best to not actually change cricket::VideoCapturer to use the new
class, and avoid interface changes there until it can finally be deleted.

2. It would make sense to have this class somewhere in the VideoTrackSource
inheritance hierarchy, as a base class only inheriting
VideoTrackSourceInterface. But that seems a bit messy, and we currently have the
VideoTrackSource base class which does almost nothing but delegating things to
an associated VideoSourceInterface. Suggestions welcome.

3. I'd like to redesign the AdaptFrame method, so we can avoid checking the
enable_video_adapter flag. It's also unclear to me if the flag is ever changed
mid stream, or if it could be a construction-time thing.

[magjed_webrtc, 2016-09-08 14:42:01]

On 2016/09/08 12:41:19, nisse-webrtc wrote:
> This is a request-for-comments cl.
> 
> Background is that we'd like to eliminate the use of cricket::VideoCapturer
from
> chrome, and there's a need for some VideoCapturer-lite class with common
logic,
> currently duplicated in cricket::VideoCapturer and AndroidVideoTrackSource,
and
> soon also for capturers in Chrome and iOS.
> 
> Some thoughts:
> 
> 1. It probably best to not actually change cricket::VideoCapturer to use the
new
> class, and avoid interface changes there until it can finally be deleted.
> 
> 2. It would make sense to have this class somewhere in the VideoTrackSource
> inheritance hierarchy, as a base class only inheriting
> VideoTrackSourceInterface. But that seems a bit messy, and we currently have
the
> VideoTrackSource base class which does almost nothing but delegating things to
> an associated VideoSourceInterface. Suggestions welcome.
> 
> 3. I'd like to redesign the AdaptFrame method, so we can avoid checking the
> enable_video_adapter flag. It's also unclear to me if the flag is ever changed
> mid stream, or if it could be a construction-time thing.

2. What is VideoTrackSource used for? If it's only one subclass, can you merge
it with that one? I don't think that class deserves such a generic name.

3. We don't have an enable_video_adapter flag in Android (and we don't want
one). Can't we just leave enable_video_adapter in cricket::VideoCapturer and not
add it to the new class? Also, can you move GetStats and OnSinkWantsChanged into
the new class?

[nisse-webrtc, 2016-09-09 08:59:09]

On 2016/09/08 14:42:01, magjed_webrtc wrote:
> 2. What is VideoTrackSource used for? If it's only one subclass, can you merge
> it with that one? I don't think that class deserves such a generic name.

It's used directly by RTPReceiver, and inherited by VideoCapturerTrackSource. I
guess we don't want adaptation logic for remote sources?
  
> 3. We don't have an enable_video_adapter flag in Android (and we don't want
> one). Can't we just leave enable_video_adapter in cricket::VideoCapturer and
not
> add it to the new class?

That would be good. I'm also considering untangling the timestamping logic and
adaptation. In a different cl, Per suggested that TimestampAligner should have a
single public method, TranslateTimestamp, and in that case doesn't have to be
part of AdaptFrame.

> Also, can you move GetStats 

Maybe. In that case, it would become more natural to have it inherit
VideoTrackSourceInterface, and have the VideoBroadcaster as a member (in the
current patch set I inherit VideoBroadcaster, because I find it a bit awkward
with a looong chain of delegations of AddOrUpdateSink.

> and OnSinkWantsChanged into
> the new class? 

Then the apply_rotation_ flag would move into the new class as well? Makes some
sense.

[perkj_webrtc, 2016-09-10 07:06:16]

On 2016/09/09 08:59:09, nisse-webrtc wrote:
> On 2016/09/08 14:42:01, magjed_webrtc wrote:
> > 2. What is VideoTrackSource used for? If it's only one subclass, can you
merge
> > it with that one? I don't think that class deserves such a generic name.
> 
> It's used directly by RTPReceiver, and inherited by VideoCapturerTrackSource.
I
> guess we don't want adaptation logic for remote sources?
>   
> > 3. We don't have an enable_video_adapter flag in Android (and we don't want
> > one). Can't we just leave enable_video_adapter in cricket::VideoCapturer and
> not
> > add it to the new class?
> 
can't we just skip enable_video_adapter()? Is it used by any production code or
just tests? 

> That would be good. I'm also considering untangling the timestamping logic and
> adaptation. In a different cl, Per suggested that TimestampAligner should have
a
> single public method, TranslateTimestamp, and in that case doesn't have to be
> part of AdaptFrame.
> 
> > Also, can you move GetStats 
> 
> Maybe. In that case, it would become more natural to have it inherit
> VideoTrackSourceInterface, and have the VideoBroadcaster as a member (in the
> current patch set I inherit VideoBroadcaster, because I find it a bit awkward
> with a looong chain of delegations of AddOrUpdateSink.
> 
> > and OnSinkWantsChanged into
> > the new class? 
> 
> Then the apply_rotation_ flag would move into the new class as well? Makes
some
> sense.

So this turns into a base class for  implementing VideoTrackSourceInterface that
Chrome and Android/ ios can use instead of as now is, cricket::VideoCapturer?
sgmt. 
Be carefull with bool IsScreenCast and bool NeedsDenoising()

[nisse-webrtc, 2016-09-20 08:21:38]

Description was changed from

==========
New helper class AdaptedVideoSource.

Keeps a TimestampAligner, a VideoAdapter and VideoBroadcaster.
Intended to be used by all capturer-like VideoTrackSource:s, to avoid
duplication of common adaptation logic.

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

to

==========
New helper class AdaptedVideoSource.

Keeps a TimestampAligner, a VideoAdapter and VideoBroadcaster.
Intended to be used by all capturer-like VideoTrackSource:s, to avoid
duplication of common adaptation logic.

Superseded by https://codereview.webrtc.org/2328333002/

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