Move ownership of PacketRouter from CongestionController to Call.

Move ownership of PacketRouter from CongestionController to Call.

And delete the method CongestionController::packet_router.

BUG=None

Committed: https://crrev.com/0245da0fa0eb370dd7b8d8fb1a2da89200fe46ff
Cr-Commit-Position: refs/heads/master@{#15323}

[nisse-webrtc, 2016-11-21 13:23:49]

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

[nisse-webrtc, 2016-11-21 13:23:50]

This cl moves packet_router ownership. Is it a step in the right direction?

[the sun, 2016-11-21 13:55:31]

solenberg@webrtc.org changed reviewers:
+ solenberg@webrtc.org

[the sun, 2016-11-21 13:55:32]

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:93: RTC_DCHECK(packet_router);
Can we DCHECK(remote_bitrate_estimator) too?

[nisse-webrtc, 2016-11-21 14:07:37]

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:93: RTC_DCHECK(packet_router);
On 2016/11/21 13:55:31, the sun wrote:
> Can we DCHECK(remote_bitrate_estimator) too?

Not sure if that's appropriate, it's used only if UseSendSideBwe(config) is true
(whatever that means), and otherwise the corresponding member variable is NULL
(by an initializer where it is declared in the header).

Since the class handles NULL, it makes some sense to allow NULL also for the
constructor argument.

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:134: remote_bitrate_estimator_ =
remote_bitrate_estimator;
A DCHECK(remote_bitrate_estimator) might make more sense at this location?

[the sun, 2016-11-21 15:36:11]

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:93: RTC_DCHECK(packet_router);
On 2016/11/21 14:07:36, nisse-webrtc wrote:
> On 2016/11/21 13:55:31, the sun wrote:
> > Can we DCHECK(remote_bitrate_estimator) too?
> 
> Not sure if that's appropriate, it's used only if UseSendSideBwe(config) is
true
> (whatever that means), and otherwise the corresponding member variable is NULL
> (by an initializer where it is declared in the header).
> 
> Since the class handles NULL, it makes some sense to allow NULL also for the
> constructor argument.

If we can avoid having to handle the null case, that makes the code simpler so
should be preferred.

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:134: remote_bitrate_estimator_ =
remote_bitrate_estimator;
On 2016/11/21 14:07:36, nisse-webrtc wrote:
> A DCHECK(remote_bitrate_estimator) might make more sense at this location?

If Call might be giving us a null in this situation, then yes. If the idea is to
avoid iterating config_.rtp.extensions in DeliverRtp(), it is probably better to
cache the result in a bool.

[nisse-webrtc, 2016-11-23 08:11:44]

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:93: RTC_DCHECK(packet_router);
On 2016/11/21 15:36:11, the sun wrote:
> On 2016/11/21 14:07:36, nisse-webrtc wrote:
> > On 2016/11/21 13:55:31, the sun wrote:
> > > Can we DCHECK(remote_bitrate_estimator) too?
> > 
> > Not sure if that's appropriate, it's used only if UseSendSideBwe(config) is
> true
> > (whatever that means), and otherwise the corresponding member variable is
NULL
> > (by an initializer where it is declared in the header).
> > 
> > Since the class handles NULL, it makes some sense to allow NULL also for the
> > constructor argument.
> 
> If we can avoid having to handle the null case, that makes the code simpler so
> should be preferred.

In this cl, the call site looks like this:

  AudioReceiveStream* receive_stream = new AudioReceiveStream(
      &packet_router_,
      // TODO(nisse): Used only when UseSendSideBwe(config) is true.
      congestion_controller_->GetRemoteBitrateEstimator(true),
      config, config_.audio_state, event_log_);

congestion_controller->GetRemoteBitrateEstimator(true) returns
&congestion_controller->remote_bitrate_estimator_proxy_, so this is non-NULL and
pretty cheap. 

Nevertheless, I'm not sure I like to DCHECK that an input parameter is non-NULL
when there are cases where the parameter is otherwise unused.

https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:134: remote_bitrate_estimator_ =
remote_bitrate_estimator;
On 2016/11/21 15:36:11, the sun wrote:
> On 2016/11/21 14:07:36, nisse-webrtc wrote:
> > A DCHECK(remote_bitrate_estimator) might make more sense at this location?
> 
> If Call might be giving us a null in this situation, then yes.

Currently we never get null. Motivation for a DCHECK here is that we might want
to enforce that UseSendSideBwe implies that we have a valid bitrate estimator.

> If the idea is to
> avoid iterating config_.rtp.extensions in DeliverRtp(), it is probably better
to
> cache the result in a bool.

I'm not entirely sure I follow. You're suggesting always initializing
remote_bitrate_estimator_, add a bool use_remote_bitrate_estimator_, and replace
all "if (remote_bitrate_estimator_)" by "if (use_remote_bitrate_estimator)"? In
other places, I think we prefer using rtc::Optional over extra flags like
use_foo, foo_is_valid etc, and in this case, a raw possibly null pointer seems
to work just as fine as an rtc::Optional.

I guess it might clarify intention for hardcore C++-lovers to write

  rtc::Optional<Foo&> 

instead of 

  Foo*

but I'm not convinced that's a good idea (if it even works)... 

Even if it would be somewhat neat if the implementation of Optional would
recognize it and implement the former in terms of the latter, without the
overhead of an extra bool. (Too go off on a tangent, that wouldn't be 100%
equivalent, because the "a reference can never be null"-thing isn't always true
at runtime).

[stefan-webrtc, 2016-11-23 08:33:53]

PacketRouter is fairly rtp specific at the moment. Should/can we do something
about that? Maybe have different implementations for rtp and quartc. For quartc
I'd expect it to not be doing much? Probably something to look at in a later
step though

[nisse-webrtc, 2016-11-28 12:27:33]

On 2016/11/23 08:11:44, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
> File webrtc/audio/audio_receive_stream.cc (right):
> 
>
https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
> webrtc/audio/audio_receive_stream.cc:93: RTC_DCHECK(packet_router);
> On 2016/11/21 15:36:11, the sun wrote:
> > On 2016/11/21 14:07:36, nisse-webrtc wrote:
> > > On 2016/11/21 13:55:31, the sun wrote:
> > > > Can we DCHECK(remote_bitrate_estimator) too?
> > > 
> > > Not sure if that's appropriate, it's used only if UseSendSideBwe(config)
is
> > true
> > > (whatever that means), and otherwise the corresponding member variable is
> NULL
> > > (by an initializer where it is declared in the header).
> > > 
> > > Since the class handles NULL, it makes some sense to allow NULL also for
the
> > > constructor argument.
> > 
> > If we can avoid having to handle the null case, that makes the code simpler
so
> > should be preferred.
> 
> In this cl, the call site looks like this:
> 
>   AudioReceiveStream* receive_stream = new AudioReceiveStream(
>       &packet_router_,
>       // TODO(nisse): Used only when UseSendSideBwe(config) is true.
>       congestion_controller_->GetRemoteBitrateEstimator(true),
>       config, config_.audio_state, event_log_);
> 
> congestion_controller->GetRemoteBitrateEstimator(true) returns
> &congestion_controller->remote_bitrate_estimator_proxy_, so this is non-NULL
and
> pretty cheap. 
> 
> Nevertheless, I'm not sure I like to DCHECK that an input parameter is
non-NULL
> when there are cases where the parameter is otherwise unused.
> 
>
https://codereview.webrtc.org/2516983004/diff/1/webrtc/audio/audio_receive_st...
> webrtc/audio/audio_receive_stream.cc:134: remote_bitrate_estimator_ =
> remote_bitrate_estimator;
> On 2016/11/21 15:36:11, the sun wrote:
> > On 2016/11/21 14:07:36, nisse-webrtc wrote:
> > > A DCHECK(remote_bitrate_estimator) might make more sense at this location?
> > 
> > If Call might be giving us a null in this situation, then yes.
> 
> Currently we never get null. Motivation for a DCHECK here is that we might
want
> to enforce that UseSendSideBwe implies that we have a valid bitrate estimator.
> 
> > If the idea is to
> > avoid iterating config_.rtp.extensions in DeliverRtp(), it is probably
better
> to
> > cache the result in a bool.
> 
> I'm not entirely sure I follow. You're suggesting always initializing
> remote_bitrate_estimator_, add a bool use_remote_bitrate_estimator_, and
replace
> all "if (remote_bitrate_estimator_)" by "if (use_remote_bitrate_estimator)"?
In
> other places, I think we prefer using rtc::Optional over extra flags like
> use_foo, foo_is_valid etc, and in this case, a raw possibly null pointer seems
> to work just as fine as an rtc::Optional.
> 
> I guess it might clarify intention for hardcore C++-lovers to write
> 
>   rtc::Optional<Foo&> 
> 
> instead of 
> 
>   Foo*
> 
> but I'm not convinced that's a good idea (if it even works)... 
> 
> Even if it would be somewhat neat if the implementation of Optional would
> recognize it and implement the former in terms of the latter, without the
> overhead of an extra bool. (Too go off on a tangent, that wouldn't be 100%
> equivalent, because the "a reference can never be null"-thing isn't always
true
> at runtime).

Note to self: In offline discussion, the conclusion was to always
assign remote_bitrate_estimator, and have the if statements check
config.rtp.transport_cc directly.

[nisse-webrtc, 2016-11-28 13:25:26]

https://codereview.webrtc.org/2516983004/diff/60001/webrtc/modules/congestion...
File
webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h
(right):

https://codereview.webrtc.org/2516983004/diff/60001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/include/mock/mock_congestion_controller.h:48:
MOCK_METHOD1(GetRemoteBitrateEstimator,
This mock method returns a NULL RemoteBitrateEstimator, breaking the change to
require that it is always valid.

Now, audio_receive_stream_unittest.cc should no longer need any congestion
controller. But it needs a RemoteBitreteEstimator.

I'll try using
webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_estimator.h
instead.

[nisse-webrtc, 2016-11-29 08:32:45]

Description was changed from

==========
RFC: Move ownership of PacketRouter from CongestionController to Call.

And delete the method CongestionController::packet_router.

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

to

==========
Move ownership of PacketRouter from CongestionController to Call.

And delete the method CongestionController::packet_router.

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

[stefan-webrtc, 2016-11-29 13:50:11]

lgtm

[nisse-webrtc, 2016-11-29 14:21:49]

Fredrik: ping?

[the sun, 2016-11-29 14:53:13]

thanks for the ping! 

LGTM

[nisse-webrtc, 2016-11-29 14:54:19]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-11-29 14:54:31]

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, 2016-11-29 15:08:41]

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

