Adding reduced size RTCP configuration down to the video stream level.

Adding reduced size RTCP configuration down to the video stream level.

Still waiting to turn on negotiation (in mediasession.cc)
until we verify it's working as expected.

BUG=webrtc:4868
Committed: https://crrev.com/1387149ad1669365ac05278bf779a407bec08a4e
Cr-Commit-Position: refs/heads/master@{#10958}

[Taylor Brandstetter, 2015-10-23 17:16:32]

deadbeef@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[Taylor Brandstetter, 2015-10-23 17:16:32]

Hi Peter; this just picks up from your CL
(https://codereview.webrtc.org/1308023004), and adds some unit tests, and logic
for ensuring reduced-size RTCP can be turned on/off with
Set(Send|Recv)Parameters.

Should pbos review this too? At least the video engine parts?

https://codereview.webrtc.org/1418123003/diff/1/talk/media/webrtc/webrtcvideo...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1418123003/diff/1/talk/media/webrtc/webrtcvideo...
talk/media/webrtc/webrtcvideoengine2.cc:872:
kv.second->SetSendParameters(send_params);
Here, I could have made a new method, "SetSendReducedSizeRtcp". But that felt
counterproductive since we plan (I assume) to eventually have just one
"SetSendParameters".

[pthatcher1, 2015-10-23 20:40:01]

https://codereview.webrtc.org/1418123003/diff/1/talk/media/webrtc/webrtcvideo...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1418123003/diff/1/talk/media/webrtc/webrtcvideo...
talk/media/webrtc/webrtcvideoengine2.cc:2099: rtc::CritScope cs(&lock_);
Should we do another check to see if it's changed before calling
RecreateWebRtcStream?

https://codereview.webrtc.org/1418123003/diff/1/talk/session/media/mediasessi...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1418123003/diff/1/talk/session/media/mediasessi...
talk/session/media/mediasession.cc:730: // offer->set_rtcp_reduced_size(true);
If we fix VideoSendStream::SignalNetworkState, I think we can offer it.  But
someone will need to be on the hook for making sure that it works correctly.  So
it might make sense to submit this CL without it offering and then ask someone
else to enable it when they feel like supporting it (pbos might be the right
person for that).

https://codereview.webrtc.org/1418123003/diff/1/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/1418123003/diff/1/webrtc/video_send_stream.h#ne...
webrtc/video_send_stream.h:104: // TODO(pbos): Use this to configure the send
stream.
To fix this TODO, I think all we have to do is change
VideoSendStream::SignalNetworkState from this:

  if (state == kNetworkUp)
    vie_channel_->SetRTCPMode(RtcpMode::kCompound);

to this:

  if (state == kNetworkUp)
    vie_channel_->SetRTCPMode(config_.rtp.rtcp_mode);

[Taylor Brandstetter, 2015-11-11 19:42:40]

https://codereview.webrtc.org/1418123003/diff/1/talk/media/webrtc/webrtcvideo...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1418123003/diff/1/talk/media/webrtc/webrtcvideo...
talk/media/webrtc/webrtcvideoengine2.cc:2099: rtc::CritScope cs(&lock_);
On 2015/10/23 20:40:01, pthatcher1 wrote:
> Should we do another check to see if it's changed before calling
> RecreateWebRtcStream? 

For the other methods, only WebRtcVideoChannel2 checks if things changed, not
WebRtcVideoSendStream. So I just kept it this way for consistency.

https://codereview.webrtc.org/1418123003/diff/1/talk/session/media/mediasessi...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1418123003/diff/1/talk/session/media/mediasessi...
talk/session/media/mediasession.cc:730: // offer->set_rtcp_reduced_size(true);
On 2015/10/23 20:40:01, pthatcher1 wrote:
> If we fix VideoSendStream::SignalNetworkState, I think we can offer it.  But
> someone will need to be on the hook for making sure that it works correctly. 
So
> it might make sense to submit this CL without it offering and then ask someone
> else to enable it when they feel like supporting it (pbos might be the right
> person for that).

I'll go with the latter option and change this to TODO(pbos). When I uncomment
these lines, I see the "reduced" configuration propagated to the RTCPSender, but
I don't see any reduced size packets. So maybe there's still work to be done?

https://codereview.webrtc.org/1418123003/diff/1/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/1418123003/diff/1/webrtc/video_send_stream.h#ne...
webrtc/video_send_stream.h:104: // TODO(pbos): Use this to configure the send
stream.
On 2015/10/23 20:40:01, pthatcher1 wrote:
> To fix this TODO, I think all we have to do is change
> VideoSendStream::SignalNetworkState from this:
> 
>   if (state == kNetworkUp)
>     vie_channel_->SetRTCPMode(RtcpMode::kCompound);
> 
> to this:
> 
>   if (state == kNetworkUp)
>     vie_channel_->SetRTCPMode(config_.rtp.rtcp_mode);
> 
> 
> 

Done.

[pthatcher1, 2015-11-23 21:59:06]

lgtm

https://codereview.webrtc.org/1418123003/diff/1/talk/media/webrtc/webrtcvideo...
File talk/media/webrtc/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/1418123003/diff/1/talk/media/webrtc/webrtcvideo...
talk/media/webrtc/webrtcvideoengine2.cc:872:
kv.second->SetSendParameters(send_params);
On 2015/10/23 17:16:32, Taylor Brandstetter wrote:
> Here, I could have made a new method, "SetSendReducedSizeRtcp". But that felt
> counterproductive since we plan (I assume) to eventually have just one
> "SetSendParameters".

Yeah, we want everything to go in SetSendParameters.

[Taylor Brandstetter, 2015-12-01 17:50:39]

The CQ bit was checked by deadbeef@webrtc.org

[commit-bot: I haz the power, 2015-12-01 17:50:49]

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

[commit-bot: I haz the power, 2015-12-01 17:55:20]

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

[commit-bot: I haz the power, 2015-12-01 17:55:21]

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

[Taylor Brandstetter, 2015-12-01 18:23:00]

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

[Taylor Brandstetter, 2015-12-01 18:23:00]

@pbos: Can you take a look at video_send_stream.cc?

[pbos-webrtc, 2015-12-03 00:15:48]

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

[pbos-webrtc, 2015-12-03 00:15:49]

Functionally this looks good, I'd like to get out of owning everything that
should be done. These tags aren't supposed to assign bugs, but rather who said
that something should be done. Can I suggest TODO(deadbeef)?

mflodman@ PTAL at the send stream, if I recall correctly we never wanted to have
rsize sent here.

https://codereview.webrtc.org/1418123003/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2.h (right):

https://codereview.webrtc.org/1418123003/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.h:263: // into this method. Currently this
method only sets the RTCP mode.
Since when is TODO(foo) the person who you want to do it and not the person who
put the TODO there? :D

https://codereview.webrtc.org/1418123003/diff/20001/talk/session/media/medias...
File talk/session/media/mediasession.cc (right):

https://codereview.webrtc.org/1418123003/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:729: // TODO(pbos): Once we're sure this
works correctly, enable it in CreateOffer.
Uh, I'd rather not own this.

https://codereview.webrtc.org/1418123003/diff/20001/talk/session/media/medias...
talk/session/media/mediasession.cc:1011: // TODO(pbos): Once we're sure this
works correctly, enable it in
Same here.

https://codereview.webrtc.org/1418123003/diff/20001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/1418123003/diff/20001/webrtc/video_send_stream....
webrtc/video_send_stream.h:104: RtcpMode rtcp_mode = RtcpMode::kCompound;
I think we intentionally didn't support this for send streams. Wasn't sure if it
was ever useful to have reducedsize for send streams, so this was intentional.

mflodman@: Does reducedsize make sense for senders?

[pbos-webrtc, 2015-12-03 00:16:00]

deadbeef: FYI I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=5296.

[Taylor Brandstetter, 2015-12-03 02:05:41]

On 2015/12/03 00:15:49, pbos-webrtc wrote:
> Functionally this looks good, I'd like to get out of owning everything that
> should be done. These tags aren't supposed to assign bugs, but rather who said
> that something should be done. Can I suggest TODO(deadbeef)?
> 

Sorry about that; I saw "TODO(pbos)"s in Peter's original CL, so I assumed it
was normal to assign other people. They're now reassigned to myself.

[mflodman, 2015-12-03 09:51:03]

This is not propagated to audio, but I assume there are no plans for reduced
size RTCP for audio.

LGTM

[mflodman, 2015-12-03 09:55:29]

On 2015/12/03 09:51:03, mflodman wrote:
> This is not propagated to audio, but I assume there are no plans for reduced
> size RTCP for audio.
> 
> LGTM

And the send side parts might be useful to tell what to expect.

[pbos-webrtc, 2015-12-03 13:25:45]

lgtm,

No Peter has been a bit special about TODOing me. :)

[pthatcher1, 2015-12-03 20:49:20]

On 2015/12/03 13:25:45, pbos-webrtc wrote:
> lgtm,
> 
> No Peter has been a bit special about TODOing me. :)

