Wire up send-side bandwidth estimation.

Wire up send-side bandwidth estimation.

BUG=webrtc:4173
Committed: https://crrev.com/ef165eefc79cf28bb67779afe303cc2365885547
Cr-Commit-Position: refs/heads/master@{#10012}

[sprang_webrtc, 2015-09-14 15:44:47]

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

[stefan-webrtc, 2015-09-17 10:33:19]

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1266:
rtcp_packet_information->rtcpPacketTypeFlags |= kRtcpTransportFeedback;
I find it a bit strange that we use this flag when we could check the pointer
below to see if it's null. What do you think? I guess how you are doing it is
how it's done elsewhere, right? Is it possible to clean up in a follow-up?

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:1221: // Note: No state
transition, SR REQ is empty!
May want to add a comment similar to this below as well.

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.h (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_utility.h:357: rtcp::RtcpPacket*
ReleaseRtcpPacket();
How does this RtcpPacket differ from RTCPPacket? Is this part of a new
refactoring to use the new RtcpPacket class?

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/loopback.cc
File webrtc/video/loopback.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/loopback.cc#newc...
webrtc/video/loopback.cc:101: send_config.rtp.extensions.push_back(
indentation

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc#...
webrtc/video/rampup_tests.cc:466: } else {
I'm starting to think that instead of adding a lot of extra special code for the
send-side BWE, maybe we can just use a true receive Call in this test, and have
both versions of the test monitor the send-side estimate? I don't think it would
be that much work actually. What do you think?

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc#...
webrtc/video/rampup_tests.cc:524: send_bitrate_adapter_.reset();
This is needed to stop the thread? Maybe comment.

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video_engine/vie_chann...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:270: feedback_observer =
transport_feedback_adapter_.get();
Why do we have something observing feedback on the receive-side? From what I can
see we only call it from RtpSender?

[sprang_webrtc, 2015-09-17 15:08:28]

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1266:
rtcp_packet_information->rtcpPacketTypeFlags |= kRtcpTransportFeedback;
On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> I find it a bit strange that we use this flag when we could check the pointer
> below to see if it's null. What do you think? I guess how you are doing it is
> how it's done elsewhere, right? Is it possible to clean up in a follow-up?

Yeah. I'm just following suit. Definitely the whole rtcp receiver et al needs an
overhaul.

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:1221: // Note: No state
transition, SR REQ is empty!
On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> May want to add a comment similar to this below as well.

Done.

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.h (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_utility.h:357: rtcp::RtcpPacket*
ReleaseRtcpPacket();
On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> How does this RtcpPacket differ from RTCPPacket? Is this part of a new
> refactoring to use the new RtcpPacket class?

This is the base class for the shiny new RtcpPacket classes asapersson@
introduced. Currently they're only used on the send side, but I think we should
work towards using them on the receive side as well..

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/loopback.cc
File webrtc/video/loopback.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/loopback.cc#newc...
webrtc/video/loopback.cc:101: send_config.rtp.extensions.push_back(
On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> indentation

Done.

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc#...
webrtc/video/rampup_tests.cc:466: } else {
On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> I'm starting to think that instead of adding a lot of extra special code for
the
> send-side BWE, maybe we can just use a true receive Call in this test, and
have
> both versions of the test monitor the send-side estimate? I don't think it
would
> be that much work actually. What do you think?

I initially tried that but there were some obstacles. Can't quite remember what
they were now.. Injecting a mock RemoteBitrateEstimator to fetch remb values is
one. Could we do this in a follow-up cl instead?

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc#...
webrtc/video/rampup_tests.cc:524: send_bitrate_adapter_.reset();
On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> This is needed to stop the thread? Maybe comment.

Done.

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video_engine/vie_chann...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video_engine/vie_chann...
webrtc/video_engine/vie_channel_group.cc:270: feedback_observer =
transport_feedback_adapter_.get();
On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> Why do we have something observing feedback on the receive-side? From what I
can
> see we only call it from RtpSender?

Oops, copy/paste error.

[stefan-webrtc, 2015-09-18 10:57:23]

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1266:
rtcp_packet_information->rtcpPacketTypeFlags |= kRtcpTransportFeedback;
On 2015/09/17 15:08:28, språng wrote:
> On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> > I find it a bit strange that we use this flag when we could check the
pointer
> > below to see if it's null. What do you think? I guess how you are doing it
is
> > how it's done elsewhere, right? Is it possible to clean up in a follow-up?
> 
> Yeah. I'm just following suit. Definitely the whole rtcp receiver et al needs
an
> overhaul.

I think we have to outline how this is going to look like in the end, otherwise
we are running risks of not ending up somewhere suboptimal.

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc#...
webrtc/video/rampup_tests.cc:466: } else {
On 2015/09/17 15:08:28, språng wrote:
> On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> > I'm starting to think that instead of adding a lot of extra special code for
> the
> > send-side BWE, maybe we can just use a true receive Call in this test, and
> have
> > both versions of the test monitor the send-side estimate? I don't think it
> would
> > be that much work actually. What do you think?
> 
> I initially tried that but there were some obstacles. Can't quite remember
what
> they were now.. Injecting a mock RemoteBitrateEstimator to fetch remb values
is
> one. Could we do this in a follow-up cl instead?

We can do it in a follow-up.

I don't see why we need to know the remb values though? Can't we make the test
rely on send-side bwe values as reported from GetStats?

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:1258: return false;
I think it would be easier to read if we to put an RTC_DCHECK(false) here and
move the EndCurrentBlock() and return false into each case, since most cases
have a return in them. Or the opposite, only have a single return here and none
inside the switch.

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video/rampup_tests...
webrtc/video/rampup_tests.cc:123: }
Basically we can move all of this into a thread which monitors GetStats(), or we
call GetStats() from SendRtp below.

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video/video_loopba...
File webrtc/video/video_loopback.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video/video_loopba...
webrtc/video/video_loopback.cc:143: DEFINE_bool(send_side_bwe, false, "Use
send-side bandwidth estimation");
Maybe we should default this to true so we have a better chance of discovering
issues early?

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel_group.cc:168: new RemoteEstimatorProxy(clock,
packet_router_.get()));
Can't these be instantiated in the initializer list?

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel_group.cc:261: }
Maybe it would be good to DCHECK that we don't run some receive channels with
send_side_bwe = false and others with true?

[sprang_webrtc, 2015-09-21 10:44:11]

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1266:
rtcp_packet_information->rtcpPacketTypeFlags |= kRtcpTransportFeedback;
On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> On 2015/09/17 15:08:28, språng wrote:
> > On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> > > I find it a bit strange that we use this flag when we could check the
> pointer
> > > below to see if it's null. What do you think? I guess how you are doing it
> is
> > > how it's done elsewhere, right? Is it possible to clean up in a follow-up?
> > 
> > Yeah. I'm just following suit. Definitely the whole rtcp receiver et al
needs
> an
> > overhaul.
> 
> I think we have to outline how this is going to look like in the end,
otherwise
> we are running risks of not ending up somewhere suboptimal.

Why would we want to end up somewhere suboptimal?
This sounds like a large discussion, let's do it offline?

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc#...
webrtc/video/rampup_tests.cc:466: } else {
On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> On 2015/09/17 15:08:28, språng wrote:
> > On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> > > I'm starting to think that instead of adding a lot of extra special code
for
> > the
> > > send-side BWE, maybe we can just use a true receive Call in this test, and
> > have
> > > both versions of the test monitor the send-side estimate? I don't think it
> > would
> > > be that much work actually. What do you think?
> > 
> > I initially tried that but there were some obstacles. Can't quite remember
> what
> > they were now.. Injecting a mock RemoteBitrateEstimator to fetch remb values
> is
> > one. Could we do this in a follow-up cl instead?
> 
> We can do it in a follow-up.
> 
> I don't see why we need to know the remb values though? Can't we make the test
> rely on send-side bwe values as reported from GetStats?

I was just under the presumption that we wanted to test the remb values, rather
than changing the semantics and instead relying on the combined bandwidth
estimate? If that isn't the case then sure, let's do a follow up where we have a
real receive call and use the get stats. All will then have to use polling, just
like send-side...

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:1258: return false;
On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> I think it would be easier to read if we to put an RTC_DCHECK(false) here and
> move the EndCurrentBlock() and return false into each case, since most cases
> have a return in them. Or the opposite, only have a single return here and
none
> inside the switch.

All cases in this switch returns true. There is only one where the value isn't
supported anymore, I think we should just remove that case-block instead. An
RTC_DCHECK is probably not desired here, if it is a RTCP message we don't
recognize, we should perhaps warn but then skip that block and move to the next,
not crash. Right?

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video/video_loopba...
File webrtc/video/video_loopback.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video/video_loopba...
webrtc/video/video_loopback.cc:143: DEFINE_bool(send_side_bwe, false, "Use
send-side bandwidth estimation");
On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> Maybe we should default this to true so we have a better chance of discovering
> issues early?

Done.

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel_group.cc:168: new RemoteEstimatorProxy(clock,
packet_router_.get()));
On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> Can't these be instantiated in the initializer list?

Done.

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel_group.cc:261: }
On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> Maybe it would be good to DCHECK that we don't run some receive channels with
> send_side_bwe = false and others with true?

Right now a mix is supported. So the not very explicitly stated requirement is
that all (video) channels must be configured the same way wrt send-side bwe?

[stefan-webrtc, 2015-09-21 11:12:07]

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc#...
webrtc/video/rampup_tests.cc:466: } else {
On 2015/09/21 10:44:10, språng wrote:
> On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> > On 2015/09/17 15:08:28, språng wrote:
> > > On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> > > > I'm starting to think that instead of adding a lot of extra special code
> for
> > > the
> > > > send-side BWE, maybe we can just use a true receive Call in this test,
and
> > > have
> > > > both versions of the test monitor the send-side estimate? I don't think
it
> > > would
> > > > be that much work actually. What do you think?
> > > 
> > > I initially tried that but there were some obstacles. Can't quite remember
> > what
> > > they were now.. Injecting a mock RemoteBitrateEstimator to fetch remb
values
> > is
> > > one. Could we do this in a follow-up cl instead?
> > 
> > We can do it in a follow-up.
> > 
> > I don't see why we need to know the remb values though? Can't we make the
test
> > rely on send-side bwe values as reported from GetStats?
> 
> I was just under the presumption that we wanted to test the remb values,
rather
> than changing the semantics and instead relying on the combined bandwidth
> estimate? If that isn't the case then sure, let's do a follow up where we have
a
> real receive call and use the get stats. All will then have to use polling,
just
> like send-side...

Right, that's my thinking. I never intended to check the remb values, it was
probably a bad choice writing the tests like this. I think the problem was
partly that the stats APIs weren't ready at that time.

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:1258: return false;
On 2015/09/21 10:44:10, språng wrote:
> On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> > I think it would be easier to read if we to put an RTC_DCHECK(false) here
and
> > move the EndCurrentBlock() and return false into each case, since most cases
> > have a return in them. Or the opposite, only have a single return here and
> none
> > inside the switch.
> 
> All cases in this switch returns true. There is only one where the value isn't
> supported anymore, I think we should just remove that case-block instead. An
> RTC_DCHECK is probably not desired here, if it is a RTCP message we don't
> recognize, we should perhaps warn but then skip that block and move to the
next,
> not crash. Right?

Sounds good. log it once every 10 seconds maybe?

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel_group.cc:261: }
On 2015/09/21 10:44:10, språng wrote:
> On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> > Maybe it would be good to DCHECK that we don't run some receive channels
with
> > send_side_bwe = false and others with true?
> 
> Right now a mix is supported. So the not very explicitly stated requirement is
> that all (video) channels must be configured the same way wrt send-side bwe?

Well, at least we could make that a requirement if we'd like to. It sounds like
asking for problems if we try to do both at the same time. On the other hand,
maybe there's a risk of getting a race when switching and therefore not worth
DCHECKing?

[stefan-webrtc, 2015-09-21 11:13:16]

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1266:
rtcp_packet_information->rtcpPacketTypeFlags |= kRtcpTransportFeedback;
On 2015/09/21 10:44:10, språng wrote:
> On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> > On 2015/09/17 15:08:28, språng wrote:
> > > On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> > > > I find it a bit strange that we use this flag when we could check the
> > pointer
> > > > below to see if it's null. What do you think? I guess how you are doing
it
> > is
> > > > how it's done elsewhere, right? Is it possible to clean up in a
follow-up?
> > > 
> > > Yeah. I'm just following suit. Definitely the whole rtcp receiver et al
> needs
> > an
> > > overhaul.
> > 
> > I think we have to outline how this is going to look like in the end,
> otherwise
> > we are running risks of not ending up somewhere suboptimal.
> 
> Why would we want to end up somewhere suboptimal?
> This sounds like a large discussion, let's do it offline?

I mostly meant that we probably should have an idea of how this is going to look
in the end. Not something we need for this CL though. :)