[commit-bot: I haz the power, 2016-11-29 15:08:42]

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

[nisse-webrtc, 2016-11-29 15:38:27]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-11-29 15:38:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2516983004/#ps100001 (title:
"Attempt to fix building of fuzzer test.")

[commit-bot: I haz the power, 2016-11-29 15:38:35]

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, 2016-11-29 15:57:44]

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

[commit-bot: I haz the power, 2016-11-29 15:57:45]

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

[nisse-webrtc, 2016-11-29 16:05:38]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-11-29 16:05:45]

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, 2016-11-29 16:35:40]

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

[commit-bot: I haz the power, 2016-11-29 16:35:42]

Failed to apply patch for
webrtc/modules/congestion_controller/include/congestion_controller.h:
While running git apply --index -p1;
  error: patch failed:
webrtc/modules/congestion_controller/include/congestion_controller.h:79
  error: webrtc/modules/congestion_controller/include/congestion_controller.h:
patch does not apply

Patch:      
webrtc/modules/congestion_controller/include/congestion_controller.h
Index: webrtc/modules/congestion_controller/include/congestion_controller.h
diff --git
a/webrtc/modules/congestion_controller/include/congestion_controller.h
b/webrtc/modules/congestion_controller/include/congestion_controller.h
index
8663489d54903d2b33c42baf5e72f20ee22f6ee7..632847e18ac70677e819cf3728269ba8be6c34b5
100644
--- a/webrtc/modules/congestion_controller/include/congestion_controller.h
+++ b/webrtc/modules/congestion_controller/include/congestion_controller.h
@@ -56,12 +56,13 @@ class CongestionController : public CallStatsObserver,
public Module {
   CongestionController(Clock* clock,
                        Observer* observer,
                        RemoteBitrateObserver* remote_bitrate_observer,
-                       RtcEventLog* event_log);
+                       RtcEventLog* event_log,
+                       PacketRouter* packet_router);
   CongestionController(Clock* clock,
                        Observer* observer,
                        RemoteBitrateObserver* remote_bitrate_observer,
                        RtcEventLog* event_log,
-                       std::unique_ptr<PacketRouter> packet_router,
+                       PacketRouter* packet_router,
                        std::unique_ptr<PacedSender> pacer);
   virtual ~CongestionController();
 
