Move RemoteBitrateEstimator::RemoveStream calls from receive streams to Call.

Move RemoteBitrateEstimator::RemoveStream calls from receive streams to Call.

We can then drop the CongestionController and RemoteBitrateEstimator
completely from the receive streams.

BUG=webrtc:6847
Review-Url: https://codereview.webrtc.org/2669463006
Cr-Commit-Position: refs/heads/master@{#16459}
Committed: https://chromium.googlesource.com/external/webrtc/+/4709e8971b506c69134420cde1fce8d61b6de58e

[nisse-webrtc, 2017-02-01 13:31:00]

Description was changed from

==========
Move RemoteBitrateEstimator::RemoveStream calls from receive streams to Call.

We cne then drop the CongestionController and RemoteBitrateEstimator
completely from the receive streams.

BUG=webrtc:6847
==========

to

==========
Move RemoteBitrateEstimator::RemoveStream calls from receive streams to Call.

We can then drop the CongestionController and RemoteBitrateEstimator
completely from the receive streams.

BUG=webrtc:6847
==========

[nisse-webrtc, 2017-02-01 13:36:01]

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

[nisse-webrtc, 2017-02-01 13:36:02]

This is a followup cl, moving the RemoveStream calls as well.

I also replaced the transport_cc flag in Call's ReceiveRtpConfig map with a
use_send_side_bwe flag which is set of both RTP extension and RTCP feedback is
negotiated.
 
I'm thinking, that maybe it would be better to add a method to the
CongestionController, say, OnReceiveStreamDestroyed, which calls RemoveStream on
all RemoteBitrateEstimators it knows of. There might be some cases where we're
using both receive side and send side bwe in parallel?

[brandtr, 2017-02-02 09:07:00]

lgtm

On 2017/02/01 13:36:02, nisse-webrtc wrote:
> This is a followup cl, moving the RemoveStream calls as well.
> 
> I also replaced the transport_cc flag in Call's ReceiveRtpConfig map with a
> use_send_side_bwe flag which is set of both RTP extension and RTCP feedback is
> negotiated.
>  
> I'm thinking, that maybe it would be better to add a method to the
> CongestionController, say, OnReceiveStreamDestroyed, which calls RemoveStream
on
> all RemoteBitrateEstimators it knows of. There might be some cases where we're
> using both receive side and send side bwe in parallel?

Not that I'm aware of, but check with holmer.

[stefan-webrtc, 2017-02-02 10:36:58]

On 2017/02/02 09:07:00, brandtr wrote:
> lgtm
> 
> On 2017/02/01 13:36:02, nisse-webrtc wrote:
> > This is a followup cl, moving the RemoveStream calls as well.
> > 
> > I also replaced the transport_cc flag in Call's ReceiveRtpConfig map with a
> > use_send_side_bwe flag which is set of both RTP extension and RTCP feedback
is
> > negotiated.
> >  
> > I'm thinking, that maybe it would be better to add a method to the
> > CongestionController, say, OnReceiveStreamDestroyed, which calls
RemoveStream
> on
> > all RemoteBitrateEstimators it knows of. There might be some cases where
we're
> > using both receive side and send side bwe in parallel?
> 
> Not that I'm aware of, but check with holmer.

No, this case shouldn't happen. We may use REMB in combination with
transport_cc, but we'll never use the RemoteBitrateEstimator in combination with
the RemoteEstimatorProxy.

[stefan-webrtc, 2017-02-02 10:42:52]

Great cleanup!

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

https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc#newcode77
webrtc/call/call.cc:77: bool UseSendSideBwe(const VideoReceiveStream::Config&
config) {
Maybe use a template instead?

https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc#newcode1283
webrtc/call/call.cc:1283: if (use_send_side_bwe !=
header.extension.hasTransportSequenceNumber) {
I'm not entirely sure it's worth logging when this happens. What if we have BWE
negotiated on video but not on audio (current default)? That means audio packets
will end up here without the header extension, which simply means we shouldn't
include them in the estimate. It's a valid case.

In theory you can also configure your receiver to be able to receive both
abs-send-time and transport sequence numbers, and to send both remb and
transport_cc, but it's up to the sender to decide which extension to use.

[brandtr_google, 2017-02-02 10:45:13]

brandtr@google.com changed reviewers:
+ brandtr@google.com

[brandtr_google, 2017-02-02 10:45:14]

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

https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc#newcode77
webrtc/call/call.cc:77: bool UseSendSideBwe(const VideoReceiveStream::Config&
config) {
On 2017/02/02 10:42:52, stefan-webrtc wrote:
> Maybe use a template instead?

Doesn't work with the FlexfecReceiveStream::Config, see below :( Since
everything in that config has to do with RTP/RTCP, I never added an rtp struct
to it.

[brandtr_google, 2017-02-02 10:45:27]

brandtr@google.com changed reviewers:
- brandtr@google.com

[stefan-webrtc, 2017-02-02 10:47:21]

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

https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc#newcode77
webrtc/call/call.cc:77: bool UseSendSideBwe(const VideoReceiveStream::Config&
config) {
On 2017/02/02 10:45:14, brandtr_google wrote:
> On 2017/02/02 10:42:52, stefan-webrtc wrote:
> > Maybe use a template instead?
> 
> Doesn't work with the FlexfecReceiveStream::Config, see below :( Since
> everything in that config has to do with RTP/RTCP, I never added an rtp struct
> to it.

ah, i see

[nisse-webrtc, 2017-02-02 11:56:00]

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

https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc#newcode1283
webrtc/call/call.cc:1283: if (use_send_side_bwe !=
header.extension.hasTransportSequenceNumber) {
On 2017/02/02 10:42:52, stefan-webrtc wrote:
> I'm not entirely sure it's worth logging when this happens. What if we have
BWE
> negotiated on video but not on audio (current default)? That means audio
packets
> will end up here without the header extension, which simply means we shouldn't
> include them in the estimate. It's a valid case.

In this case I think use_send_side_bwe will be false, and this if-block doesn't
apply. That's because use_send_side_bwe is set if both RTCP feedback and the RTP
extension are negotiated. And that differs from the preceding cl which instead
used transport_cc here.

[stefan-webrtc, 2017-02-02 12:27:37]

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

https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc#newcode1283
webrtc/call/call.cc:1283: if (use_send_side_bwe !=
header.extension.hasTransportSequenceNumber) {
On 2017/02/02 11:56:00, nisse-webrtc wrote:
> On 2017/02/02 10:42:52, stefan-webrtc wrote:
> > I'm not entirely sure it's worth logging when this happens. What if we have
> BWE
> > negotiated on video but not on audio (current default)? That means audio
> packets
> > will end up here without the header extension, which simply means we
shouldn't
> > include them in the estimate. It's a valid case.
> 
> In this case I think use_send_side_bwe will be false, and this if-block
doesn't
> apply. That's because use_send_side_bwe is set if both RTCP feedback and the
RTP
> extension are negotiated. And that differs from the preceding cl which instead
> used transport_cc here.

But as I see it use_send_side_bwe can be true if extension and transport_cc has
been negotiated. But a packet still may not have the extension for some of the
streams we receive? Won't that hit this?

[nisse-webrtc, 2017-02-02 12:30:06]

On 2017/02/02 12:27:37, stefan-webrtc wrote:
> https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc
> File webrtc/call/call.cc (right):
> 
>
https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc#newcode1283
> webrtc/call/call.cc:1283: if (use_send_side_bwe !=
> header.extension.hasTransportSequenceNumber) {
> On 2017/02/02 11:56:00, nisse-webrtc wrote:
> > On 2017/02/02 10:42:52, stefan-webrtc wrote:
> > > I'm not entirely sure it's worth logging when this happens. What if we
have
> > BWE
> > > negotiated on video but not on audio (current default)? That means audio
> > packets
> > > will end up here without the header extension, which simply means we
> shouldn't
> > > include them in the estimate. It's a valid case.
> > 
> > In this case I think use_send_side_bwe will be false, and this if-block
> doesn't
> > apply. That's because use_send_side_bwe is set if both RTCP feedback and the
> RTP
> > extension are negotiated. And that differs from the preceding cl which
instead
> > used transport_cc here.
> 
> But as I see it use_send_side_bwe can be true if extension and transport_cc
has
> been negotiated. But a packet still may not have the extension for some of the
> streams we receive? Won't that hit this?

Isn't the RTP extension negotiated per stream? If it's not negotiated for a
particular stream, use_send_side_bwe should be false here.

[brandtr_google, 2017-02-02 12:39:43]

On 2017/02/02 12:30:06, nisse-webrtc wrote:
> On 2017/02/02 12:27:37, stefan-webrtc wrote:
> > https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc
> > File webrtc/call/call.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2669463006/diff/1/webrtc/call/call.cc#newcode1283
> > webrtc/call/call.cc:1283: if (use_send_side_bwe !=
> > header.extension.hasTransportSequenceNumber) {
> > On 2017/02/02 11:56:00, nisse-webrtc wrote:
> > > On 2017/02/02 10:42:52, stefan-webrtc wrote:
> > > > I'm not entirely sure it's worth logging when this happens. What if we
> have
> > > BWE
> > > > negotiated on video but not on audio (current default)? That means audio
> > > packets
> > > > will end up here without the header extension, which simply means we
> > shouldn't
> > > > include them in the estimate. It's a valid case.
> > > 
> > > In this case I think use_send_side_bwe will be false, and this if-block
> > doesn't
> > > apply. That's because use_send_side_bwe is set if both RTCP feedback and
the
> > RTP
> > > extension are negotiated. And that differs from the preceding cl which
> instead
> > > used transport_cc here.
> > 
> > But as I see it use_send_side_bwe can be true if extension and transport_cc
> has
> > been negotiated. But a packet still may not have the extension for some of
the
> > streams we receive? Won't that hit this?
> 
> Isn't the RTP extension negotiated per stream? If it's not negotiated for a
> particular stream, use_send_side_bwe should be false here.

The extension is negotiated per "m= line" in the SDP, i.e., all payload types
related to video: media, RTX, RED, FlexFEC, etc.

The RTCP feedback message extension is negotiated per payload type.

The only thing that is negotiated per SSRC is relationships between SSRCs: media
<-> media (SIM, i.e., simulcast), media <-> RTX (FID), and media <-> FlexFEC
(FEC-FR).

So it's a bit hard to talk about "streams" when it comes to SDP negotiation :(
Hence the duplication of some of this information in the map of
ReceiveRtpConfigs.

[nisse-webrtc, 2017-02-06 11:56:39]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 11:56:48]

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

[nisse-webrtc, 2017-02-06 12:42:12]

https://codereview.webrtc.org/2669463006/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2669463006/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:791: void
Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) {
I guess this method ought to call 

congestion_controller_->GetRemoteBitrateEstimator(...)->RemoveStream(ssrc)?

like the other Destroy*Stream methods?

[brandtr, 2017-02-06 12:55:15]

https://codereview.webrtc.org/2669463006/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2669463006/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:791: void
Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) {
On 2017/02/06 12:42:12, nisse-webrtc wrote:
> I guess this method ought to call 
> 
> congestion_controller_->GetRemoteBitrateEstimator(...)->RemoveStream(ssrc)?
> 
> like the other Destroy*Stream methods?

Yep, that makes sense!

[nisse-webrtc, 2017-02-06 13:02:15]

https://codereview.webrtc.org/2669463006/diff/20001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2669463006/diff/20001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:791: void
Call::DestroyFlexfecReceiveStream(FlexfecReceiveStream* receive_stream) {
On 2017/02/06 12:55:14, brandtr wrote:
> On 2017/02/06 12:42:12, nisse-webrtc wrote:
> > I guess this method ought to call 
> > 
> > congestion_controller_->GetRemoteBitrateEstimator(...)->RemoveStream(ssrc)?
> > 
> > like the other Destroy*Stream methods?
> 
> Yep, that makes sense!

Done.

[nisse-webrtc, 2017-02-06 13:13:19]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 13:13:31]

Dry run: 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, 2017-02-06 14:13:17]

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

[commit-bot: I haz the power, 2017-02-06 14:13:18]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-02-07 07:38:37]

lgtm

[nisse-webrtc, 2017-02-07 07:41:10]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-02-07 07:41:11]

The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2669463006/#ps40001 (title:
"Call RBE::RemoveStream also from DestroyFlexfecReceiveStream.")

[commit-bot: I haz the power, 2017-02-07 07:41:23]

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, 2017-02-07 07:45:11]

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

[commit-bot: I haz the power, 2017-02-07 07:45:12]

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

[nisse-webrtc, 2017-02-07 08:02:17]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-02-07 08:02:17]

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

[commit-bot: I haz the power, 2017-02-07 08:02:27]

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, 2017-02-07 09:18:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486454537456030,
"parent_rev": "6b3fcfd82318aea0cd4ce1705f510ece0057491a", "commit_rev":
"4709e8971b506c69134420cde1fce8d61b6de58e"}

[commit-bot: I haz the power, 2017-02-07 09:18:47]

Description was changed from

==========
Move RemoteBitrateEstimator::RemoveStream calls from receive streams to Call.

We can then drop the CongestionController and RemoteBitrateEstimator
completely from the receive streams.

BUG=webrtc:6847
==========

to

==========
Move RemoteBitrateEstimator::RemoveStream calls from receive streams to Call.

We can then drop the CongestionController and RemoteBitrateEstimator
completely from the receive streams.

BUG=webrtc:6847

Review-Url: https://codereview.webrtc.org/2669463006
Cr-Commit-Position: refs/heads/master@{#16459}
Committed:
https://chromium.googlesource.com/external/webrtc/+/4709e8971b506c69134420cde...
==========

[commit-bot: I haz the power, 2017-02-07 09:18:48]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/4709e8971b506c69134420cde...