Actually, according to the style guide, the TODO name should be:

"the username ... of the person or issue with the best context about the problem
referenced by the TODO"

It's not my fault you're the person with the best context :).

[pbos-webrtc, 2015-12-03 20:56:18]

On 2015/12/03 20:49:20, pthatcher1 wrote:
> On 2015/12/03 13:25:45, pbos-webrtc wrote:
> > lgtm,
> > 
> > No Peter has been a bit special about TODOing me. :)
> 
> Actually, according to the style guide, the TODO name should be:
> 
> "the username ... of the person or issue with the best context about the
problem
> referenced by the TODO"
> 
> It's not my fault you're the person with the best context :).

If we think Taylor has enough context I'd like to not be the sole owner of TODOs
here (but I can consider fixing the linked bug regardless). Shared ownership is
good. :)

[Taylor Brandstetter, 2015-12-04 22:08:30]

Description was changed from

==========
Adding reduced size RTCP configuration down to the video stream level.

Still needs to be implemented for the WebRtcVideoSendStream. For that
reason, it's not turned on yet in mediasession.cc.

BUG=webrtc:4868
==========

to

==========
Adding reduced size RTCP configuration down to the video stream level.

Still waiting to turn on negotiation (in mediasession.cc)
until we verify it's working as expected.

BUG=webrtc:4868
==========

