Move ownership of send ViEChannels and ViEEncoder to VideoSendStream.

Move ownership of send ViEChannels and ViEEncoder to VideoSendStream.

This is the first CL to get ready for adapting audio bitrate based on
BWE. I've kept this CL as small as possible and had to add a few getters
to ChannelManager. The next CL will do the same for receive ViEChannels.

The getters are a bit uggly, but is an in-between-state. Let's discuss
future ownership of the different modules and what do do with
ChannelGroup.

BUG=webrtc:5079
Committed: https://crrev.com/949c2f04b4156095090e02f3f13613aadacce88d
Cr-Commit-Position: refs/heads/master@{#10298}

[mflodman, 2015-10-15 08:55:57]

mflodman@webrtc.org changed reviewers:
+ pbos@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2015-10-15 11:33:45]

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:140: RTC_DCHECK(!ssrcs.empty());
No need to DCHECK this here and on line 127.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:163: vie_encoder_->SetSsrcs(first_ssrc);
It's not entirely clear to me why we only set the first ssrc here. Maybe we
could add a comment?

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:288: }
Maybe make encoders_ a list and do encoders_.remove(encoder) instead?

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:331: TransportFeedbackObserver*
ChannelGroup::GetTransportFeedbackObserver() {
It doesn't look like transport feedback adapter should be owned by the
ChannelGroup... I think TransportFeedbackAdapter soon will become
BandwidthEstimator, and also get to own BitrateController /
SendSideBandwidthEstimation, and if we run the send-side-only BWE that will be
the only BWE object.

I'm not sure this makes a difference for this CL, probably not. It may not be
worth the trouble to remove this method now.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:381: pad_up_to_bitrate_bps +=
encoder->GetPaddingNeededBps();
It would really be nice to improve this... Maybe add a todo that we should
rethink how we decide how much padding we need, and that we should try to remove
encoders_ from ChannelGroup?

[pbos-webrtc, 2015-10-15 12:07:55]

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:144: config.pre_encode_callback,
channel_group_->pacer(),
Do we want pacer/allocator through constructor instead of hopping between
channel_group_?

I think call and channel_group_ should be unified so Call should own these when
calling down (but that can easily be punted of course).

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:156: RTC_CHECK(vie_channel_->Init() == 0);
Can we make this one a bool while we're at it?

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:163: vie_encoder_->SetSsrcs(first_ssrc);
On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> It's not entirely clear to me why we only set the first ssrc here. Maybe we
> could add a comment?

Pref setting all the ssrcs, and then vie_encoder_ can do what it pleases with
it?

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:259: unsigned int remote_ssrc = 0;
Is this not an uint32_t?

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:260:
vie_channel_->GetRemoteSSRC(&remote_ssrc);
Can this fail? If so should we remove the stream zero?

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_stream.h
File webrtc/video/video_send_stream.h (left):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.h:81: const int channel_id_;
Woohooo!

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:280: 
Remove empty line

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:288: }
On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> Maybe make encoders_ a list and do encoders_.remove(encoder) instead?

Don't think so, iterating a vector is faster and this is rarely ever added
to/deleted from.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:381: pad_up_to_bitrate_bps +=
encoder->GetPaddingNeededBps();
On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> It would really be nice to improve this... Maybe add a todo that we should
> rethink how we decide how much padding we need, and that we should try to
remove
> encoders_ from ChannelGroup?

Yeah I think registering encoders on the pacer directly makes more sense. That's
definitely a TODO though.

[mflodman, 2015-10-15 14:39:22]

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:140: RTC_DCHECK(!ssrcs.empty());
On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> No need to DCHECK this here and on line 127.

Thanks, copy paste generated from channel group.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:144: config.pre_encode_callback,
channel_group_->pacer(),
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> Do we want pacer/allocator through constructor instead of hopping between
> channel_group_?
> 
> I think call and channel_group_ should be unified so Call should own these
when
> calling down (but that can easily be punted of course).

Kept it as is for now, since I need to leave for today :)
Let me know if you think we should do this alternatively keep it until we decide
how to treat the ChannelManager members.

The rest is something we should discuss, maybe outside of this CL. I don't think
Call should own all of the ChannelManager content and I think Stefan was on the
same track. Let's discuss that outside of this CL.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:156: RTC_CHECK(vie_channel_->Init() == 0);
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> Can we make this one a bool while we're at it?

