Delete VieRemb class, move functionality to PacketRouter.

Delete VieRemb class, move functionality to PacketRouter.

Also rename SendFeedback --> SendTransportFeedback.

BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2789843002
Cr-Commit-Position: refs/heads/master@{#17755}
Committed: https://chromium.googlesource.com/external/webrtc/+/05843312195912a0fab761702991f5041aba9be3

[nisse-webrtc, 2017-03-31 13:47:56]

Description was changed from

==========
Rename SendFeedback --> SendTransportFeedback.

BUG=webrtc:7135
==========

to

==========
Delete VieRemb class, move functionality to PacketRouter.

Also rename SendFeedback --> SendTransportFeedback.

BUG=webrtc:7135
==========

[nisse-webrtc, 2017-03-31 13:49:35]

nisse@webrtc.org changed reviewers:
+ danilchap@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org

[nisse-webrtc, 2017-03-31 13:49:35]

PTAL, this is a first attempt, not yet working.

[stefan-webrtc, 2017-03-31 14:26:49]

Looks good, when it works. :)

[nisse-webrtc, 2017-04-03 11:32:36]

Seems to pass tests now.

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/congestion_controller_unittest.cc
(right):

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/congestion_controller_unittest.cc:347:
TEST(ReceiveSideCongestionControllerTest, OnReceivedPacketWithAbsSendTime) {
This test works fine without a test fixture. At some point, 
more of these tests need reorganization, in particular when
the time comes to remove the old CongestionController class
and use the receive side or send side classes directly.

[stefan-webrtc, 2017-04-03 12:31:51]

lgtm % nits

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.h:93: uint32_t bitrate_
GUARDED_BY(remb_crit_);
Add units to these members

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:884: // delete the object?
Feel free to remove in that case. The only reason I can think of where this
could make sense is if we have some other thread which may try to send REMB
while the module is being deleted.

[nisse-webrtc, 2017-04-03 14:45:03]

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.cc:159: // TODO(nisse): Check REMB status of
the modules? Or add a loop,
Can you have another look at this logic? Can we get away with using the first
module?

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/modules/pacing/pac...
File webrtc/modules/pacing/packet_router.h (right):

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/modules/pacing/pac...
webrtc/modules/pacing/packet_router.h:93: uint32_t bitrate_
GUARDED_BY(remb_crit_);
On 2017/04/03 12:31:51, stefan-webrtc wrote:
> Add units to these members

Done.

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2789843002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:884: // delete the object?
On 2017/04/03 12:31:51, stefan-webrtc wrote:
> Feel free to remove in that case.

I'll delete then.

> The only reason I can think of where this
> could make sense is if we have some other thread which may try to send REMB
> while the module is being deleted.

As far as I understand, in this case, the call to RemoveSendRtpModule
below will block until the call from the other thread is finished. So we won't
delete an object which is in use.

[danilchap, 2017-04-03 16:31:34]

https://codereview.webrtc.org/2789843002/diff/100001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2789843002/diff/100001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:159: {
Do you mind splitting this block into own public function?
bool PacketRouter::SendRemb(bitrate_bps, ssrcs) {
  rtc::CritScope cs(&modules_crit_);
  RtpRtcp* remb_module = nullptr;
  if (!rtp_send_modules_.empty())
    remb_module = rtp_send_modules_.front();
  else if (!rtp_receive_modules_.empty())
    remb_module = rtp_receive_modules_.front();
  else
    return false;
  remb_module->SetREMBData(bitrate_bps, ssrcs);
  // and may be to actually send remb...
  return remb_module->SendRtcp(kRtcpRemb) == 0;
}

[nisse-webrtc, 2017-04-04 09:46:44]

https://codereview.webrtc.org/2789843002/diff/100001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2789843002/diff/100001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:159: {
On 2017/04/03 16:31:34, danilchap wrote:
> Do you mind splitting this block into own public function?

Not at all. (If possible, I'd actually prefer to not implement the
RtpReceiveBitrateUpdate call and move its logic elsewhere. But if you don't see
any easy way to do that, it has to wait for some other cl).

> bool PacketRouter::SendRemb(bitrate_bps, ssrcs) {
>   rtc::CritScope cs(&modules_crit_);
>   RtpRtcp* remb_module = nullptr;
>   if (!rtp_send_modules_.empty())
>     remb_module = rtp_send_modules_.front();
>   else if (!rtp_receive_modules_.empty())
>     remb_module = rtp_receive_modules_.front();
>   else
>     return false;
>   remb_module->SetREMBData(bitrate_bps, ssrcs);
>   // and may be to actually send remb...
>   return remb_module->SendRtcp(kRtcpRemb) == 0;

Is the last call needed? RTCPSender::SetREMBData ends with

  // Send a REMB immediately if we have a new REMB. The frequency of REMBs is
  // throttled by the caller.
  next_time_to_send_rtcp_ = clock_->TimeInMilliseconds();

[danilchap, 2017-04-04 10:18:27]

lgtm

https://codereview.webrtc.org/2789843002/diff/100001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2789843002/diff/100001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:159: {
On 2017/04/04 09:46:44, nisse-webrtc wrote:
> Is the last call needed? RTCPSender::SetREMBData ends with
> 
>   // Send a REMB immediately if we have a new REMB. The frequency of REMBs is
>   // throttled by the caller.
>   next_time_to_send_rtcp_ = clock_->TimeInMilliseconds();

not strictly needed, but this is one of the way to resolve your TODO: Check REMB
status.

without explicit SendRtcp(Remb) it will be eventually send [on process thread]
only if remb_module was configured with remb (SetRembStatus(true)).
Calling SendRtcp(Remb) explicetly will ensure remb is actually sent.

On the other side there is risk here: SendRtcp is a large function that involve
taking bunch of locks, so need to be careful not add a deadlock.
Also, calling SendRtcp makes it harder to reason what threads rtcp messages are
send from.

not calling SendRtcp(Remb) probably a better decision in this CL.

[nisse-webrtc, 2017-04-04 12:31:46]

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:130: if (config_.rtp.remb) {
Can I delete the config_.rtp.remb flag? Not sure how configuration should work,
with this cl, we'll send REMB info if and only if OnReceiveBitrateChanged is
called, which depends on the estimators fed by the receive side congestion
controller.

[danilchap, 2017-04-07 07:37:41]

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:130: if (config_.rtp.remb) {
On 2017/04/04 12:31:46, nisse-webrtc wrote:
> Can I delete the config_.rtp.remb flag? Not sure how configuration should
work,
> with this cl, we'll send REMB info if and only if OnReceiveBitrateChanged is
> called, which depends on the estimators fed by the receive side congestion
> controller.

Since you now control remb in PacketRouter, this flag/block likely can be
removed.

In perfect world I guess this flag should be pass to packet router, that
shouldn't then try to send remb when it is not configured to avoid wasting few
bytes per rtcp packet.

[nisse-webrtc, 2017-04-07 08:21:44]

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:130: if (config_.rtp.remb) {
On 2017/04/07 07:37:40, danilchap wrote:
> On 2017/04/04 12:31:46, nisse-webrtc wrote:
> > Can I delete the config_.rtp.remb flag? Not sure how configuration should
> work,
> > with this cl, we'll send REMB info if and only if OnReceiveBitrateChanged is
> > called, which depends on the estimators fed by the receive side congestion
> > controller.
> 
> Since you now control remb in PacketRouter, this flag/block likely can be
> removed.
> 
> In perfect world I guess this flag should be pass to packet router, that
> shouldn't then try to send remb when it is not configured to avoid wasting few
> bytes per rtcp packet.

I'll try deleting it then, we can add configurability in PacketRouter if we ever
need it.

[nisse-webrtc, 2017-04-07 08:32:18]

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:130: if (config_.rtp.remb) {
On 2017/04/07 08:21:44, nisse-webrtc wrote:
> On 2017/04/07 07:37:40, danilchap wrote:
> > On 2017/04/04 12:31:46, nisse-webrtc wrote:
> > > Can I delete the config_.rtp.remb flag? Not sure how configuration should
> > work,
> > > with this cl, we'll send REMB info if and only if OnReceiveBitrateChanged
is
> > > called, which depends on the estimators fed by the receive side congestion
> > > controller.
> > 
> > Since you now control remb in PacketRouter, this flag/block likely can be
> > removed.
> > 
> > In perfect world I guess this flag should be pass to packet router, that
> > shouldn't then try to send remb when it is not configured to avoid wasting
few
> > bytes per rtcp packet.
> 
> I'll try deleting it then, we can add configurability in PacketRouter if we
ever
> need it.

What about this HasRemb method?

https://cs.chromium.org/chromium/src/third_party/webrtc/media/base/codec.cc?t...

If I delete the only place reading VideoReceiveStream::Config::Rtp::remb, it
would be good to also delete all code related to writing that setting.

[danilchap, 2017-04-07 08:45:28]

On 2017/04/07 08:32:18, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
> File webrtc/video/rtp_stream_receiver.cc (right):
> 
>
https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
> webrtc/video/rtp_stream_receiver.cc:130: if (config_.rtp.remb) {
> On 2017/04/07 08:21:44, nisse-webrtc wrote:
> > On 2017/04/07 07:37:40, danilchap wrote:
> > > On 2017/04/04 12:31:46, nisse-webrtc wrote:
> > > > Can I delete the config_.rtp.remb flag? Not sure how configuration
should
> > > work,
> > > > with this cl, we'll send REMB info if and only if
OnReceiveBitrateChanged
> is
> > > > called, which depends on the estimators fed by the receive side
congestion
> > > > controller.
> > > 
> > > Since you now control remb in PacketRouter, this flag/block likely can be
> > > removed.
> > > 
> > > In perfect world I guess this flag should be pass to packet router, that
> > > shouldn't then try to send remb when it is not configured to avoid wasting
> few
> > > bytes per rtcp packet.
> > 
> > I'll try deleting it then, we can add configurability in PacketRouter if we
> ever
> > need it.
> 
> What about this HasRemb method?
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/media/base/codec.cc?t...
> 

This one doesn't look related to this CL, but I'm not familiar enough with code
in media/ to be sure.

> If I delete the only place reading VideoReceiveStream::Config::Rtp::remb, it
> would be good to also delete all code related to writing that setting.

it still should be added back [to packet router or similar] structure
eventually, so probably better not to delete too much.

[nisse-webrtc, 2017-04-07 09:14:31]

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
File webrtc/video/rtp_stream_receiver.cc (right):

https://codereview.webrtc.org/2789843002/diff/120001/webrtc/video/rtp_stream_...
webrtc/video/rtp_stream_receiver.cc:130: if (config_.rtp.remb) {
On 2017/04/07 08:32:18, nisse-webrtc wrote:
> On 2017/04/07 08:21:44, nisse-webrtc wrote:
> > On 2017/04/07 07:37:40, danilchap wrote:
> > > On 2017/04/04 12:31:46, nisse-webrtc wrote:
> > > > Can I delete the config_.rtp.remb flag? Not sure how configuration
should
> > > work,
> > > > with this cl, we'll send REMB info if and only if
OnReceiveBitrateChanged
> is
> > > > called, which depends on the estimators fed by the receive side
congestion
> > > > controller.
> > > 
> > > Since you now control remb in PacketRouter, this flag/block likely can be
> > > removed.
> > > 
> > > In perfect world I guess this flag should be pass to packet router, that
> > > shouldn't then try to send remb when it is not configured to avoid wasting
> few
> > > bytes per rtcp packet.
> > 
> > I'll try deleting it then, we can add configurability in PacketRouter if we
> ever
> > need it.
> 
> What about this HasRemb method?
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/media/base/codec.cc?t...
> 
> If I delete the only place reading VideoReceiveStream::Config::Rtp::remb, it
> would be good to also delete all code related to writing that setting.

I'll leave the code setting config.rtp.remb for now, just adding a TODO
explaining that it is currently ignored.

[nisse-webrtc, 2017-04-11 08:24:17]

New version, converting test which I forgot in the earlier patchsets. I also
fixed an initialization bug.

Please have a look at the tests, I have a couple of questions marked with TODO
comments.

[stefan-webrtc, 2017-04-11 09:28:36]

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:94: }
We need unittests which verify this behavior. Or maybe we should have DCHECKs in
the add and remove methods that verifies only one module is active?

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:204: // otherwise, they will send REMB
with stale info.
Does this mean we periodically send REMB if REMB is enabled? Do we have to
disable REMB on all modules if we run send-side BWE and aren't interested in
REMB?

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:263: // logic probably won't do
the right thing in this case.
Can this even be done in the code right now? I thought send-side and
receive-side were separated enough that this wouldn't be the case?

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:457: TEST(PacketRouterRembTest,
OnlyOneRembForDoubleProcess) {
Process has no meaning below. Should it say OnReceiveBitrateChanged?

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/video_receive_str...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/video_receive_str...
webrtc/video_receive_stream.h:142: // disable REMB feedback.
This would be important if there are no other ways for a user to disable REMB.
Can it be done by simply not negotiating abs-send-time or toffset header
extensions?

[nisse-webrtc, 2017-04-11 10:57:42]

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:94: }
On 2017/04/11 09:28:36, stefan-webrtc wrote:
> We need unittests which verify this behavior. Or maybe we should have DCHECKs
in
> the add and remove methods that verifies only one module is active?

I'd like to avoid DCHECKs which iterates over the lists. I could add DCHECKs
which examine the first or first two elements on each list. Is that enough?

The PacketRouter unit tests don't cover all the cases of added and removed
modules.

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:204: // otherwise, they will send REMB
with stale info.
On 2017/04/11 09:28:36, stefan-webrtc wrote:
> Does this mean we periodically send REMB if REMB is enabled? Do we have to
> disable REMB on all modules if we run send-side BWE and aren't interested in
> REMB?

I'm not sure, but I think that each module will generate REMB info if the REMB
status is true, and SetREMBData has ever been called.

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:263: // logic probably won't do
the right thing in this case.
On 2017/04/11 09:28:36, stefan-webrtc wrote:
> Can this even be done in the code right now? I thought send-side and
> receive-side were separated enough that this wouldn't be the case?

I don't think there's any code that calls AddSendRtpModule and
AddReceiveRtpModule with the same module. Except this test.

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/video_receive_str...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/video_receive_str...
webrtc/video_receive_stream.h:142: // disable REMB feedback.
On 2017/04/11 09:28:36, stefan-webrtc wrote:
> This would be important if there are no other ways for a user to disable REMB.
> Can it be done by simply not negotiating abs-send-time or toffset header
> extensions?

As far as I understand, if there are no abs-send-time or toffset header
extensions, no REMB packets will be sent.

[danilchap, 2017-04-11 12:34:14]

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:204: // otherwise, they will send REMB
with stale info.
On 2017/04/11 10:57:41, nisse-webrtc wrote:
> On 2017/04/11 09:28:36, stefan-webrtc wrote:
> > Does this mean we periodically send REMB if REMB is enabled? Do we have to
> > disable REMB on all modules if we run send-side BWE and aren't interested in
> > REMB?
> 
> I'm not sure, but I think that each module will generate REMB info if the REMB
> status is true, and SetREMBData has ever been called.

yes, if remb is enabled (SetREMBStatus(true)), it will be attached to each
sender/receiver report.

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:261: // TODO(nisse): This test
is a bit pointless. Do we really want to
Is there a use case where same module used both as sender and receiver (audio?)
If not - probably better to remove this test - you do not want to advocate using
same module both as sender and receiver.
If yes - mention the use case, probably with todo to remove this test when use
case will be gone.

May be this test will make sense if different modules will be used as send and
receive (i.e. 'PreferSenderModuleToSendRemb'. in that test it would also make
sense to add receive module 1st and send module 2nd to test remb handover).

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:304: std::vector<uint32_t>
ssrcs(&ssrc, &ssrc + 1);
std::vector<uint32_t> ssrcs = {1234};
likely cleaner.

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:306: EXPECT_CALL(rtp,
REMB()).WillRepeatedly(Return(true));
do you mean ON_CALL here? (i.e. you do not want to test how many times REMB()
was called)

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:324: TEST(PacketRouterRembTest,
UseReceiveModule) {
may be name test 'UseReceiveModuleWhenNoSendModules'
how is it differerent to the last TEST in this file: 'NoSendingRtpModule'?

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:357: MockRtpRtcp rtp_1;  //
TODO(nisse): How is this relevant to this test?
May be add test 'PreferSenderModuleToSendRemb' where modules are called
sender_rtp/receiver_rtp, 
then it will make sense to delete rtp_1 in this and below tests.

[nisse-webrtc, 2017-04-11 14:29:32]

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

[commit-bot: I haz the power, 2017-04-11 14:29:36]

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-04-11 14:30:14]

Updated tests. Do we need any additional tests of the SetREMBStatus logic?

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router.cc (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router.cc:94: }
On 2017/04/11 10:57:41, nisse-webrtc wrote:
> On 2017/04/11 09:28:36, stefan-webrtc wrote:
> > We need unittests which verify this behavior. Or maybe we should have
DCHECKs
> in
> > the add and remove methods that verifies only one module is active?
> 
> I'd like to avoid DCHECKs which iterates over the lists. I could add DCHECKs
> which examine the first or first two elements on each list. Is that enough?
> 
> The PacketRouter unit tests don't cover all the cases of added and removed
> modules.

There are now two unit tests adding both a send and receive module (in different
order) verifying calls to SetREMBStatus. Is that sufficient?

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:261: // TODO(nisse): This test
is a bit pointless. Do we really want to
On 2017/04/11 12:34:14, danilchap wrote:
> Is there a use case where same module used both as sender and receiver
(audio?)
> If not - probably better to remove this test - you do not want to advocate
using
> same module both as sender and receiver.
> If yes - mention the use case, probably with todo to remove this test when use
> case will be gone.
> 
> May be this test will make sense if different modules will be used as send and
> receive (i.e. 'PreferSenderModuleToSendRemb'. in that test it would also make
> sense to add receive module 1st and send module 2nd to test remb handover).

Reworked to test handover.

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:304: std::vector<uint32_t>
ssrcs(&ssrc, &ssrc + 1);
On 2017/04/11 12:34:14, danilchap wrote:
> std::vector<uint32_t> ssrcs = {1234};
> likely cleaner.

Done, all occurences.

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:306: EXPECT_CALL(rtp,
REMB()).WillRepeatedly(Return(true));
On 2017/04/11 12:34:14, danilchap wrote:
> do you mean ON_CALL here? (i.e. you do not want to test how many times REMB()
> was called)

Done. All replaced with ON_CALL(...).WillByDefault(Return(...))

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:324: TEST(PacketRouterRembTest,
UseReceiveModule) {
On 2017/04/11 12:34:14, danilchap wrote:
> may be name test 'UseReceiveModuleWhenNoSendModules'
> how is it differerent to the last TEST in this file: 'NoSendingRtpModule'?

No interesting difference, it just uses a bit stricter EXPECT_CALLs. I'll delete
this, and copy the expectations to the other test.

(I copied tests from vie_remb_unittest.cc quite mindlessly...)

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:357: MockRtpRtcp rtp_1;  //
TODO(nisse): How is this relevant to this test?
On 2017/04/11 12:34:14, danilchap wrote:
> May be add test 'PreferSenderModuleToSendRemb' where modules are called
> sender_rtp/receiver_rtp, 
> then it will make sense to delete rtp_1 in this and below tests.

Deleting rtp_1. There are now two tests  using rtp_send and rtp_recv.

https://codereview.webrtc.org/2789843002/diff/240001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:457: TEST(PacketRouterRembTest,
OnlyOneRembForDoubleProcess) {
On 2017/04/11 09:28:36, stefan-webrtc wrote:
> Process has no meaning below. Should it say OnReceiveBitrateChanged?

Renamed to OnlyOneRembForRepeatedOnReceiveBitrateChanged.

[commit-bot: I haz the power, 2017-04-11 14:31:06]

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

[commit-bot: I haz the power, 2017-04-11 14:31:06]

Dry run: Try jobs failed on following builders:
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24748)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23583)
  mac_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/24774)

[nisse-webrtc, 2017-04-11 14:47:30]

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

[commit-bot: I haz the power, 2017-04-11 14:47:33]

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-04-11 15:21:11]

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

[commit-bot: I haz the power, 2017-04-11 15:21:12]

Dry run: Try jobs failed on following builders:
  linux_memcheck on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/7151)

[danilchap, 2017-04-13 12:27:47]

lgtm % style nits

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:270: const uint32_t
bitrate_estimate = 456;
may be
either remove const, or change variable name to kBitrateEstimate
(same for ssrcs)

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:271: const std::vector<uint32_t>
ssrcs { 1234 };
add = between declaration and the init value.
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:335: uint32_t ssrc[] = {1234,
5678};
is this value needed for anything other than initializing ssrcs?

[nisse-webrtc, 2017-04-18 09:51:17]

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:270: const uint32_t
bitrate_estimate = 456;
On 2017/04/13 12:27:46, danilchap wrote:
> may be
> either remove const, or change variable name to kBitrateEstimate
> (same for ssrcs)

I prefer to not change. |bitrate_estimate| is the name used in the other tests
(whether they modify it or not). And I don't think a const annotation on a local
variable should require a rename.

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:271: const std::vector<uint32_t>
ssrcs { 1234 };
On 2017/04/13 12:27:46, danilchap wrote:
> add = between declaration and the init value.
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-...

Done. (In several places).

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:335: uint32_t ssrc[] = {1234,
5678};
On 2017/04/13 12:27:46, danilchap wrote:
> is this value needed for anything other than initializing ssrcs?

No, I'm moving the list to the initializer for ssrcs, below. Also several
places.

[danilchap, 2017-04-18 10:13:56]

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2789843002/diff/280001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:270: const uint32_t
bitrate_estimate = 456;
On 2017/04/18 09:51:16, nisse-webrtc wrote:
> And I don't think a const annotation on a local
> variable should require a rename.

You right, for local variables it is not required.
Quote from style guide
(https://google.github.io/styleguide/cppguide.html#Constant_Names):
"...with static storage duration should be named this way. This convention is
optional for variables of other storage classes"

[nisse-webrtc, 2017-04-18 11:57:38]

+philipel. I hope removing the suppression (which you added in cl
https://codereview.webrtc.org/2278183002) works fine.

https://codereview.webrtc.org/2789843002/diff/320001/webrtc/modules/pacing/pa...
File webrtc/modules/pacing/packet_router_unittest.cc (right):

https://codereview.webrtc.org/2789843002/diff/320001/webrtc/modules/pacing/pa...
webrtc/modules/pacing/packet_router_unittest.cc:253: EXPECT_CALL(rtp_1,
SendFeedbackPacket(_)).Times(1).WillOnce(Return(true));
This check seems to have fixed the failure on the memcheck bot. Still puzzling
why we could get use of uninitialized values, though.

I've also found suppression, which explains why the appeared after renaming
SendFeedback --> SendTransportFeedback. I'll try deleting that suppression now,
and see if things still work.

[nisse-webrtc, 2017-04-18 12:12:33]

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

[commit-bot: I haz the power, 2017-04-18 12:12:40]

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-04-18 12:13:41]

memcheck looks green. The android failure (which I didn't investigate) seems to
also have disappeared now. I intend to land this soon.

[commit-bot: I haz the power, 2017-04-18 12:54:10]

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

[commit-bot: I haz the power, 2017-04-18 12:54:11]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-04-19 06:35:48]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-04-19 06:35:49]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2789843002/#ps340001 (title:
"Delete obsolete suppression for PacketRouterTest.SendTransportFeedback.")

[commit-bot: I haz the power, 2017-04-19 06:36:01]

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-04-19 06:38:35]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1492583748739470,
"parent_rev": "69d6c00ae993d5ab395c975e925773444270752c", "commit_rev":
"05843312195912a0fab761702991f5041aba9be3"}

[commit-bot: I haz the power, 2017-04-19 06:38:40]

Description was changed from

==========
Delete VieRemb class, move functionality to PacketRouter.

Also rename SendFeedback --> SendTransportFeedback.

BUG=webrtc:7135
==========

to

==========
Delete VieRemb class, move functionality to PacketRouter.

Also rename SendFeedback --> SendTransportFeedback.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2789843002
Cr-Commit-Position: refs/heads/master@{#17755}
Committed:
https://chromium.googlesource.com/external/webrtc/+/05843312195912a0fab761702...
==========

[commit-bot: I haz the power, 2017-04-19 06:38:41]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/external/webrtc/+/05843312195912a0fab761702...