[sprang_webrtc, 2015-09-21 13:42:53]

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc
File webrtc/video/rampup_tests.cc (right):

https://codereview.webrtc.org/1338203003/diff/1/webrtc/video/rampup_tests.cc#...
webrtc/video/rampup_tests.cc:466: } else {
On 2015/09/21 11:12:07, stefan-webrtc (holmer) wrote:
> On 2015/09/21 10:44:10, språng wrote:
> > On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> > > On 2015/09/17 15:08:28, språng wrote:
> > > > On 2015/09/17 10:33:19, stefan-webrtc (holmer) wrote:
> > > > > I'm starting to think that instead of adding a lot of extra special
code
> > for
> > > > the
> > > > > send-side BWE, maybe we can just use a true receive Call in this test,
> and
> > > > have
> > > > > both versions of the test monitor the send-side estimate? I don't
think
> it
> > > > would
> > > > > be that much work actually. What do you think?
> > > > 
> > > > I initially tried that but there were some obstacles. Can't quite
remember
> > > what
> > > > they were now.. Injecting a mock RemoteBitrateEstimator to fetch remb
> values
> > > is
> > > > one. Could we do this in a follow-up cl instead?
> > > 
> > > We can do it in a follow-up.
> > > 
> > > I don't see why we need to know the remb values though? Can't we make the
> test
> > > rely on send-side bwe values as reported from GetStats?
> > 
> > I was just under the presumption that we wanted to test the remb values,
> rather
> > than changing the semantics and instead relying on the combined bandwidth
> > estimate? If that isn't the case then sure, let's do a follow up where we
have
> a
> > real receive call and use the get stats. All will then have to use polling,
> just
> > like send-side...
> 
> Right, that's my thinking. I never intended to check the remb values, it was
> probably a bad choice writing the tests like this. I think the problem was
> partly that the stats APIs weren't ready at that time.

Acknowledged.

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_utility.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_utility.cc:1258: return false;
On 2015/09/21 11:12:07, stefan-webrtc (holmer) wrote:
> On 2015/09/21 10:44:10, språng wrote:
> > On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> > > I think it would be easier to read if we to put an RTC_DCHECK(false) here
> and
> > > move the EndCurrentBlock() and return false into each case, since most
cases
> > > have a return in them. Or the opposite, only have a single return here and
> > none
> > > inside the switch.
> > 
> > All cases in this switch returns true. There is only one where the value
isn't
> > supported anymore, I think we should just remove that case-block instead. An
> > RTC_DCHECK is probably not desired here, if it is a RTCP message we don't
> > recognize, we should perhaps warn but then skip that block and move to the
> next,
> > not crash. Right?
> 
> Sounds good. log it once every 10 seconds maybe?

Done.

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel_group.cc:261: }
On 2015/09/21 11:12:07, stefan-webrtc (holmer) wrote:
> On 2015/09/21 10:44:10, språng wrote:
> > On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> > > Maybe it would be good to DCHECK that we don't run some receive channels
> with
> > > send_side_bwe = false and others with true?
> > 
> > Right now a mix is supported. So the not very explicitly stated requirement
is
> > that all (video) channels must be configured the same way wrt send-side bwe?
> 
> Well, at least we could make that a requirement if we'd like to. It sounds
like
> asking for problems if we try to do both at the same time. On the other hand,
> maybe there's a risk of getting a race when switching and therefore not worth
> DCHECKing?

