Require negotiation to send transport cc feedback over RTCP.

Require negotiation to send transport cc feedback over RTCP.

BUG=4312
Committed: https://crrev.com/43edf0ffb91a50e2efa01c7befe4d188a7e30ea2
Cr-Commit-Position: refs/heads/master@{#10735}

[stefan-webrtc, 2015-11-16 22:51:19]

Description was changed from

==========
Require negotiation to send transport cc feedback over RTCP.

BUG=4173
==========

to

==========
Require negotiation to send transport cc feedback over RTCP.

BUG=4312
==========

[stefan-webrtc, 2015-11-17 04:41:34]

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

[commit-bot: I haz the power, 2015-11-17 04:41:47]

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

[stefan-webrtc, 2015-11-17 05:09:56]

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

[commit-bot: I haz the power, 2015-11-17 05:32:14]

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

[commit-bot: I haz the power, 2015-11-17 05:32:15]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2015-11-17 11:13:25]

lgtm for the code change, but maybe you should do a negative test that verifies
that no transport cc are sent when this is off?

[stefan-webrtc, 2015-11-17 14:20:48]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-11-17 14:20:51]

The CQ bit was unchecked by stefan@webrtc.org

[commit-bot: I haz the power, 2015-11-17 14:20:58]

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

[stefan-webrtc, 2015-11-17 14:21:24]

On 2015/11/17 14:20:58, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1452883002/20001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1452883002/20001

Sorry, pulled the trigger on the wrong CL... Aborted. :)

[stefan-webrtc, 2015-11-18 15:41:24]

pthatcher, any chance you'll have time to take a look at this today? Thanks!

[pthatcher, 2015-11-19 00:15:09]

On 2015/11/18 15:41:24, stefan-webrtc (holmer) wrote:
> pthatcher, any chance you'll have time to take a look at this today? Thanks!

Looking now

[pthatcher, 2015-11-19 00:19:19]

pthatcher@google.com changed reviewers:
+ pthatcher@google.com

[pthatcher, 2015-11-19 00:19:20]

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/base/codec.cc
File talk/media/base/codec.cc (right):

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/base/codec.cc#...
talk/media/base/codec.cc:337: bool HasTransportCcFeedback(const VideoCodec&
codec) {
I think just HasTransportCc would be sufficient and match HasRemb and HasNack.

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/base/constants.cc
File talk/media/base/constants.cc (right):

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/base/constants...
talk/media/base/constants.cc:93: const char kRtcpFbParamTransportCc[] =
"goog-transport-cc";
I thought we weren't doing goog- prefixes anymore.  Maybe check with Harald.

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

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1233: config.rtp.transport_cc_feedback =
transport_cc would be more consistent with other names, such as remb.

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

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.h:406: bool transport_cc_enabled);
Why not SetFeedbackParameters or SetFeedbackParams?

[stefan-webrtc, 2015-11-19 00:21:56]

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

[stefan-webrtc, 2015-11-19 00:21:56]

Harald, could you give your opinion on how we are going to add the new feedback
message in sdp? Is it right to do "goog-", or is it better to do it some other
way?

[stefan-webrtc, 2015-11-19 00:22:29]

On 2015/11/19 00:21:56, stefan-webrtc (holmer) wrote:
> Harald, could you give your opinion on how we are going to add the new
feedback
> message in sdp? Is it right to do "goog-", or is it better to do it some other
> way?

I guess one option would be to simply remove the "goog-" prefix?

[pthatcher1, 2015-11-19 00:57:13]

On 2015/11/19 00:22:29, stefan-webrtc (holmer) wrote:
> On 2015/11/19 00:21:56, stefan-webrtc (holmer) wrote:
> > Harald, could you give your opinion on how we are going to add the new
> feedback
> > message in sdp? Is it right to do "goog-", or is it better to do it some
other
> > way?
> 
> I guess one option would be to simply remove the "goog-" prefix?

I vote for that.

[stefan-webrtc, 2015-11-20 16:46:13]

Comments addressed

[stefan-webrtc, 2015-11-20 16:58:48]

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/base/codec.cc
File talk/media/base/codec.cc (right):

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/base/codec.cc#...
talk/media/base/codec.cc:337: bool HasTransportCcFeedback(const VideoCodec&
codec) {
On 2015/11/19 00:19:20, pthatcher wrote:
> I think just HasTransportCc would be sufficient and match HasRemb and HasNack.

Done.

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/base/constants.cc
File talk/media/base/constants.cc (right):

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/base/constants...
talk/media/base/constants.cc:93: const char kRtcpFbParamTransportCc[] =
"goog-transport-cc";
On 2015/11/19 00:19:20, pthatcher wrote:
> I thought we weren't doing goog- prefixes anymore.  Maybe check with Harald.

Done.

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

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.cc:1233: config.rtp.transport_cc_feedback =
On 2015/11/19 00:19:20, pthatcher wrote:
> transport_cc would be more consistent with other names, such as remb.

Done.

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

https://codereview.webrtc.org/1452883002/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2.h:406: bool transport_cc_enabled);
On 2015/11/19 00:19:20, pthatcher wrote:
> Why not SetFeedbackParameters or SetFeedbackParams?

Done.

[stefan-webrtc, 2015-11-20 17:34:44]

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

[commit-bot: I haz the power, 2015-11-20 17:34:53]

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

[commit-bot: I haz the power, 2015-11-20 19:35:04]

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

[commit-bot: I haz the power, 2015-11-20 19:35:05]

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

[pthatcher1, 2015-11-20 22:11:40]

lgtm

[stefan-webrtc, 2015-11-20 22:31:23]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-11-20 22:31:23]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1452883002/#ps40001 (title:
"Comments addressed")

[commit-bot: I haz the power, 2015-11-20 22:31:31]

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

[commit-bot: I haz the power, 2015-11-20 22:33:25]

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

[commit-bot: I haz the power, 2015-11-20 22:33:26]

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

[stefan-webrtc, 2015-11-20 22:41:02]

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

[stefan-webrtc, 2015-11-20 22:41:03]

Magnus, please take a look at the files in webrtc/ root

[stefan-webrtc, 2015-11-20 22:41:22]

webrtc/video_receive_stream.h, that is... :)

[mflodman, 2015-11-20 22:48:53]

LGTM

[stefan-webrtc, 2015-11-20 22:49:17]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2015-11-20 22:49:23]

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

[commit-bot: I haz the power, 2015-11-21 00:31:44]

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

[commit-bot: I haz the power, 2015-11-21 00:31:45]

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

[stefan-webrtc, 2015-11-21 01:57:25]

The CQ bit was checked by stefan@webrtc.org

[commit-bot: I haz the power, 2015-11-21 01:57:33]

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

[commit-bot: I haz the power, 2015-11-21 02:05:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-21 02:06:01]

Patchset 3 (id:??) landed as
https://crrev.com/43edf0ffb91a50e2efa01c7befe4d188a7e30ea2
Cr-Commit-Position: refs/heads/master@{#10735}