Extract common simulcast logic from VP8 wrapper and simulcast adapter

Extract common simulcast logic from VP8 wrapper and simulcast adapter

Move duplicate logic for rate allocation and sending/keyframe state to a
new SimulcastState class instead.

---------------

I'm closing this and replacing it, starting with https://codereview.webrtc.org/2288223002.

[sprang_webrtc, 2016-04-25 13:00:54]

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

[pbos-webrtc, 2016-04-29 21:23:26]

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

[pbos-webrtc, 2016-04-29 21:23:27]

Some initial feedback.

stefan@ I need a second pair of eyes on this, it's complex.

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

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:320:
stream_states_->AllocateBitrate(new_bitrate_kbit);
Shouldn't we also set the rates? I'm concerned with the number of tests not
failing right now.

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:343:
encoders_[0].first->OnDroppedFrame();
I think this should report to all encoders.

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h (right):

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.h:103:
std::unique_ptr<SimulcastState> stream_states_;
Can you stack allocate this (e.g. SimulcastState stream_states_) ?

You can still use copy constructors to reset it, right?

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc (right):

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:872: (encoders_.size() > 1) ?
simulcast_state_->NumSendingStreams() : -1;
This is not NumStreamsDisabled? Why are tests not exploding?

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/vp8_impl.h (right):

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/vp8_impl.h:103:
std::unique_ptr<SimulcastState> simulcast_state_;
stack allocate please

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_state.cc (right):

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.cc:70: for (auto stream =
streams_.begin(); stream != streams_.end(); ++stream) {
auto& stream : streams_, or auto& it

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.cc:78: // Not enough bitrate
for this stream, or for some stream before this one;
Can you do disabling in a loop outside this one? The continue; logic is very
hard to read.

for (;last_active_stream < streams.size(); ++last_active_stream) {
  disable;
}

Break this loop if allocated <= 0

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.cc:93: Stream*
highest_stream = &streams_[last_active_stream];
put &streams_[...] inside the below call

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.cc:155: int
SimulcastState::SumAllocatedBitrate() const {
Where do we actually need this?

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.cc:172: bool
SimulcastState::AnyKeyFrameRequested() const {
Is this used anywhere or should we just GetAndResetKeyframeRequest?

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_state.h (right):

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.h:27: int
AllocateBitrate(int target_bitrate_kbps);
Can we make all of these _bps instead and move in that direction?

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.h:28: void
RequestKeyFrame(uint8_t idx);
Do we ever actually use the index here? Seems to me like all implementations
check if any are set and then send keyframes for all layers.

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.h:52: int
allocated_rate_kbps;
including here

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.h:55:
std::vector<Stream>::const_iterator begin() const { return streams_.begin(); }
Replace with const std::vector<Stream>& streams() const { return streams; } so
you use it as for(stream : state->streams()), this is closer to operator
overloading which is disallowed

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/1913073002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.h:149: std::unique_ptr<SimulcastState> simulcast_rates_
GUARDED_BY(data_cs_);
stack alloc please

[stefan-webrtc, 2016-05-06 11:43:21]

https://codereview.webrtc.org/1913073002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/utility/simulcast_state.h (right):

https://codereview.webrtc.org/1913073002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/utility/simulcast_state.h:27: void
RequestKeyFrame(uint8_t idx);
Could we split this class in two and have one for rate allocation and another
one for things like key frames etc? Then we could potentially move the
allocation part out from the codecs entirely and put it inside, e.g.,
BitrateAllocator instead, which would then allocate several bitrates for a
simulcast send-stream.

[sprang_webrtc, 2016-08-29 12:37:19]

Description was changed from

==========
Extract common simulcast logic from VP8 wrapper and simulcast adapter

Move duplicate logic for rate allocation and sending/keyframe state to a
new SimulcastState class instead.

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

to

==========
Extract common simulcast logic from VP8 wrapper and simulcast adapter

Move duplicate logic for rate allocation and sending/keyframe state to a
new SimulcastState class instead.

---------------

I'm closing this and replacing it, starting with
https://codereview.webrtc.org/2288223002.
==========