Extract simulcast rate allocation outside of video encoder.

Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

BUG=webrtc:5206
R=asapersson@webrtc.org, kjellander@webrtc.org, noahric@chromium.org

Committed: https://crrev.com/78ce619a0cd871a769b5c3892dc930df10dfa253
Cr-Commit-Position: refs/heads/master@{#14186}

[sprang_webrtc, 2016-08-29 12:49:51]

Description was changed from

==========
Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

BUG=webrtc:5206
==========

to

==========
Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

It also changes the allocation logic somewhat. Now, if the top layer has been
saturated, any remaining bitrate will be filled also into the lower layers.


BUG=webrtc:5206
==========

[sprang_webrtc, 2016-08-29 12:52:24]

sprang@webrtc.org changed reviewers:
+ marpan@webrtc.org, noahric@chromium.org

[sprang_webrtc, 2016-08-29 12:52:24]

sprang@webrtc.org changed reviewers:
+ marpan@webrtc.org, noahric@chromium.org

[sprang_webrtc, 2016-08-29 12:52:25]

ptal

Noah, add you thinking that you might have insight into why a disabled layer is
configured with max bitrate of the previous layer. Is this needed? If it's only
to keep an external encoder alive, why not just set it to the min bitrate of
that layer?

[sprang_webrtc, 2016-08-29 12:52:25]

ptal

Noah, add you thinking that you might have insight into why a disabled layer is
configured with max bitrate of the previous layer. Is this needed? If it's only
to keep an external encoder alive, why not just set it to the min bitrate of
that layer?

[noahric, 2016-08-29 21:32:09]

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:199: if
(!start_bitrate_kbps) {
Prefer == 0 for integral values. Most of libjingle does anyway, though it's also
because I put this comment on libjingle CLs :)

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:379: //
work.
I see this comment goes waaaay back in webrtc (in vp8_internal_impl.cc in much
older versions), but it's not obvious to me why it is needed. Maybe there was an
assert somewhere else that the bitrate for each layer had to be >= the bitrate
of the lower layer, so max of stream_idx-1 was the lowest safest bitrate to
pick.

We should see if it works with just using 0. Internal source encoders don't
otherwise know to stop producing frames (they can't otherwise observe the
send_stream state). Today, we are wasting cycles in simulcast cases encoding
streams that aren't being sent, so there could be an interesting improvement
there as well.

If it is needed, it should move into vp8_impl.cc, because my guess is that was
the only place that ever actually needed it (no other external vp8 codec
implementation I'm aware of ever needed it).

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:42: uint32_t
layer = 0;
size_t for an array index?

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:54: for (; layer
--> 0;) {
Can you rewrite this? As written, the code looks wrong, because it's non-obvious
that the first loop decrements layer from the last loop.

This would be a bit clearer:

// <existing comment>
// The active layer starts with layer (above) - 1, because the loop always steps
// past the last active layer.
for (size_t active_layer = layer - 1; active_layer >= 0; --active_layer) {
}

[sprang_webrtc, 2016-08-30 13:27:24]

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:199: if
(!start_bitrate_kbps) {
On 2016/08/29 21:32:09, noahric wrote:
> Prefer == 0 for integral values. Most of libjingle does anyway, though it's
also
> because I put this comment on libjingle CLs :)

Done.

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:379: //
work.
On 2016/08/29 21:32:09, noahric wrote:
> I see this comment goes waaaay back in webrtc (in vp8_internal_impl.cc in much
> older versions), but it's not obvious to me why it is needed. Maybe there was
an
> assert somewhere else that the bitrate for each layer had to be >= the bitrate
> of the lower layer, so max of stream_idx-1 was the lowest safest bitrate to
> pick.
> 
> We should see if it works with just using 0. Internal source encoders don't
> otherwise know to stop producing frames (they can't otherwise observe the
> send_stream state). Today, we are wasting cycles in simulcast cases encoding
> streams that aren't being sent, so there could be an interesting improvement
> there as well.
> 
> If it is needed, it should move into vp8_impl.cc, because my guess is that was
> the only place that ever actually needed it (no other external vp8 codec
> implementation I'm aware of ever needed it).

Gotcha, thanks.

vp8_impl.cc already has it's own internal handling of simulcast though. Looking
at WebRtcVideoChannel2::WebRtcVideoSendStream::CreateVideoEncoder, is seems like
SimulcastEncoderAdapter will only be created if it is an external encoder being
used. Otherwise  webrtc::VideoEncoder::Create(webrtc::VideoEncoder::kVp8) is
called directly, creating a VP8EncoderImpl without the wrapper.

I think I'll change this and set target bitrate to 0 for unused streams. Can you
help check that I don't break too many things upstream?

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:42: uint32_t
layer = 0;
On 2016/08/29 21:32:09, noahric wrote:
> size_t for an array index?

Done.

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:54: for (; layer
--> 0;) {
On 2016/08/29 21:32:09, noahric wrote:
> Can you rewrite this? As written, the code looks wrong, because it's
non-obvious
> that the first loop decrements layer from the last loop.
> 
> This would be a bit clearer:
> 
> // <existing comment>
> // The active layer starts with layer (above) - 1, because the loop always
steps
> // past the last active layer.
> for (size_t active_layer = layer - 1; active_layer >= 0; --active_layer) {
> }

Aww. I was was aware this was going to be shot down, I just like the "i --> 0"
syntax as it intuitively looks like "as i approaches 0". But yeah, how it
actually works isn't really that intuitive :)
Also, your suggestion won't work. size_t is unsigned and so active_layer will
always be >= 0. I'll test for equality to static_cast<size_t>(-1), which is
kinda ugly but correct. Post-decrementing in the test is the other alternative
as far as I can see.

[noahric, 2016-08-30 18:14:33]

lgtm.

> Now, if the top layer has been saturated, any remaining bitrate will be filled
also into the lower layers.

I hadn't thought about that yet. That means, in a call that isn't bandwidth
limited by estimation, an HD call will now use 250kbps more than it would before
(200kbps for VGA and 50kbps for QVGA). That's 8% more than before, which isn't a
huge amount, but I'll give a heads up to some folks to expect an increase in
sent bitrate for whatever version of chrome this ends up in.

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc
(right):

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:379: //
work.
> I think I'll change this and set target bitrate to 0 for unused streams. Can
you
> help check that I don't break too many things upstream?

Yup! By inspection, at least, I don't expect problems, but we'll see what
happens :)

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc (right):

https://codereview.webrtc.org/2288223002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_rate_allocator.cc:54: for (; layer
--> 0;) {
On 2016/08/30 13:27:24, språng wrote:
> On 2016/08/29 21:32:09, noahric wrote:
> > Can you rewrite this? As written, the code looks wrong, because it's
> non-obvious
> > that the first loop decrements layer from the last loop.
> > 
> > This would be a bit clearer:
> > 
> > // <existing comment>
> > // The active layer starts with layer (above) - 1, because the loop always
> steps
> > // past the last active layer.
> > for (size_t active_layer = layer - 1; active_layer >= 0; --active_layer) {
> > }
> 
> Aww. I was was aware this was going to be shot down, I just like the "i --> 0"
> syntax as it intuitively looks like "as i approaches 0". But yeah, how it
> actually works isn't really that intuitive :)
> Also, your suggestion won't work. size_t is unsigned and so active_layer will
> always be >= 0. I'll test for equality to static_cast<size_t>(-1), which is
> kinda ugly but correct. Post-decrementing in the test is the other alternative
> as far as I can see.

Ah, yeah :( You can also just make active_layer an int, and optionally add a
comment for why it has to be signed. Up to you, though.

[sprang_webrtc, 2016-08-31 09:41:07]

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

[sprang_webrtc, 2016-08-31 09:41:07]

+mflodman

[sprang_webrtc, 2016-09-01 07:47:14]

Reverted to old allocation strategy (don't spill over to vga/qvga) for now.
Let's enable this again once we have improved full stack tests with better
simulcast support, so that we have a better idea of performance implications.

mflodman ptal

[sprang_webrtc, 2016-09-05 12:20:23]

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

[sprang_webrtc, 2016-09-05 12:20:24]

-mflodman +stefan

[sprang_webrtc, 2016-09-06 15:42:52]

ping?

[sprang_webrtc, 2016-09-07 09:51:27]

sprang@webrtc.org changed reviewers:
+ asapersson@webrtc.org
- stefan@webrtc.org

[sprang_webrtc, 2016-09-07 09:51:28]

-stefan +asapersson

[åsapersson, 2016-09-07 15:35:33]

lgtm

[sprang_webrtc, 2016-09-07 15:46:55]

Description was changed from

==========
Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

It also changes the allocation logic somewhat. Now, if the top layer has been
saturated, any remaining bitrate will be filled also into the lower layers.


BUG=webrtc:5206
==========

to

==========
Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

BUG=webrtc:5206
==========

[sprang_webrtc, 2016-09-07 15:47:00]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-09-07 15:47:01]

The patchset sent to the CQ was uploaded after l-g-t-m from noahric@chromium.org
Link to the patchset: https://codereview.webrtc.org/2288223002/#ps80001 (title:
"Revert to old allocation strategy")

[commit-bot: I haz the power, 2016-09-07 15:47:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-07 15:57:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 15:57:01]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[sprang_webrtc, 2016-09-07 17:31:38]

sprang@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[sprang_webrtc, 2016-09-07 17:31:38]

+kjellander as owner for BUILD.gn
Any chance you could update owners for GN to match what we had for GYP?

[kjellander_webrtc, 2016-09-08 06:13:12]

On 2016/09/07 17:31:38, språng wrote:
> +kjellander as owner for BUILD.gn
> Any chance you could update owners for GN to match what we had for GYP?

GYP/GN: lgtm

Me being an owner of BUILD.gn doesn't mean that I have to review all BUILD.gn
changes (approval from a regular owner in a directory is good for all file
types).
That said, I think we've reached the point of having everyone being able to
change all GN files, similar to what we had for GYP, so I'll change that rule in
a CL.

[kjellander_webrtc, 2016-09-08 06:57:05]

On 2016/09/08 06:13:12, kjellander_webrtc wrote:
> On 2016/09/07 17:31:38, språng wrote:
> > +kjellander as owner for BUILD.gn
> > Any chance you could update owners for GN to match what we had for GYP?
> 
> GYP/GN: lgtm
> 
> Me being an owner of BUILD.gn doesn't mean that I have to review all BUILD.gn
> changes (approval from a regular owner in a directory is good for all file
> types).
> That said, I think we've reached the point of having everyone being able to
> change all GN files, similar to what we had for GYP, so I'll change that rule
in
> a CL.

CL up: https://codereview.webrtc.org/2320043002/

[sprang_webrtc, 2016-09-12 07:54:04]

On 2016/09/08 06:57:05, kjellander_webrtc wrote:
> On 2016/09/08 06:13:12, kjellander_webrtc wrote:
> > On 2016/09/07 17:31:38, språng wrote:
> > > +kjellander as owner for BUILD.gn
> > > Any chance you could update owners for GN to match what we had for GYP?
> > 
> > GYP/GN: lgtm
> > 
> > Me being an owner of BUILD.gn doesn't mean that I have to review all
BUILD.gn
> > changes (approval from a regular owner in a directory is good for all file
> > types).
> > That said, I think we've reached the point of having everyone being able to
> > change all GN files, similar to what we had for GYP, so I'll change that
rule
> in
> > a CL.
> 
> CL up: https://codereview.webrtc.org/2320043002/

Right, I was just reflecting that webrtc/modules/BUILD.gn covered basically all
of the modules codes, and the only owners for that build file was you and the
root owners.
Great that all have access now, thanks!

[sprang_webrtc, 2016-09-12 07:54:09]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-09-12 07:54:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org, noahric@chromium.org
Link to the patchset: https://codereview.webrtc.org/2288223002/#ps100001 (title:
"Rebase")

[commit-bot: I haz the power, 2016-09-12 07:54:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-12 08:01:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 08:01:23]

Try jobs failed on following builders:
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/18139)

[sprang_webrtc, 2016-09-12 12:39:11]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-09-12 12:39:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
noahric@chromium.org, asapersson@webrtc.org, kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2288223002/#ps120001 (title:
"Rebase, handle pause case in simulcast wrapper")

[commit-bot: I haz the power, 2016-09-12 12:39:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-12 13:33:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 13:33:41]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[commit-bot: I haz the power, 2016-09-12 14:04:59]

Description was changed from

==========
Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

BUG=webrtc:5206
==========

to

==========
Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

BUG=webrtc:5206
R=asapersson@webrtc.org, kjellander@webrtc.org, noahric@chromium.org

Committed: https://crrev.com/78ce619a0cd871a769b5c3892dc930df10dfa253
Cr-Commit-Position: refs/heads/master@{#14186}
==========

[sprang_webrtc, 2016-09-12 14:05:00]

Description was changed from

==========
Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

BUG=webrtc:5206
R=asapersson@webrtc.org, kjellander@webrtc.org, noahric@chromium.org

Committed: https://crrev.com/78ce619a0cd871a769b5c3892dc930df10dfa253
Cr-Commit-Position: refs/heads/master@{#14186}
==========

to

==========
Extract simulcast rate allocation outside of video encoder.

This is a first step to refactor this code.
I'm deprecating https://codereview.webrtc.org/1913073002 and
implementing this in smaller more isolated steps.

BUG=webrtc:5206
R=asapersson@webrtc.org, kjellander@webrtc.org, noahric@chromium.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/78ce619a0cd871a769b5c3892...
==========

[commit-bot: I haz the power, 2016-09-12 14:05:01]

Patchset 7 (id:??) landed as
https://crrev.com/78ce619a0cd871a769b5c3892dc930df10dfa253
Cr-Commit-Position: refs/heads/master@{#14186}

[sprang_webrtc, 2016-09-12 14:05:03]

Committed patchset #7 (id:120001) manually as
78ce619a0cd871a769b5c3892dc930df10dfa253 (presubmit successful).