@@ -79,7 +80,6 @@ class CongestionController : public CallStatsObserver, public
Module {
       bool send_side_bwe);
   virtual int64_t GetPacerQueuingDelayMs() const;
   virtual PacedSender* pacer() { return pacer_.get(); }
-  virtual PacketRouter* packet_router() { return packet_router_.get(); }
   virtual TransportFeedbackObserver* GetTransportFeedbackObserver();
   RateLimiter* GetRetransmissionRateLimiter();
 
@@ -106,7 +106,6 @@ class CongestionController : public CallStatsObserver,
public Module {
   void Process() override;
 
  private:
-  void Init();
   void MaybeTriggerOnNetworkChanged();
 
   bool IsSendQueueFull() const;
@@ -116,7 +115,7 @@ class CongestionController : public CallStatsObserver,
public Module {
                                            int64_t rtt);
   Clock* const clock_;
   Observer* const observer_;
-  const std::unique_ptr<PacketRouter> packet_router_;
+  PacketRouter* const packet_router_;
   const std::unique_ptr<PacedSender> pacer_;
   const std::unique_ptr<RemoteBitrateEstimator> remote_bitrate_estimator_;
   const std::unique_ptr<BitrateController> bitrate_controller_;

[nisse-webrtc, 2016-11-30 10:22:57]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-11-30 10:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2516983004/#ps140001 (title:
"Add back packet_router method and 4-argument constructor.")

[commit-bot: I haz the power, 2016-11-30 10:23:03]

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, 2016-11-30 11:35:20]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1480501377792550,
"parent_rev": "ba173f80bb3ce09acf200c2ff8b17a9e6613552c", "commit_rev":
"3d8e56bf63e80de7075805de167474a2b720498d"}

[commit-bot: I haz the power, 2016-11-30 11:35:23]

Description was changed from

==========
Move ownership of PacketRouter from CongestionController to Call.

And delete the method CongestionController::packet_router.

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

to

==========
Move ownership of PacketRouter from CongestionController to Call.

And delete the method CongestionController::packet_router.

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

[commit-bot: I haz the power, 2016-11-30 11:35:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-30 11:35:35]

Description was changed from

==========
Move ownership of PacketRouter from CongestionController to Call.

And delete the method CongestionController::packet_router.

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

to

==========
Move ownership of PacketRouter from CongestionController to Call.

And delete the method CongestionController::packet_router.

BUG=None

Committed: https://crrev.com/0245da0fa0eb370dd7b8d8fb1a2da89200fe46ff
Cr-Commit-Position: refs/heads/master@{#15323}
==========

[commit-bot: I haz the power, 2016-11-30 11:35:36]

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