Move Ownership of RtpModules to VideoSendStream.

Move Ownership of RtpModules to VideoSendStream from VieChannel and remove use of vie_channel and vie_receiver from video_send_stream.

The purpose of this refactoring is a first step of separating the encoder parts from the RTP transport.

BUG=webrtc:5687
R=mflodman@webrtc.org, pbos@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/83d09106943b3b70d9aaa92bc674fae473443f89
Cr-Commit-Position: refs/heads/master@{#12377}

[perkj_webrtc, 2016-04-07 14:00:05]

Description was changed from

==========
Move Ownership of RtpModules to PayLoadRouter.
Remove use of vie_channel and vie_receiver from video_send_stream.
BUG=
==========

to

==========
Move Ownership of RtpModules to VideoSendStream from VieChannel and remove use
of vie_channel and vie_receiver from video_send_stream.

The purpose of this refactoring is a first step of separating the encoder parts
from the RTP transport.

BUG=webrtc:5687
==========

[perkj_webrtc, 2016-04-07 14:00:05]

perkj@webrtc.org changed reviewers:
+ pbos@webrtc.org

[perkj_webrtc, 2016-04-07 14:00:21]

Can you take a first pass?

[pbos-webrtc, 2016-04-07 16:02:08]

First pass, adding both stefan@ and mflodman@ since this is large and scary and
we don't wanna get it wrong.

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

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:249: ->CreateRtcpBandwidthObserver(),
This one leaks, perhaps it should go into a scoped_ptr?

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:255: &stats_proxy_,
Since this function is only used here can you replace the four &stats_proxy_'s
here with one SendStatisticsProxy argument to CreateRtpRtcpModules?

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:281: payload_router_.SetSendingRtpModules(1);
Should this just default to 1 within PayloadRouter?

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:285: //  NOW! This only seems to be used
receive side from looking at the code??
Stefan, do you know what these CallStats are?

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:321: uint8_t payload_type_red = 0;
Can you break out this block into a separate method?

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:421: // 
congestion_controller_->GetRemoteBitrateEstimator(false)->RemoveStream(
Looks removable, don't think we need to remote estimate anything from a sender's
perspective. Stefan?

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:681: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
Can't we use rtp_rtcp_modules_[i]->GetRtpStateForSsrc(ssrc) directly? instead of
mapping from index to SSRC and then go through all modules?

Maybe remove and say GetRtpState and GetRtxState if we no longer need to provide
SSRC as input to the module?

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

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:85: // implements
webrtc::VCMProtectionCallback.
Implements

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:118: // RtpRtcp modules, declared here as they 
use other members on construction.
remove double space.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:138: if (paced_sender_) {
I'd expect that paced_sender_ is either always true or false here and we
shouldn't have to check it anymore.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:143: if (vcm_->RegisterReceiveCallback(this) != 0) {
I think we could RTC_CHECK_EQ this and make it void

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel.h
File webrtc/video/vie_channel.h (right):

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.h:221: const std::unique_ptr<RtpRtcp>
rtp_rtcp_modules_;
rtp_rtcp_ instead of modules_

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_receiver.h
File webrtc/video/vie_receiver.h (right):

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_receiver...
webrtc/video/vie_receiver.h:105: RtpRtcp* rtp_rtcp_;  // Owned by VieChannel
ViEChannel.

[pbos-webrtc, 2016-04-07 16:03:22]

pbos@webrtc.org changed reviewers:
+ mflodman@webrtc.org, stefan@webrtc.org

[perkj_webrtc, 2016-04-08 10:59:04]

Addressed pbos comments.

Stefan - would you mind taking a look as well?

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

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:249: ->CreateRtcpBandwidthObserver(),
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> This one leaks, perhaps it should go into a scoped_ptr?

Done.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:255: &stats_proxy_,
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> Since this function is only used here can you replace the four &stats_proxy_'s
> here with one SendStatisticsProxy argument to CreateRtpRtcpModules?

Done.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:281: payload_router_.SetSendingRtpModules(1);
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> Should this just default to 1 within PayloadRouter?

Done.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:321: uint8_t payload_type_red = 0;
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> Can you break out this block into a separate method?

Done.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:681: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> Can't we use rtp_rtcp_modules_[i]->GetRtpStateForSsrc(ssrc) directly? instead
of
> mapping from index to SSRC and then go through all modules?
> 
> Maybe remove and say GetRtpState and GetRtxState if we no longer need to
provide
> SSRC as input to the module?

hum.... right...

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

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:85: // implements
webrtc::VCMProtectionCallback.
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> Implements

Done.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:118: // RtpRtcp modules, declared here as they 
use other members on construction.
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> remove double space.

Done.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:138: if (paced_sender_) {
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> I'd expect that paced_sender_ is either always true or false here and we
> shouldn't have to check it anymore.

Looks like this is only nack - send side.
Removing.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:143: if (vcm_->RegisterReceiveCallback(this) != 0) {
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> I think we could RTC_CHECK_EQ this and make it void

not part of this cl.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel.h
File webrtc/video/vie_channel.h (right):

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.h:221: const std::unique_ptr<RtpRtcp>
rtp_rtcp_modules_;
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> rtp_rtcp_ instead of modules_

Done.

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_receiver.h
File webrtc/video/vie_receiver.h (right):

https://codereview.webrtc.org/1864313003/diff/80001/webrtc/video/vie_receiver...
webrtc/video/vie_receiver.h:105: RtpRtcp* rtp_rtcp_;  // Owned by VieChannel
On 2016/04/07 16:02:08, pbos-webrtc wrote:
> ViEChannel.

Done.

[pbos-webrtc, 2016-04-11 09:35:23]

LG, I'd like to check when all is done, some // ???s that need to be removed
pending other review feedback

[perkj_webrtc, 2016-04-11 10:42:53]

On 2016/04/11 09:35:23, pbos-webrtc wrote:
> LG, I'd like to check when all is done, some // ???s that need to be removed
> pending other review feedback

Thanks- Stefan- will you have time?

[perkj_webrtc, 2016-04-12 07:20:03]

On 2016/04/11 10:42:53, perkj_webrtc wrote:
> On 2016/04/11 09:35:23, pbos-webrtc wrote:
> > LG, I'd like to check when all is done, some // ???s that need to be removed
> > pending other review feedback
> 
> Thanks- Stefan- will you have time?

friendly ping

[stefan-webrtc, 2016-04-12 07:59:11]

A first pass.

I'd really like to see a short document with a simple class diagram showing
where we want to arrive when this refactoring is done. Will we end up with a big
video_send_stream.cc file, or is the goal to introduce new classes, like
RtpTransport? How will they be connected, etc. Nothing complicated, just a few
boxes and arrows showing what we have now and what we want. Does that sound
reasonable?

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/payload_rou...
File webrtc/video/payload_router.cc (right):

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:21: SetSendingRtpModules(1);
Is it necessary to take the lock in SetSendingRtpModules here? Maybe we can have
a version of the method which doesn't take the lock.

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/payload_rou...
File webrtc/video/payload_router_unittest.cc (left):

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/payload_rou...
webrtc/video/payload_router_unittest.cc:146: EXPECT_EQ(kDefaultMaxLength,
payload_router_->MaxPayloadLength());
Any reason for removing this EXPECT?

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:277: //  NOW! This only seems to be used
receive side from looking at the code??
AFAICT you are right.

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:361: // What is this???????
Looks incorrect to me, and should be removed since the send stream doesn't use a
remote bitrate estimator when not doing send-side bwe.

[perkj_webrtc, 2016-04-13 11:43:49]

PTAL 

This cl moves the ownership of the RTP module remove the dependency from receive
only ViEChannel. 

Regardless of how we decide the transport and encoder should interact- it should
not depend on ViEChannel.

A document is in progress here: 
https://docs.google.com/document/d/1XF3gTtpYS-Fi5gU1VT3YTZ1LC1Rp5ruwxCaMarxVx...

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/payload_rou...
File webrtc/video/payload_router.cc (right):

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:21: SetSendingRtpModules(1);
On 2016/04/12 07:59:10, stefan-webrtc (holmer) wrote:
> Is it necessary to take the lock in SetSendingRtpModules here? Maybe we can
have
> a version of the method which doesn't take the lock.

no, its not since it is the ctor. done

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/payload_rou...
File webrtc/video/payload_router_unittest.cc (left):

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/payload_rou...
webrtc/video/payload_router_unittest.cc:146: EXPECT_EQ(kDefaultMaxLength,
payload_router_->MaxPayloadLength());
On 2016/04/12 07:59:10, stefan-webrtc (holmer) wrote:
> Any reason for removing this EXPECT?

We now set the first rtp module as sending in the ctor. 
Previously that was also done but from video_send_stream. 
MaxPayloadLength() will then return the the max payload length from the rtp
module.
This is covered by the test below and this extra call did not make sense.

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:277: //  NOW! This only seems to be used
receive side from looking at the code??
On 2016/04/12 07:59:10, stefan-webrtc (holmer) wrote:
> AFAICT you are right.

Done.

https://codereview.webrtc.org/1864313003/diff/120001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:361: // What is this???????
On 2016/04/12 07:59:10, stefan-webrtc (holmer) wrote:
> Looks incorrect to me, and should be removed since the send stream doesn't use
a
> remote bitrate estimator when not doing send-side bwe.

Done.

[pbos-webrtc, 2016-04-14 14:56:42]

lgtm

[mflodman, 2016-04-15 08:25:42]

LG, but with one ask for one more modification.

https://codereview.webrtc.org/1864313003/diff/140001/webrtc/video/payload_rou...
File webrtc/video/payload_router.cc (right):

https://codereview.webrtc.org/1864313003/diff/140001/webrtc/video/payload_rou...
webrtc/video/payload_router.cc:21: UpdateModuleSendingState();
This means the RTP module will be in a sending state from the start, but I
assume in practice there will always be a call to SetSendingModules very soon
after anyway and this is fine.

https://codereview.webrtc.org/1864313003/diff/140001/webrtc/video/vie_receiver.h
File webrtc/video/vie_receiver.h (right):

https://codereview.webrtc.org/1864313003/diff/140001/webrtc/video/vie_receive...
webrtc/video/vie_receiver.h:105: RtpRtcp* rtp_rtcp_;  // Owned by ViEChannel
Maybe resolve pbos' todo when at this, should be easy since Init(...) is called
from the ViEChannel ctor anyway.

[perkj_webrtc, 2016-04-15 08:36:43]

https://codereview.webrtc.org/1864313003/diff/140001/webrtc/video/vie_receiver.h
File webrtc/video/vie_receiver.h (right):

https://codereview.webrtc.org/1864313003/diff/140001/webrtc/video/vie_receive...
webrtc/video/vie_receiver.h:105: RtpRtcp* rtp_rtcp_;  // Owned by ViEChannel
On 2016/04/15 08:25:42, mflodman wrote:
> Maybe resolve pbos' todo when at this, should be easy since Init(...) is
called
> from the ViEChannel ctor anyway.

Its not that easy since there is a circular dependency in ViEChannel between
VieReceiver and the ctor of the RTP module. So since that is unrelated to this
cl I would like to leave as is.

[mflodman, 2016-04-15 08:40:15]

Doh, I totally missed that :/

LGTM

[stefan-webrtc, 2016-04-15 09:09:05]

lgtm

[perkj_webrtc, 2016-04-15 09:33:15]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-04-15 09:33:15]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1864313003/#ps160001 (title:
"Rebased")

[commit-bot: I haz the power, 2016-04-15 09:33:32]

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

[commit-bot: I haz the power, 2016-04-15 11:02:33]

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

[commit-bot: I haz the power, 2016-04-15 11:02:34]

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

[perkj_webrtc, 2016-04-15 12:59:25]

Description was changed from

==========
Move Ownership of RtpModules to VideoSendStream from VieChannel and remove use
of vie_channel and vie_receiver from video_send_stream.

The purpose of this refactoring is a first step of separating the encoder parts
from the RTP transport.

BUG=webrtc:5687
==========

to

==========
Move Ownership of RtpModules to VideoSendStream from VieChannel and remove use
of vie_channel and vie_receiver from video_send_stream.

The purpose of this refactoring is a first step of separating the encoder parts
from the RTP transport.

BUG=webrtc:5687
R=mflodman@webrtc.org, pbos@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/83d09106943b3b70d9aaa92bc...
==========

[perkj_webrtc, 2016-04-15 12:59:27]

Committed patchset #9 (id:160001) manually as
83d09106943b3b70d9aaa92bc674fae473443f89 (presubmit successful).

[commit-bot: I haz the power, 2016-04-15 12:59:28]

Description was changed from

==========
Move Ownership of RtpModules to VideoSendStream from VieChannel and remove use
of vie_channel and vie_receiver from video_send_stream.

The purpose of this refactoring is a first step of separating the encoder parts
from the RTP transport.

BUG=webrtc:5687
R=mflodman@webrtc.org, pbos@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/83d09106943b3b70d9aaa92bc...
==========

to

==========
Move Ownership of RtpModules to VideoSendStream from VieChannel and remove use
of vie_channel and vie_receiver from video_send_stream.

The purpose of this refactoring is a first step of separating the encoder parts
from the RTP transport.

BUG=webrtc:5687
R=mflodman@webrtc.org, pbos@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/83d09106943b3b70d9aaa92bc674fae473443f89
Cr-Commit-Position: refs/heads/master@{#12377}
==========

[commit-bot: I haz the power, 2016-04-15 12:59:29]

Patchset 9 (id:??) landed as
https://crrev.com/83d09106943b3b70d9aaa92bc674fae473443f89
Cr-Commit-Position: refs/heads/master@{#12377}