Is suspect there might. It's also gonna be kinda messy to verify. Do we allow
mixing send-side bwe on the receive streams but not on the send streams?

[stefan-webrtc, 2015-09-21 14:06:30]

lgtm

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
File webrtc/video_engine/vie_channel_group.cc (right):

https://codereview.webrtc.org/1338203003/diff/20001/webrtc/video_engine/vie_c...
webrtc/video_engine/vie_channel_group.cc:261: }
On 2015/09/21 13:42:53, språng wrote:
> On 2015/09/21 11:12:07, stefan-webrtc (holmer) wrote:
> > On 2015/09/21 10:44:10, språng wrote:
> > > On 2015/09/18 10:57:23, stefan-webrtc (holmer) wrote:
> > > > Maybe it would be good to DCHECK that we don't run some receive channels
> > with
> > > > send_side_bwe = false and others with true?
> > > 
> > > Right now a mix is supported. So the not very explicitly stated
requirement
> is
> > > that all (video) channels must be configured the same way wrt send-side
bwe?
> > 
> > Well, at least we could make that a requirement if we'd like to. It sounds
> like
> > asking for problems if we try to do both at the same time. On the other
hand,
> > maybe there's a risk of getting a race when switching and therefore not
worth
> > DCHECKing?
> 
> Is suspect there might. It's also gonna be kinda messy to verify. Do we allow
> mixing send-side bwe on the receive streams but not on the send streams?

