BWE allocation strategy

BWE allocation strategy allows controlling of bitrate allocation with WEBRTC external logic.

This CL implements the main logic and IOS appRTC integration.

Unit tests and Android appRTC will be in separate CL.

BUG=webrtc:8243

[alexnarest, 2017-08-08 15:08:23]

Description was changed from

==========
BWE allocation strategy

BUG=None
==========

to

==========
BWE allocation strategy allows controlling of bitrate allocation with WEBRTC
external logic.

BUG=None
==========

[alexnarest, 2017-08-11 11:06:50]

Description was changed from

==========
BWE allocation strategy allows controlling of bitrate allocation with WEBRTC
external logic.

BUG=None
==========

to

==========
BWE allocation strategy allows controlling of bitrate allocation with WEBRTC
external logic.

This CL implements the main logic and IOS appRTC integration.

Unit tests and Android appRTC will be in separate CL.

BUG=None
==========

[alexnarest, 2017-08-11 11:09:32]

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

[stefan-webrtc, 2017-08-14 07:46:03]

I've taken a look at the high level and have some early comments.

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:765: virtual RTCError
SetBitrateAllocationStrategy(
I would prefer if we could pass the allocation strategy to PeerConnection and
Call upon creation via the constructor instead of using a setter. It's easier to
see life time, and doesn't require locks.

It should be doable by adding an argument to this method:
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/peerconnectionfact...

WDYT?

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:459: virtual void
AccountForAudioPackets(bool account_for_audio) = 0;
SetAccountForAudioPackets

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/sdk/objc/Framework...
File
webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCBitrateAllocationStrategy.mm
(right):

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCBitrateAllocationStrategy.mm:2:
*  Copyright 2015 The WebRTC project authors. All Rights Reserved.
2017

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:324: void AccountForAudioPackets(bool
account_for_audio) override {
I don't think this should ever be called as it's a proxy that only RtpRtcp uses
to access the real RtpPacketSender once it's available. RtpRtcp never calls this
method.

Maybe change to RTC_NOTREACHED() instead?

[alexnarest, 2017-08-15 06:02:37]

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/api/peerconnection...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/api/peerconnection...
webrtc/api/peerconnectioninterface.h:765: virtual RTCError
SetBitrateAllocationStrategy(
On 2017/08/14 07:46:03, stefan-webrtc wrote:
> I would prefer if we could pass the allocation strategy to PeerConnection and
> Call upon creation via the constructor instead of using a setter. It's easier
to
> see life time, and doesn't require locks.
> 
> It should be doable by adding an argument to this method:
>
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/peerconnectionfact...
> 
> WDYT?

Helper would let users to change strategies during a call. For example audio
only hangout session may not need a custom strategy but as soon as video/screen
share tracks are added it may want to change allocations. Having strategy
allocated in constructor will require users to implement allocations even for
the cases the default implementation is ok.

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right):

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:459: virtual void
AccountForAudioPackets(bool account_for_audio) = 0;
On 2017/08/14 07:46:03, stefan-webrtc wrote:
> SetAccountForAudioPackets

Done.

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/sdk/objc/Framework...
File
webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCBitrateAllocationStrategy.mm
(right):

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCBitrateAllocationStrategy.mm:2:
*  Copyright 2015 The WebRTC project authors. All Rights Reserved.
On 2017/08/14 07:46:03, stefan-webrtc wrote:
> 2017

Done.

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2996643002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:324: void AccountForAudioPackets(bool
account_for_audio) override {
On 2017/08/14 07:46:03, stefan-webrtc wrote:
> I don't think this should ever be called as it's a proxy that only RtpRtcp
uses
> to access the real RtpPacketSender once it's available. RtpRtcp never calls
this
> method.
> 
> Maybe change to RTC_NOTREACHED() instead?

Done.

[stefan-webrtc, 2017-08-17 13:49:43]

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

[stefan-webrtc, 2017-08-17 13:49:44]

Nisse, would you mind helping out with the review of this?

[nisse-webrtc, 2017-08-22 11:46:06]

I've had a first look at webrtc/rtc_base/bitrateallocationstrategy.h. I think I
need to understand the interfaces before I can give any useful feedback on the
rest of the code.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:34: TrackConfig(const TrackConfig&
track_config)
Use "= default" for the copy constructor, if possible.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:44: std::string track_id;
Track ids are unclear to me. How are they assigned, and at which level do they
belong?

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:49: typedef std::map<std::string,
TrackConfig> TrackConfigs;
Seems redundant to have track_ids inside TrackConfig, and also as keys in this
mapping. If the ids are just to associate passed in config with returned
allocation, it would be simpler to pass and return vectors, and drop the ids.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:56: : public
RefCountedObject<BitrateAllocationStrategyConfig> {
This inheritance is puzzling to me, a strategy and a strategy conf sounds like
very different things.

I would have expected a BitrateAllocationStrategyInterface, one or two
implementations of that interface, and perhaps some convenience base class in
between, for code sharing.

[alexnarest, 2017-08-22 14:52:18]

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:34: TrackConfig(const TrackConfig&
track_config)
On 2017/08/22 11:46:05, nisse-webrtc wrote:
> Use "= default" for the copy constructor, if possible.

Done.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:44: std::string track_id;
On 2017/08/22 11:46:06, nisse-webrtc wrote:
> Track ids are unclear to me. How are they assigned, and at which level do they
> belong?

Track IDs are assigned by the app, for example please look at
https://cs.corp.google.com/piper///depot/google3/third_party/webrtc/files/sta....

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:49: typedef std::map<std::string,
TrackConfig> TrackConfigs;
On 2017/08/22 11:46:05, nisse-webrtc wrote:
> Seems redundant to have track_ids inside TrackConfig, and also as keys in this
> mapping. If the ids are just to associate passed in config with returned
> allocation, it would be simpler to pass and return vectors, and drop the ids.

It can be done with the vectors but the map saves few lines at
https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
line 276

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:56: : public
RefCountedObject<BitrateAllocationStrategyConfig> {
On 2017/08/22 11:46:06, nisse-webrtc wrote:
> This inheritance is puzzling to me, a strategy and a strategy conf sounds like
> very different things.
> 
> I would have expected a BitrateAllocationStrategyInterface, one or two
> implementations of that interface, and perhaps some convenience base class in
> between, for code sharing.

Will renaming BitrateAllocationStrategyConfig to
BitrateAllocationStrategyInterface and BitrateAllocationStrategy to
BitrateAllocationStrategyBase make things clear?

[nisse-webrtc, 2017-08-22 15:23:34]

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:272: (std::stringstream()
Converting a pointer to a string and using it as id seems awkward. Can't we
leave the id empty if the application didn't assign it?

Also use of std::stringstream is discouraged.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:276: track_configs[observer_config.track_id] =
Could we rearrange the code to eliminate this copying into track_configs? Can we
rearrange things so that a reference to |bitrate_observer_configs_| is passed
directly to ->AllocateBitrates below? 

To me, it seems that using vectors rather than mappings as input and output to
AllocateBitrates would make things simpler.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:49: typedef std::map<std::string,
TrackConfig> TrackConfigs;
On 2017/08/22 14:52:17, alexnarest wrote:
> On 2017/08/22 11:46:05, nisse-webrtc wrote:
> > Seems redundant to have track_ids inside TrackConfig, and also as keys in
this
> > mapping. If the ids are just to associate passed in config with returned
> > allocation, it would be simpler to pass and return vectors, and drop the
ids.
> 
> It can be done with the vectors but the map saves few lines at
>
https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
> line 276

Not sure. See my comments on that place.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:56: : public
RefCountedObject<BitrateAllocationStrategyConfig> {
On 2017/08/22 14:52:17, alexnarest wrote:
> On 2017/08/22 11:46:06, nisse-webrtc wrote:
> > This inheritance is puzzling to me, a strategy and a strategy conf sounds
like
> > very different things.
> > 
> > I would have expected a BitrateAllocationStrategyInterface, one or two
> > implementations of that interface, and perhaps some convenience base class
in
> > between, for code sharing.
> 
> Will renaming BitrateAllocationStrategyConfig to
> BitrateAllocationStrategyInterface and BitrateAllocationStrategy to
> BitrateAllocationStrategyBase make things clear?

Sounds good. But don't do the Base class unless there really is a lot code it
makes sense to share between implementations of the interface.

[alexnarest, 2017-08-31 18:40:27]

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:272: (std::stringstream()
On 2017/08/22 15:23:33, nisse-webrtc wrote:
> Converting a pointer to a string and using it as id seems awkward. Can't we
> leave the id empty if the application didn't assign it?
> 
> Also use of std::stringstream is discouraged.

Track ID should not be empty because strategies do not know observers and the
track ID is the only way for them address a specific track.

Track ID uniqueness check added and pointer is not used.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:276: track_configs[observer_config.track_id] =
On 2017/08/22 15:23:33, nisse-webrtc wrote:
> Could we rearrange the code to eliminate this copying into track_configs? Can
we
> rearrange things so that a reference to |bitrate_observer_configs_| is passed
> directly to ->AllocateBitrates below? 
> 
> To me, it seems that using vectors rather than mappings as input and output to
> AllocateBitrates would make things simpler.

Done.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:49: typedef std::map<std::string,
TrackConfig> TrackConfigs;
On 2017/08/22 15:23:34, nisse-webrtc wrote:
> On 2017/08/22 14:52:17, alexnarest wrote:
> > On 2017/08/22 11:46:05, nisse-webrtc wrote:
> > > Seems redundant to have track_ids inside TrackConfig, and also as keys in
> this
> > > mapping. If the ids are just to associate passed in config with returned
> > > allocation, it would be simpler to pass and return vectors, and drop the
> ids.
> > 
> > It can be done with the vectors but the map saves few lines at
> >
>
https://codereview.webrtc.org/2996643002/diff/100001/webrtc/call/bitrate_allo...
> > line 276
> 
> Not sure. See my comments on that place.

Done.

https://codereview.webrtc.org/2996643002/diff/100001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:56: : public
RefCountedObject<BitrateAllocationStrategyConfig> {
On 2017/08/22 15:23:34, nisse-webrtc wrote:
> On 2017/08/22 14:52:17, alexnarest wrote:
> > On 2017/08/22 11:46:06, nisse-webrtc wrote:
> > > This inheritance is puzzling to me, a strategy and a strategy conf sounds
> like
> > > very different things.
> > > 
> > > I would have expected a BitrateAllocationStrategyInterface, one or two
> > > implementations of that interface, and perhaps some convenience base class
> in
> > > between, for code sharing.
> > 
> > Will renaming BitrateAllocationStrategyConfig to
> > BitrateAllocationStrategyInterface and BitrateAllocationStrategy to
> > BitrateAllocationStrategyBase make things clear?
> 
> Sounds good. But don't do the Base class unless there really is a lot code it
> makes sense to share between implementations of the interface.

Done.

[alexnarest, 2017-09-04 15:46:08]

TrackAllocations changed to vector.

[stefan-webrtc, 2017-09-05 10:42:48]

https://codereview.webrtc.org/2996643002/diff/200001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2996643002/diff/200001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:238: // Audio BWE is enabled
End with .

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:270: allocation[observer_config->observer] =
*track_allocations_it++;
Increment on a separate line for better readability.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDBitrateAllocationStrategy.mm (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDBitrateAllocationStrategy.mm:25:
std::string(audioTrackId.UTF8String),
These lines look too long.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:1291: rtc::Thread* worker_thread =
factory_->worker_thread();
Do we have to invoke SetBitrateAllocationStrategy on the worker thread if we
anyway make sure to do all the work on the worker_queue_ inside that method?

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:28:
BitrateAllocationStrategy::DistributeBitratesEvenly(
This code is coming from BitrateAllocator, right? Just so that I know whether to
review it in detail or not.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:24: // Plugable strategy allows
configuration of bitrate allocation per media track
End with .

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/sdk/objc/Framewor...
File
webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCBitrateAllocationStrategy.mm
(right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCBitrateAllocationStrategy.mm:20:
-
(instancetype)initWith:(rtc::scoped_refptr<rtc::BitrateAllocationStrategy>)strategy
{
Line length. Or perhaps longer lines are allowed in the objc style guide?

[nisse-webrtc, 2017-09-05 11:03:46]

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:92: ObserverConfig* config =
static_cast<ObserverConfig*>(track_config.get());
It not so nice to have these static casts everywhere bitrate_observer_configs_
is accessed. If it is safe, then you should be able to change the type of
|bitrate_observer_configs_| to a std::vector<std::unique_ptr<ObserverConfig>>.

Hmm, or would you need to convert it to a
std::vector<std::unique_ptr<TrackConfig>> elsewhere? If there's a problem with
that, I think we should consult one one of our C++ experts to see if we can find
a reasonable way to avoid the type casts.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:262: bitrate, bitrate_observer_configs_);
You could consider using ArrayView as argument type for AllocateBitrates (see
rtc_base/array_view.h). Not sure whether or not that would help with the typing,
though.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.h:121: void SetAccountForAudioPackets(bool
account_for_audio) override;
Unclear to me how this change is related to the BWE strategy?

It's preferable to split out independent changes to their own cls, to ease
review and testing, but I don't insist that you do that here.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:47: typedef
std::vector<std::unique_ptr<TrackConfig>> TrackConfigs;
It's recommended to avoid convenience aliases. See go/totw/137

[alexnarest, 2017-09-05 13:37:44]

alexnarest@webrtc.org changed reviewers:
+ deadbeef@webrtc.org

[alexnarest, 2017-09-05 13:37:46]

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:92: ObserverConfig* config =
static_cast<ObserverConfig*>(track_config.get());
On 2017/09/05 11:03:46, nisse-webrtc wrote:
> It not so nice to have these static casts everywhere bitrate_observer_configs_
> is accessed. If it is safe, then you should be able to change the type of
> |bitrate_observer_configs_| to a std::vector<std::unique_ptr<ObserverConfig>>.
> 
> Hmm, or would you need to convert it to a
> std::vector<std::unique_ptr<TrackConfig>> elsewhere? If there's a problem with
> that, I think we should consult one one of our C++ experts to see if we can
find
> a reasonable way to avoid the type casts.

TrackConfig is visible at BitrateAllocationStrategy and does not have observer
field. ObserverConfig inherits TrackConfig to reuse common fields and add
observer. The collection bitrate_observer_configs_ is passed to
BitrateAllocationStrategy as it is. This is the reason the collection is
TrackConfig based.
I'm not aware of a way to downcast without a cast.

We could hide cast into some utility class that would implement the conversion
but it still would cast. I do not think this would be nice to hide the cast.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:262: bitrate, bitrate_observer_configs_);
On 2017/09/05 11:03:46, nisse-webrtc wrote:
> You could consider using ArrayView as argument type for AllocateBitrates (see
> rtc_base/array_view.h). Not sure whether or not that would help with the
typing,
> though.

ArrayView does not implement the needed conversion

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:270: allocation[observer_config->observer] =
*track_allocations_it++;
On 2017/09/05 10:42:47, stefan-webrtc wrote:
> Increment on a separate line for better readability.

git CL format does not like it on the separate line and LINT Is getting
confused, so I'll keep it as format likes it.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/examples/objc/App...
File webrtc/examples/objc/AppRTCMobile/ARDBitrateAllocationStrategy.mm (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/examples/objc/App...
webrtc/examples/objc/AppRTCMobile/ARDBitrateAllocationStrategy.mm:25:
std::string(audioTrackId.UTF8String),
On 2017/09/05 10:42:47, stefan-webrtc wrote:
> These lines look too long.

git CL format and presubmit checks did not complain about it. Probably it is ok.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/paced_sender.h (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/paced_sender.h:121: void SetAccountForAudioPackets(bool
account_for_audio) override;
On 2017/09/05 11:03:46, nisse-webrtc wrote:
> Unclear to me how this change is related to the BWE strategy?
> 
> It's preferable to split out independent changes to their own cls, to ease
> review and testing, but I don't insist that you do that here.

The strategy allows us to enable audio BWE for video calls and as soon as it
will be enabled it will make sense to account for audio packets.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/pc/peerconnection.cc
File webrtc/pc/peerconnection.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/pc/peerconnection...
webrtc/pc/peerconnection.cc:1291: rtc::Thread* worker_thread =
factory_->worker_thread();
On 2017/09/05 10:42:47, stefan-webrtc wrote:
> Do we have to invoke SetBitrateAllocationStrategy on the worker thread if we
> anyway make sure to do all the work on the worker_queue_ inside that method?

It looks like worker_thread and worker_queue_ are two different things. It could
be worker_thread is still needed here because of RTC_DCHECK(call_.get())

Probably deadbeef@ will know better.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:28:
BitrateAllocationStrategy::DistributeBitratesEvenly(
On 2017/09/05 10:42:47, stefan-webrtc wrote:
> This code is coming from BitrateAllocator, right? Just so that I know whether
to
> review it in detail or not.

It is not copy/paste but the logic is similar.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:24: // Plugable strategy allows
configuration of bitrate allocation per media track
On 2017/09/05 10:42:48, stefan-webrtc wrote:
> End with .

Done.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:47: typedef
std::vector<std::unique_ptr<TrackConfig>> TrackConfigs;
On 2017/09/05 11:03:46, nisse-webrtc wrote:
> It's recommended to avoid convenience aliases. See go/totw/137

I agree about TrackAllocations and removed it.
I think TrackConfigs somewhere on the borderline  and would prefer to keep it if
you do not mind.

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/sdk/objc/Framewor...
File
webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCBitrateAllocationStrategy.mm
(right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/PeerConnection/RTCBitrateAllocationStrategy.mm:20:
-
(instancetype)initWith:(rtc::scoped_refptr<rtc::BitrateAllocationStrategy>)strategy
{
On 2017/09/05 10:42:48, stefan-webrtc wrote:
> Line length. Or perhaps longer lines are allowed in the objc style guide?

Seems OK for Obj-C

[kwiberg-webrtc, 2017-09-06 08:49:46]

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:92: ObserverConfig* config =
static_cast<ObserverConfig*>(track_config.get());
On 2017/09/05 13:37:45, alexnarest wrote:
> On 2017/09/05 11:03:46, nisse-webrtc wrote:
> > It not so nice to have these static casts everywhere
bitrate_observer_configs_
> > is accessed. If it is safe, then you should be able to change the type of
> > |bitrate_observer_configs_| to a
std::vector<std::unique_ptr<ObserverConfig>>.
> > 
> > Hmm, or would you need to convert it to a
> > std::vector<std::unique_ptr<TrackConfig>> elsewhere? If there's a problem
with
> > that, I think we should consult one one of our C++ experts to see if we can
> find
> > a reasonable way to avoid the type casts.
> 
> TrackConfig is visible at BitrateAllocationStrategy and does not have observer
> field. ObserverConfig inherits TrackConfig to reuse common fields and add
> observer. The collection bitrate_observer_configs_ is passed to
> BitrateAllocationStrategy as it is. This is the reason the collection is
> TrackConfig based.
> I'm not aware of a way to downcast without a cast.
> 
> We could hide cast into some utility class that would implement the conversion
> but it still would cast. I do not think this would be nice to hide the cast.

+1 to what nisse@ said about downcasting. It's really nice to not have to do it.
Also, having a unique_ptr<BitrateAllocationStrategy::TrackConfig> owning an
ObserverConfig is not safe, since BitrateAllocationStrategy::TrackConfig's
destructor is not virtual; the unique_ptr will destroy just the base class, not
the derived class. Currently, it happens to be the case that the fields added by
ObserverConfig are all trivially destructible so that it works anyway, but as
soon as a non-trivially destructible field is added, its destructor will
silently not get called.

I would propose that bitrate_observer_configs_ should be e.g. an
std::vector<ObserverConfig>; you can then *up*cast pointers to its elements to
BitrateAllocationStrategy::TrackConfig* when you need to.

[nisse-webrtc, 2017-09-06 09:04:02]

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:92: ObserverConfig* config =
static_cast<ObserverConfig*>(track_config.get());
On 2017/09/06 08:49:46, kwiberg-webrtc wrote:
> On 2017/09/05 13:37:45, alexnarest wrote:
> > On 2017/09/05 11:03:46, nisse-webrtc wrote:
> > > It not so nice to have these static casts everywhere
> bitrate_observer_configs_
> > > is accessed. If it is safe, then you should be able to change the type of
> > > |bitrate_observer_configs_| to a
> std::vector<std::unique_ptr<ObserverConfig>>.
> > > 
> > > Hmm, or would you need to convert it to a
> > > std::vector<std::unique_ptr<TrackConfig>> elsewhere? If there's a problem
> with
> > > that, I think we should consult one one of our C++ experts to see if we
can
> > find
> > > a reasonable way to avoid the type casts.
> > 
> > TrackConfig is visible at BitrateAllocationStrategy and does not have
observer
> > field. ObserverConfig inherits TrackConfig to reuse common fields and add
> > observer. The collection bitrate_observer_configs_ is passed to
> > BitrateAllocationStrategy as it is. This is the reason the collection is
> > TrackConfig based.
> > I'm not aware of a way to downcast without a cast.
> > 
> > We could hide cast into some utility class that would implement the
conversion
> > but it still would cast. I do not think this would be nice to hide the cast.
> 
> +1 to what nisse@ said about downcasting. It's really nice to not have to do
it.
> Also, having a unique_ptr<BitrateAllocationStrategy::TrackConfig> owning an
> ObserverConfig is not safe, since BitrateAllocationStrategy::TrackConfig's
> destructor is not virtual; the unique_ptr will destroy just the base class,
not
> the derived class. Currently, it happens to be the case that the fields added
by
> ObserverConfig are all trivially destructible so that it works anyway, but as
> soon as a non-trivially destructible field is added, its destructor will
> silently not get called.
> 
> I would propose that bitrate_observer_configs_ should be e.g. an
> std::vector<ObserverConfig>; you can then *up*cast pointers to its elements to
> BitrateAllocationStrategy::TrackConfig* when you need to.

And we'd then need to change the signature of the AllocateBitrates method. I see
two reasonable options:

1. Take an ArrayView<const TrackConfig*>. Has the drawback that we'd 
   need to construct an array or vector of pointers, either for each
   call, or cached in some way. But at least we wouldn't need to copy
   the objects.

2. A pointer to an interface a method GetTrackConfig(int index), 
   which could be implemented by returning a pointer directly into
   the std::Vector<ObserverConfig>.

There's also a third alternative: keep a std::vector<TrackConfig>, pass a
reference to it as an ArrayView<TrackConfig>, and use a *separate* vector for
storing the additional per-track information.

https://codereview.webrtc.org/2996643002/diff/280001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/280001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:25: class BitrateAllocationStrategy
: public RefCountedObject<RefCountInterface> {
Just spotted this, you should not inherit RefCountedObject, if you do, that
blocks cl https://codereview.webrtc.org/3007973002/ which tries to mark it as a
final class.

Also, not sure this really needs to be refcounted at all. Shouldn't it be owned
by the application, to manage ownership in any way it sees fit?

[alexnarest, 2017-09-08 17:09:23]

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/240001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:92: ObserverConfig* config =
static_cast<ObserverConfig*>(track_config.get());
On 2017/09/06 09:04:02, nisse-webrtc wrote:
> On 2017/09/06 08:49:46, kwiberg-webrtc wrote:
> > On 2017/09/05 13:37:45, alexnarest wrote:
> > > On 2017/09/05 11:03:46, nisse-webrtc wrote:
> > > > It not so nice to have these static casts everywhere
> > bitrate_observer_configs_
> > > > is accessed. If it is safe, then you should be able to change the type
of
> > > > |bitrate_observer_configs_| to a
> > std::vector<std::unique_ptr<ObserverConfig>>.
> > > > 
> > > > Hmm, or would you need to convert it to a
> > > > std::vector<std::unique_ptr<TrackConfig>> elsewhere? If there's a
problem
> > with
> > > > that, I think we should consult one one of our C++ experts to see if we
> can
> > > find
> > > > a reasonable way to avoid the type casts.
> > > 
> > > TrackConfig is visible at BitrateAllocationStrategy and does not have
> observer
> > > field. ObserverConfig inherits TrackConfig to reuse common fields and add
> > > observer. The collection bitrate_observer_configs_ is passed to
> > > BitrateAllocationStrategy as it is. This is the reason the collection is
> > > TrackConfig based.
> > > I'm not aware of a way to downcast without a cast.
> > > 
> > > We could hide cast into some utility class that would implement the
> conversion
> > > but it still would cast. I do not think this would be nice to hide the
cast.
> > 
> > +1 to what nisse@ said about downcasting. It's really nice to not have to do
> it.
> > Also, having a unique_ptr<BitrateAllocationStrategy::TrackConfig> owning an
> > ObserverConfig is not safe, since BitrateAllocationStrategy::TrackConfig's
> > destructor is not virtual; the unique_ptr will destroy just the base class,
> not
> > the derived class. Currently, it happens to be the case that the fields
added
> by
> > ObserverConfig are all trivially destructible so that it works anyway, but
as
> > soon as a non-trivially destructible field is added, its destructor will
> > silently not get called.
> > 
> > I would propose that bitrate_observer_configs_ should be e.g. an
> > std::vector<ObserverConfig>; you can then *up*cast pointers to its elements
to
> > BitrateAllocationStrategy::TrackConfig* when you need to.
> 
> And we'd then need to change the signature of the AllocateBitrates method. I
see
> two reasonable options:
> 
> 1. Take an ArrayView<const TrackConfig*>. Has the drawback that we'd 
>    need to construct an array or vector of pointers, either for each
>    call, or cached in some way. But at least we wouldn't need to copy
>    the objects.
> 
> 2. A pointer to an interface a method GetTrackConfig(int index), 
>    which could be implemented by returning a pointer directly into
>    the std::Vector<ObserverConfig>.
> 
> There's also a third alternative: keep a std::vector<TrackConfig>, pass a
> reference to it as an ArrayView<TrackConfig>, and use a *separate* vector for
> storing the additional per-track information. 

I'm not fully agree with the tradeoff between performance (creation of the array
of pointers on every call) and code beauty (static_cast) but will accept your
suggestion because in my view the potential impact is small either way.

https://codereview.webrtc.org/2996643002/diff/280001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/280001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:25: class BitrateAllocationStrategy
: public RefCountedObject<RefCountInterface> {
On 2017/09/06 09:04:02, nisse-webrtc wrote:
> Just spotted this, you should not inherit RefCountedObject, if you do, that
> blocks cl https://codereview.webrtc.org/3007973002/ which tries to mark it as
a
> final class.
> 
> Also, not sure this really needs to be refcounted at all. Shouldn't it be
owned
> by the application, to manage ownership in any way it sees fit?

I think it is more convenient to app to create the strategy and pass it to API
rather than maintain it explicitly by setting strategy to null before releasing
it.  App does not have a lot to do with the strategy on its own.

But it is too small to discuss, I removed ref pointer and added setting to null
on disconnect.

[Taylor Brandstetter, 2017-09-11 16:55:07]

I think the code could use some more comments, but overall this looks good.

A couple process-related comments: can you add unique descriptions when you
upload patchsets, to help reviewers keep track of which is which? Also, I think
this is large enough that it deserves its own bug entry (instead of Bug=NONE).

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:776: // WEBRTC allocator will be used. May
be changed during an active session.
nit: "WebRTC"

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:778: virtual RTCError
SetBitrateAllocationStrategy(
nit: "Should be set to null before the strategy is destroyed."

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.cc (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:44: return track_allocations;
Where is "enforce_min_bitrate" used?

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:46: std::multimap<uint32_t, size_t>
max_bitrate_sorted_configs;
This logic is a little confusing at first glance; can you add comments where
applicable?

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:95: sufficient_audio_bitrate_ <=
audio_track_config->max_bitrate_bps);
Is it possible that this DCHECK could be hit due to certain SDP being set? If so
this situation should be handled more gracefully.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:1: /*
Not sure if this has been discussed before, but does this belong in rtc_base/ or
call/?

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:25: // Plugable strategy allows
configuration of bitrate allocation per media track.
Would be nice to have a more detailed explanation here.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:27: public:
Can you document the arguments to TrackConfig's constructor? I assume
"enforce_min_bitrate" means "must allocate at least min_bitrate_bps", and
"track_id" is a MediaStreamTrack ID, but it would be help if this were
clarified.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:51: uint32_t available_bitrate);
If these methods aren't virtual, do they need to be here, or could they be
implemented in Call internally? Or, if you want to re-use them elsewhere, they
could just be BitrateAllocationStrategy subclasses.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:53: virtual std::vector<uint32_t>
AllocateBitrates(
Some comments around this would be helpful. Some questions that come to mind:
What happens if a subclass allocates more than available_bitrate? Or less than
it? Or violates the minimum, when enforce_min_bitrate is true?

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:60: // Simple allocation strategy
giving priority to audio at low bitrates.
Can this comment elaborate a bit more? I'd summarize it as "prioritize audio
bitrate 100% until sufficient_audio_bitrate is reached, then follow normal
bitrate allocation algorithm".

[nisse-webrtc, 2017-09-12 10:39:05]

Looks pretty good now. I'd like to see the additional documentation suggested by
Taylor.

I have one question about the type used to represent a bit rate, we use uint32_t
almost everywhere, except that call/bitrate_allocator.{h,cc} uses int in a few
places. Stefan, do you know if there's any good reason for one or the other, or
if some decision has been made on which type to prefer?

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:778: virtual RTCError
SetBitrateAllocationStrategy(
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> nit: "Should be set to null before the strategy is destroyed."

Maybe these comments could make ownership clearer. As far as I understand, the
current version of this cl, ownership of the strategy stays with the
application, and the application is responsible for keeping it alive as long as
any of its peerconnections are using it.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:254: track_configs;
Consider allocating a vector of size bitrate_observer_configs_.size() up-front,
instead of growing it an element at a time. But don't do that now, if it makes
the code longer or harder to read. There's also std::transform, but unclear to
me how useful that is.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:262: ObserverAllocation allocation;
Not for this cl to fix, but I dislike both the ObserverAllocation alias, and the
use of a mapping. I think BitrateAllocator::AllocateBitrates could just return a
vector of integer rates.

[alexnarest, 2017-09-13 15:15:03]

Description was changed from

==========
BWE allocation strategy allows controlling of bitrate allocation with WEBRTC
external logic.

This CL implements the main logic and IOS appRTC integration.

Unit tests and Android appRTC will be in separate CL.

BUG=None
==========

to

==========
BWE allocation strategy allows controlling of bitrate allocation with WEBRTC
external logic.

This CL implements the main logic and IOS appRTC integration.

Unit tests and Android appRTC will be in separate CL.

BUG=8243
==========

[alexnarest, 2017-09-14 13:08:34]

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:776: // WEBRTC allocator will be used. May
be changed during an active session.
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> nit: "WebRTC"

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:778: virtual RTCError
SetBitrateAllocationStrategy(
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> nit: "Should be set to null before the strategy is destroyed."

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/api/peerconnectio...
webrtc/api/peerconnectioninterface.h:778: virtual RTCError
SetBitrateAllocationStrategy(
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> nit: "Should be set to null before the strategy is destroyed."

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:254: track_configs;
On 2017/09/12 10:39:05, nisse-webrtc wrote:
> Consider allocating a vector of size bitrate_observer_configs_.size()
up-front,
> instead of growing it an element at a time. But don't do that now, if it makes
> the code longer or harder to read. There's also std::transform, but unclear to
> me how useful that is.

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:262: ObserverAllocation allocation;
On 2017/09/12 10:39:05, nisse-webrtc wrote:
> Not for this cl to fix, but I dislike both the ObserverAllocation alias, and
the
> use of a mapping. I think BitrateAllocator::AllocateBitrates could just return
a
> vector of integer rates.

Acknowledged.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.cc (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:44: return track_allocations;
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> Where is "enforce_min_bitrate" used?

Current implementation does not pause tracks.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:46: std::multimap<uint32_t, size_t>
max_bitrate_sorted_configs;
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> This logic is a little confusing at first glance; can you add comments where
> applicable?

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:95: sufficient_audio_bitrate_ <=
audio_track_config->max_bitrate_bps);
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> Is it possible that this DCHECK could be hit due to certain SDP being set? If
so
> this situation should be handled more gracefully.

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:1: /*
On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> Not sure if this has been discussed before, but does this belong in rtc_base/
or
> call/?

I thought rtc_base/ is relevant because this provides both API and basic
implementation.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:25: // Plugable strategy allows
configuration of bitrate allocation per media track.
On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> Would be nice to have a more detailed explanation here.

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:27: public:
On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> Can you document the arguments to TrackConfig's constructor? I assume
> "enforce_min_bitrate" means "must allocate at least min_bitrate_bps", and
> "track_id" is a MediaStreamTrack ID, but it would be help if this were
> clarified.

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:51: uint32_t available_bitrate);
On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> If these methods aren't virtual, do they need to be here, or could they be
> implemented in Call internally? Or, if you want to re-use them elsewhere, they
> could just be BitrateAllocationStrategy subclasses. 

These are utility methods. Future strategies will use them similarly to
AudioPriorityBitrateAllocationStrategy.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:53: virtual std::vector<uint32_t>
AllocateBitrates(
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> Some comments around this would be helpful. Some questions that come to mind:
> What happens if a subclass allocates more than available_bitrate? Or less than
> it? Or violates the minimum, when enforce_min_bitrate is true?

Done.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:60: // Simple allocation strategy
giving priority to audio at low bitrates.
On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> Can this comment elaborate a bit more? I'd summarize it as "prioritize audio
> bitrate 100% until sufficient_audio_bitrate is reached, then follow normal
> bitrate allocation algorithm".

Done.

[Taylor Brandstetter, 2017-09-15 01:24:41]

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.cc (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:44: return track_allocations;
On 2017/09/14 13:08:33, alexnarest wrote:
> On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> > Where is "enforce_min_bitrate" used?
> 
> Current implementation does not pause tracks.

Should that be documented in a comment?

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:51: uint32_t available_bitrate);
On 2017/09/14 13:08:34, alexnarest wrote:
> On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> > If these methods aren't virtual, do they need to be here, or could they be
> > implemented in Call internally? Or, if you want to re-use them elsewhere,
they
> > could just be BitrateAllocationStrategy subclasses. 
> 
> These are utility methods. Future strategies will use them similarly to
> AudioPriorityBitrateAllocationStrategy. 

I understand. I just think it would be cleaner if they were standalone
functions, or subclasses of BitrateAllocationStrategy, so that
BitrateAllocationStrategy can be a pure virtual interface. I don't see the
reason for them being member functions.

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:258: }
nit: I think a foreach style loop would still be more readable:

int i = 0;
for (const auto& c : bitrate_observer_configs_) {
  track_configs[i++] = &c;
}

Or you could even do this if you wanna be fancy:

std::transform(bitrate_observer_configs_.begin(), 
               bitrate_observer_configs_.end(),
               track_configs.begin(),
               [](const rtc::BitrateAllocationStrategy::TrackConfig& c) { return
&c; });

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.cc (right):

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:46: // If sum_min_bitrates <
available_bitrate > sum_max_bitrates allocate
Is the ">" meant to be a "<"?

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:69: // enforce_min_bitrate set to
true are expecting to get min_bitrate_bps.
nit: Paragraph break here?

[Taylor Brandstetter, 2017-09-15 01:26:21]

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:1: /*
On 2017/09/14 13:08:34, alexnarest wrote:
> On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> > Not sure if this has been discussed before, but does this belong in
rtc_base/
> or
> > call/?
> 
> I thought rtc_base/ is relevant because this provides both API and basic
> implementation.

Something I forgot from the round of comments I just sent: Since this is exposed
from PeerConnectionInterface, it should actually go in api/. Our end goal is
make api/ self-contained (not include anything from other directories, even
rtc_base).

[nisse-webrtc, 2017-09-15 07:05:45]

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:1: /*
On 2017/09/15 01:26:21, Taylor Brandstetter wrote:
> On 2017/09/14 13:08:34, alexnarest wrote:
> > On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> > > Not sure if this has been discussed before, but does this belong in
> rtc_base/
> > or
> > > call/?
> > 
> > I thought rtc_base/ is relevant because this provides both API and basic
> > implementation.
> 
> Something I forgot from the round of comments I just sent: Since this is
exposed
> from PeerConnectionInterface, it should actually go in api/. Our end goal is
> make api/ self-contained (not include anything from other directories, even
> rtc_base).

Agreed. When moving to api/, be extra careful about good naming. In particular,
|enforce_min_bitrate| is a bit confusing. Maybe something like |can_pause| (with
opposite meaning) would be clearer? If this affects code before this cl (I think
it does), it may make things easier to do it in a separate cl, to be landed
first.

If the other reviewrs agree, it's also ok with me to leave file in rtc_base in
this cl, and do a followup cl moving to api/ and tweaking the naming and other
api cleanup.

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:51: uint32_t available_bitrate);
On 2017/09/15 01:24:41, Taylor Brandstetter wrote:
> On 2017/09/14 13:08:34, alexnarest wrote:
> > On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> > > If these methods aren't virtual, do they need to be here, or could they be
> > > implemented in Call internally? Or, if you want to re-use them elsewhere,
> they
> > > could just be BitrateAllocationStrategy subclasses. 
> > 
> > These are utility methods. Future strategies will use them similarly to
> > AudioPriorityBitrateAllocationStrategy. 
> 
> I understand. I just think it would be cleaner if they were standalone
> functions, or subclasses of BitrateAllocationStrategy, so that
> BitrateAllocationStrategy can be a pure virtual interface. I don't see the
> reason for them being member functions.

Are they intended as internal utility functions, or do we really want to
advertise them to applications?

I think it would be more appropriate with functions or static methods (a static
method is just a function which is grouped with the class for namespace
purposes).

[alexnarest, 2017-09-15 08:29:01]

On 2017/09/15 07:05:45, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
> File webrtc/rtc_base/bitrateallocationstrategy.h (right):
> 
>
https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
> webrtc/rtc_base/bitrateallocationstrategy.h:1: /*
> On 2017/09/15 01:26:21, Taylor Brandstetter wrote:
> > On 2017/09/14 13:08:34, alexnarest wrote:
> > > On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> > > > Not sure if this has been discussed before, but does this belong in
> > rtc_base/
> > > or
> > > > call/?
> > > 
> > > I thought rtc_base/ is relevant because this provides both API and basic
> > > implementation.
> > 
> > Something I forgot from the round of comments I just sent: Since this is
> exposed
> > from PeerConnectionInterface, it should actually go in api/. Our end goal is
> > make api/ self-contained (not include anything from other directories, even
> > rtc_base).
> 
> Agreed. When moving to api/, be extra careful about good naming. In
particular,
> |enforce_min_bitrate| is a bit confusing. Maybe something like |can_pause|
(with
> opposite meaning) would be clearer? If this affects code before this cl (I
think
> it does), it may make things easier to do it in a separate cl, to be landed
> first.
> 
> If the other reviewrs agree, it's also ok with me to leave file in rtc_base in
> this cl, and do a followup cl moving to api/ and tweaking the naming and other
> api cleanup.
> 
>
https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
> webrtc/rtc_base/bitrateallocationstrategy.h:51: uint32_t available_bitrate);
> On 2017/09/15 01:24:41, Taylor Brandstetter wrote:
> > On 2017/09/14 13:08:34, alexnarest wrote:
> > > On 2017/09/11 16:55:07, Taylor Brandstetter wrote:
> > > > If these methods aren't virtual, do they need to be here, or could they
be
> > > > implemented in Call internally? Or, if you want to re-use them
elsewhere,
> > they
> > > > could just be BitrateAllocationStrategy subclasses. 
> > > 
> > > These are utility methods. Future strategies will use them similarly to
> > > AudioPriorityBitrateAllocationStrategy. 
> > 
> > I understand. I just think it would be cleaner if they were standalone
> > functions, or subclasses of BitrateAllocationStrategy, so that
> > BitrateAllocationStrategy can be a pure virtual interface. I don't see the
> > reason for them being member functions.
> 
> Are they intended as internal utility functions, or do we really want to
> advertise them to applications?
> 
> I think it would be more appropriate with functions or static methods (a
static
> method is just a function which is grouped with the class for namespace
> purposes).

I think the applications implementing strategies may find these functions
useful. Changed to static methods.

[alexnarest, 2017-09-15 08:29:42]

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.cc (right):

https://codereview.webrtc.org/2996643002/diff/300001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:44: return track_allocations;
On 2017/09/15 01:24:41, Taylor Brandstetter wrote:
> On 2017/09/14 13:08:33, alexnarest wrote:
> > On 2017/09/11 16:55:06, Taylor Brandstetter wrote:
> > > Where is "enforce_min_bitrate" used?
> > 
> > Current implementation does not pause tracks.
> 
> Should that be documented in a comment?

Done.

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:258: }
On 2017/09/15 01:24:41, Taylor Brandstetter wrote:
> nit: I think a foreach style loop would still be more readable:
> 
> int i = 0;
> for (const auto& c : bitrate_observer_configs_) {
>   track_configs[i++] = &c;
> }
> 
> Or you could even do this if you wanna be fancy:
> 
> std::transform(bitrate_observer_configs_.begin(), 
>                bitrate_observer_configs_.end(),
>                track_configs.begin(),
>                [](const rtc::BitrateAllocationStrategy::TrackConfig& c) {
return
> &c; });

Done.

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.cc (right):

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.cc:46: // If sum_min_bitrates <
available_bitrate > sum_max_bitrates allocate
On 2017/09/15 01:24:41, Taylor Brandstetter wrote:
> Is the ">" meant to be a "<"?

Done.

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/rtc_base/bitratea...
File webrtc/rtc_base/bitrateallocationstrategy.h (right):

https://codereview.webrtc.org/2996643002/diff/320001/webrtc/rtc_base/bitratea...
webrtc/rtc_base/bitrateallocationstrategy.h:69: // enforce_min_bitrate set to
true are expecting to get min_bitrate_bps.
On 2017/09/15 01:24:41, Taylor Brandstetter wrote:
> nit: Paragraph break here?

Done.

[Taylor Brandstetter, 2017-09-15 17:34:20]

lgtm, assuming files will be moved to api/ in a separate CL

[Taylor Brandstetter, 2017-09-15 17:35:38]

Actually, not lgtm. I just noticed that BitrateAllocationStrategy still needs
tests.

[alexnarest, 2017-09-21 08:37:17]

Description was changed from

==========
BWE allocation strategy allows controlling of bitrate allocation with WEBRTC
external logic.

This CL implements the main logic and IOS appRTC integration.

Unit tests and Android appRTC will be in separate CL.

BUG=8243
==========

to

==========
BWE allocation strategy allows controlling of bitrate allocation with WEBRTC
external logic.

This CL implements the main logic and IOS appRTC integration.

Unit tests and Android appRTC will be in separate CL.

BUG=webrtc:8243
==========