Refactor before implementing per stream suspension.

Refactor before implementing per stream suspension.

This CL contains a few minor changes to names, function signatures and
merges two structs into one.

BUG=5868
Committed: https://crrev.com/2ebe5b1cd8e2e9b3142a122a56a1204904aad1d2
Cr-Commit-Position: refs/heads/master@{#12716}

[mflodman, 2016-05-06 13:44:52]

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

[mflodman, 2016-05-06 13:44:52]

Stefan,
Please take a look.

[mflodman, 2016-05-09 04:17:08]

On 2016/05/06 13:44:52, mflodman wrote:
> Stefan,
> Please take a look.

I'll look at the failing SuspendBelowMinBitrate, I must have changed something I
didn't expect to change...

[stefan-webrtc, 2016-05-09 13:46:53]

Don't think I found the issue, but otherwise a nice improvement!

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator.cc
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:58: ObserverConfigList::iterator it =
FindObserverConfig(observer);
Optionally you can change this to "auto"

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:87: ObserverConfigList::iterator it =
FindObserverConfig(observer);
auto here too if you want.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:101: it != bitrate_observer_configs_.end();
++it) {
for (auto it : bitrate_observer_configs_) {

or auto kv

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:116: return ZeroRateAllocation();
Move these two lines to line 111 since they don't depend on sum_min_bitrates?

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:127:
static_cast<uint32_t>(bitrate_observer_configs_.size());
DCHECK that num_remaining_observers is greater than zero.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:148: if (num_remaining_observers != 0) {
Feel free to remove {}

[mflodman, 2016-05-11 14:53:04]

PTAL Stefan,
Comment addressed / answered and issue found.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator.cc
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:58: ObserverConfigList::iterator it =
FindObserverConfig(observer);
On 2016/05/09 13:46:53, stefan-webrtc (holmer) wrote:
> Optionally you can change this to "auto"

Done.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:87: ObserverConfigList::iterator it =
FindObserverConfig(observer);
On 2016/05/09 13:46:53, stefan-webrtc (holmer) wrote:
> auto here too if you want.

Done.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:101: it != bitrate_observer_configs_.end();
++it) {
On 2016/05/09 13:46:53, stefan-webrtc (holmer) wrote:
> for (auto it : bitrate_observer_configs_) {
> 
> or auto kv

Then I'll get an ObserverConfig reference instead of an iterator and the
returned iterator is used for erasing at one of the two instances it's called.
Changed to use auto, but as an iterator in this case.

One option, maybe nicer?, is to return ObserverConfig here and another method
for erasing. WDYT? Both methods will only be used at one place each though.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:116: return ZeroRateAllocation();
On 2016/05/09 13:46:53, stefan-webrtc (holmer) wrote:
> Move these two lines to line 111 since they don't depend on sum_min_bitrates?

Done.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:127:
static_cast<uint32_t>(bitrate_observer_configs_.size());
On 2016/05/09 13:46:53, stefan-webrtc (holmer) wrote:
> DCHECK that num_remaining_observers is greater than zero.

Done.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:148: if (num_remaining_observers != 0) {
On 2016/05/09 13:46:53, stefan-webrtc (holmer) wrote:
> Feel free to remove {}

Done.

[stefan-webrtc, 2016-05-12 12:26:59]

LG, two minor comments.

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator.cc
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/1952923005/diff/1/webrtc/call/bitrate_allocator...
webrtc/call/bitrate_allocator.cc:101: it != bitrate_observer_configs_.end();
++it) {
On 2016/05/11 14:53:04, mflodman wrote:
> On 2016/05/09 13:46:53, stefan-webrtc (holmer) wrote:
> > for (auto it : bitrate_observer_configs_) {
> > 
> > or auto kv
> 
> Then I'll get an ObserverConfig reference instead of an iterator and the
> returned iterator is used for erasing at one of the two instances it's called.
> Changed to use auto, but as an iterator in this case.
> 
> One option, maybe nicer?, is to return ObserverConfig here and another method
> for erasing. WDYT? Both methods will only be used at one place each though.

Ah, good point, letäs keep it as is.

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/call/bitrate_alloc...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator.cc:119: if (last_bitrate_bps_ <= sum_min_bitrates)
Add {} for this if/else

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:574: // vie_encoder_.SetEncoder must be called
before this.
Is it possible to DCHECK on something in ViEEncoder or VideoSender to ensure
that the call order is correct?

[mflodman, 2016-05-12 13:10:59]

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/call/bitrate_alloc...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator.cc:119: if (last_bitrate_bps_ <= sum_min_bitrates)
On 2016/05/12 12:26:59, stefan-webrtc (holmer) wrote:
> Add {} for this if/else

It was correct as it was, given that no one-line-if has {} in this file. I
removed the else completely though.

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:574: // vie_encoder_.SetEncoder must be called
before this.
On 2016/05/12 12:26:59, stefan-webrtc (holmer) wrote:
> Is it possible to DCHECK on something in ViEEncoder or VideoSender to ensure
> that the call order is correct?

There is an assert video_sender_->media_opt triggereintriggering when the error
happened + this call will be removed when I move everything to BitrateAllocator
in the follow up work on this, so I don't think it's worth checking more than
already done.

[stefan-webrtc, 2016-05-12 13:33:44]

lgtm

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/call/bitrate_alloc...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator.cc:119: if (last_bitrate_bps_ <= sum_min_bitrates)
On 2016/05/12 13:10:59, mflodman wrote:
> On 2016/05/12 12:26:59, stefan-webrtc (holmer) wrote:
> > Add {} for this if/else
> 
> It was correct as it was, given that no one-line-if has {} in this file. I
> removed the else completely though.

Yes I meant if you were going to keep the else than this is a multiline if/else.
If the else is removed it's a twoline so then it's fine :)

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1952923005/diff/20001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:574: // vie_encoder_.SetEncoder must be called
before this.
On 2016/05/12 13:10:59, mflodman wrote:
> On 2016/05/12 12:26:59, stefan-webrtc (holmer) wrote:
> > Is it possible to DCHECK on something in ViEEncoder or VideoSender to ensure
> > that the call order is correct?
> 
> There is an assert video_sender_->media_opt triggereintriggering when the
error
> happened + this call will be removed when I move everything to
BitrateAllocator
> in the follow up work on this, so I don't think it's worth checking more than
> already done.

great

[mflodman, 2016-05-13 07:44:17]

The CQ bit was checked by mflodman@webrtc.org

[mflodman, 2016-05-13 07:44:17]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1952923005/#ps60001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-05-13 07:44:23]

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

[commit-bot: I haz the power, 2016-05-13 08:43:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-13 08:43:59]

Description was changed from

==========
Refactor before implementing per stream suspension.

This CL contains a few minor changes to names, function signatures and
merges two structs into one.

BUG=5868
==========

to

==========
Refactor before implementing per stream suspension.

This CL contains a few minor changes to names, function signatures and
merges two structs into one.

BUG=5868

Committed: https://crrev.com/2ebe5b1cd8e2e9b3142a122a56a1204904aad1d2
Cr-Commit-Position: refs/heads/master@{#12716}
==========

[commit-bot: I haz the power, 2016-05-13 08:44:00]

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