Yes, that may be so. Let's leave it as is for the moment at least. :)

[sprang_webrtc, 2015-09-21 14:25:01]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2015-09-21 14:25:11]

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

[commit-bot: I haz the power, 2015-09-21 14:26:14]

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

[commit-bot: I haz the power, 2015-09-21 14:26:15]

Try jobs failed on following builders:
  android_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/5744)
  linux on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux/builds/9707)

[sprang_webrtc, 2015-09-21 15:19:52]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2015-09-21 15:19:53]

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

[commit-bot: I haz the power, 2015-09-21 15:20:04]

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

[commit-bot: I haz the power, 2015-09-21 15:25:07]

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

[commit-bot: I haz the power, 2015-09-21 15:25:08]

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

[sprang_webrtc, 2015-09-22 08:04:57]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2015-09-22 08:04:57]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1338203003/#ps140001 (title:
"More GN fixes...")

[commit-bot: I haz the power, 2015-09-22 08:05:01]

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

[commit-bot: I haz the power, 2015-09-22 10:05:24]

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

[commit-bot: I haz the power, 2015-09-22 10:05:25]

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

[sprang_webrtc, 2015-09-22 12:09:40]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2015-09-22 12:09:44]

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

[commit-bot: I haz the power, 2015-09-22 12:10:56]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-22 12:11:08]

Patchset 8 (id:??) landed as
https://crrev.com/ef165eefc79cf28bb67779afe303cc2365885547
Cr-Commit-Position: refs/heads/master@{#10012}

[sprang_webrtc, 2015-09-23 11:09:08]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.webrtc.org/1362923002/ by sprang@webrtc.org.

The reason for reverting is: Breaking some Android bots.
https://chromegw.corp.google.com/i/client.webrtc/builders/Android32%20Tests%2....