Move BitrateAllocator reference from ViEEncoder to VideoSendStream.

Move BitrateAllocator reference from ViEEncoder to VideoSendStream.

This CL will be followed up with a CL adding AudioSendStream to
BitrateAllocator, so this is a small CL to have the video connection to
BitrateAllocator "at the same level" as for audio.

BUG=webrtc:5079
R=stefan@webrtc.org

Committed: https://crrev.com/86aabb288d61d56955e7fa4890f46f3388f28263
Cr-Commit-Position: refs/heads/master@{#11955}

[mflodman, 2016-03-11 12:45:02]

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

[mflodman, 2016-03-11 12:45:03]

Stefan,
Please take a look. It's a quite small CL.

https://codereview.webrtc.org/1785283002/diff/1/webrtc/call/bitrate_allocator.h
File webrtc/call/bitrate_allocator.h (right):

https://codereview.webrtc.org/1785283002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.h:28: class BitrateAllocatorObserver {
This is mainly to disconnect the send streams from BitrateController and hava
all bitrate updates through BitrateAllocator instead.

WDYT?

[stefan-webrtc, 2016-03-11 13:43:54]

https://codereview.webrtc.org/1785283002/diff/1/webrtc/call/bitrate_allocator.h
File webrtc/call/bitrate_allocator.h (right):

https://codereview.webrtc.org/1785283002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.h:10: *  Usage: this class will register multiple
RtcpBitrateObserver's one at each
Change this comment to be C++ style and separate it from the copyright header.

https://codereview.webrtc.org/1785283002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.h:28: class BitrateAllocatorObserver {
On 2016/03/11 12:45:03, mflodman wrote:
> This is mainly to disconnect the send streams from BitrateController and hava
> all bitrate updates through BitrateAllocator instead.
> 
> WDYT?

Probably a good idea to make it clear that send streams shouldn't be connected
directly to the bitrate controller.

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

https://codereview.webrtc.org/1785283002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:159: BitrateAllocator* bitrate_allocator,
It would have been nicer if we didn't have to pass this in at all to the send
streams and instead let Call add the send streams to the allocator. The problem
with that is that we can't set the start bitrate, but maybe that doesn't matter
since we anyway will get a bitrate update (almost?) immediately? WDYT?

[mflodman, 2016-03-11 14:26:05]

https://codereview.webrtc.org/1785283002/diff/1/webrtc/call/bitrate_allocator.h
File webrtc/call/bitrate_allocator.h (right):

https://codereview.webrtc.org/1785283002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.h:10: *  Usage: this class will register multiple
RtcpBitrateObserver's one at each
On 2016/03/11 13:43:53, stefan-webrtc (holmer) wrote:
> Change this comment to be C++ style and separate it from the copyright header.

Done, and moved to the class as in style guide.

https://codereview.webrtc.org/1785283002/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.h:28: class BitrateAllocatorObserver {
On 2016/03/11 13:43:53, stefan-webrtc (holmer) wrote:
> On 2016/03/11 12:45:03, mflodman wrote:
> > This is mainly to disconnect the send streams from BitrateController and
hava
> > all bitrate updates through BitrateAllocator instead.
> > 
> > WDYT?
> 
> Probably a good idea to make it clear that send streams shouldn't be connected
> directly to the bitrate controller.

Done.

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

https://codereview.webrtc.org/1785283002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:159: BitrateAllocator* bitrate_allocator,
On 2016/03/11 13:43:53, stefan-webrtc (holmer) wrote:
> It would have been nicer if we didn't have to pass this in at all to the send
> streams and instead let Call add the send streams to the allocator. The
problem
> with that is that we can't set the start bitrate, but maybe that doesn't
matter
> since we anyway will get a bitrate update (almost?) immediately? WDYT?

Yes, I did that initially but reverted for the reason you mentioned.

Let's discuss.

[mflodman, 2016-03-11 14:28:39]

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

https://codereview.webrtc.org/1785283002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:159: BitrateAllocator* bitrate_allocator,
On 2016/03/11 14:26:05, mflodman wrote:
> On 2016/03/11 13:43:53, stefan-webrtc (holmer) wrote:
> > It would have been nicer if we didn't have to pass this in at all to the
send
> > streams and instead let Call add the send streams to the allocator. The
> problem
> > with that is that we can't set the start bitrate, but maybe that doesn't
> matter
> > since we anyway will get a bitrate update (almost?) immediately? WDYT?
> 
> Yes, I did that initially but reverted for the reason you mentioned.
> 
> Let's discuss.

Just to get the discussion on the record.

This doesn't work since VideoSendStream::ReconfigureBitrate currently calls
BitrateAllocator::AddObserver with new bitrate limits. This could be changed in
the future to have BitrateAllocator instead ask BitrateAllocatorObserver for
bitrate limits each new allocation round, but it's a larger change and something
we possibly will  followup with later.

[stefan-webrtc, 2016-03-11 14:42:30]

lgtm

[mflodman, 2016-03-11 14:43:48]

On 2016/03/11 14:42:30, stefan-webrtc (holmer) wrote:
> lgtm

Submitting since ps#1 passed all bots and ps#2 only contains changes to
comments.

[mflodman, 2016-03-11 14:44:49]

Description was changed from

==========
Move BitrateAllocator reference from ViEEncoder to VideoSendStream.

This CL will be followed up with a CL adding AudioSendStream to
BitrateAllocator, so this is a small CL to have the video connection to
BitrateAllocator "at the same level" as for audio.

BUG=webrtc:5079
==========

to

==========
Move BitrateAllocator reference from ViEEncoder to VideoSendStream.

This CL will be followed up with a CL adding AudioSendStream to
BitrateAllocator, so this is a small CL to have the video connection to
BitrateAllocator "at the same level" as for audio.

BUG=webrtc:5079
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/86aabb288d61d56955e7fa489...
==========

[mflodman, 2016-03-11 14:44:51]

Committed patchset #2 (id:20001) manually as
86aabb288d61d56955e7fa4890f46f3388f28263 (presubmit successful).

[commit-bot: I haz the power, 2016-03-11 14:44:51]

Description was changed from

==========
Move BitrateAllocator reference from ViEEncoder to VideoSendStream.

This CL will be followed up with a CL adding AudioSendStream to
BitrateAllocator, so this is a small CL to have the video connection to
BitrateAllocator "at the same level" as for audio.

BUG=webrtc:5079
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/86aabb288d61d56955e7fa489...
==========

to

==========
Move BitrateAllocator reference from ViEEncoder to VideoSendStream.

This CL will be followed up with a CL adding AudioSendStream to
BitrateAllocator, so this is a small CL to have the video connection to
BitrateAllocator "at the same level" as for audio.

BUG=webrtc:5079
R=stefan@webrtc.org

Committed: https://crrev.com/86aabb288d61d56955e7fa4890f46f3388f28263
Cr-Commit-Position: refs/heads/master@{#11955}
==========

[commit-bot: I haz the power, 2016-03-11 14:44:52]

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