[Taylor Brandstetter, 2015-12-04 22:08:57]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-12-04 22:08:58]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1418123003/#ps40001 (title:
"Reassigning TODOs.")

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

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

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

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

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

Try jobs failed on following builders:
  android_clang_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/build...)
  android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/11250)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/9903)
  linux_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  linux_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_dbg/builds/7258)
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/8493)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[Taylor Brandstetter, 2015-12-09 20:35:31]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2015-12-09 20:35:32]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org,
pthatcher@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1418123003/#ps60001 (title:
"Fixing patch conflicts")

[commit-bot: I haz the power, 2015-12-09 20:35:40]

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

[commit-bot: I haz the power, 2015-12-09 20:37:56]

Description was changed from

==========
Adding reduced size RTCP configuration down to the video stream level.

Still waiting to turn on negotiation (in mediasession.cc)
until we verify it's working as expected.

BUG=webrtc:4868
==========

to

==========
Adding reduced size RTCP configuration down to the video stream level.

Still waiting to turn on negotiation (in mediasession.cc)
until we verify it's working as expected.

BUG=webrtc:4868
==========

[commit-bot: I haz the power, 2015-12-09 20:37:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-09 20:38:06]

Description was changed from

==========
Adding reduced size RTCP configuration down to the video stream level.

Still waiting to turn on negotiation (in mediasession.cc)
until we verify it's working as expected.

BUG=webrtc:4868
==========

to

==========
Adding reduced size RTCP configuration down to the video stream level.

Still waiting to turn on negotiation (in mediasession.cc)
until we verify it's working as expected.

BUG=webrtc:4868

Committed: https://crrev.com/1387149ad1669365ac05278bf779a407bec08a4e
Cr-Commit-Position: refs/heads/master@{#10958}
==========

[commit-bot: I haz the power, 2015-12-09 20:38:07]

Patchset 4 (id:??) landed as
https://crrev.com/1387149ad1669365ac05278bf779a407bec08a4e
Cr-Commit-Position: refs/heads/master@{#10958}