I did that change, but reverted. I'm not a big fan of mixing bool and int
returns in the same class for error values.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:163: vie_encoder_->SetSsrcs(first_ssrc);
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> > It's not entirely clear to me why we only set the first ssrc here. Maybe we
> > could add a comment?
> 
> Pref setting all the ssrcs, and then vie_encoder_ can do what it pleases with
> it?

This is just copy paste, I can try to figure out and comment but would prefer to
not change too much logic at the same time.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:259: unsigned int remote_ssrc = 0;
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> Is this not an uint32_t?

Yes, should be. Copy paste from ChannelManager.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:260:
vie_channel_->GetRemoteSSRC(&remote_ssrc);
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> Can this fail? If so should we remove the stream zero?

No, it can't fail.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_stream.h
File webrtc/video/video_send_stream.h (left):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.h:81: const int channel_id_;
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> Woohooo!

:)

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:280: 
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> Remove empty line

Done.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:288: }
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> > Maybe make encoders_ a list and do encoders_.remove(encoder) instead?
> 
> Don't think so, iterating a vector is faster and this is rarely ever added
> to/deleted from.

Yes, I chose a vector since we do iterate over this quite often.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:331: TransportFeedbackObserver*
ChannelGroup::GetTransportFeedbackObserver() {
On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> It doesn't look like transport feedback adapter should be owned by the
> ChannelGroup... I think TransportFeedbackAdapter soon will become
> BandwidthEstimator, and also get to own BitrateController /
> SendSideBandwidthEstimation, and if we run the send-side-only BWE that will be
> the only BWE object.
> 
> I'm not sure this makes a difference for this CL, probably not. It may not be
> worth the trouble to remove this method now.

I agree. I do prefer to keep this logic in ChannelGroup for this CL since it was
here earlier. But a good point and we should agree on where it should be.

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:381: pad_up_to_bitrate_bps +=
encoder->GetPaddingNeededBps();
On 2015/10/15 12:07:55, pbos-webrtc wrote:
> On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> > It would really be nice to improve this... Maybe add a todo that we should
> > rethink how we decide how much padding we need, and that we should try to
> remove
> > encoders_ from ChannelGroup?
> 
> Yeah I think registering encoders on the pacer directly makes more sense.
That's
> definitely a TODO though.

TODO added.

[pbos-webrtc, 2015-10-15 14:41:44]

lgtm, but please return an uint32_t since it can't fail. :)

https://codereview.webrtc.org/1394243006/diff/20001/webrtc/video_engine/vie_c...
File webrtc/video_engine/vie_channel.cc (right):

https://codereview.webrtc.org/1394243006/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel.cc:728: void ViEChannel::GetRemoteSSRC(uint32_t*
ssrc) {
return uint32_t then?

[mflodman, 2015-10-15 14:47:43]

https://codereview.webrtc.org/1394243006/diff/20001/webrtc/video_engine/vie_c...
File webrtc/video_engine/vie_channel.cc (right):

https://codereview.webrtc.org/1394243006/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel.cc:728: void ViEChannel::GetRemoteSSRC(uint32_t*
ssrc) {
On 2015/10/15 14:41:44, pbos-webrtc wrote:
> return uint32_t then?

Hmm, I kind of see your point...

[stefan-webrtc, 2015-10-16 06:51:54]

lgtm

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1394243006/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:288: }
On 2015/10/15 14:39:22, mflodman wrote:
> On 2015/10/15 12:07:55, pbos-webrtc wrote:
> > On 2015/10/15 11:33:45, stefan-webrtc (holmer) wrote:
> > > Maybe make encoders_ a list and do encoders_.remove(encoder) instead?
> > 
> > Don't think so, iterating a vector is faster and this is rarely ever added
> > to/deleted from.
> 
> Yes, I chose a vector since we do iterate over this quite often.

I doubt the loop we do over this will make a difference regarding performance,
and personally prefer simpler code for that reason. But I can live with it since
it's going away.

[mflodman, 2015-10-16 09:29:59]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2015-10-16 09:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1394243006/#ps60001 (title:
"Removed header leftovers.")

[commit-bot: I haz the power, 2015-10-16 09:30:08]

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

[commit-bot: I haz the power, 2015-10-16 09:31:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-16 09:31:23]

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