Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2817553004:
Add content type extension to capabilities (Closed)
DescriptionAdd content type extension to capabilities
BUG=webrtc:7420
Review-Url: https://codereview.webrtc.org/2817553004
Cr-Commit-Position: refs/heads/master@{#17839}
Committed: https://chromium.googlesource.com/external/webrtc/+/a244ec659d7973e78952065b332511d5533ca9e0
Patch Set 1 #
Total comments: 1
Patch Set 2 : Report new capabilities only if field trial is enabled #Patch Set 3 : Fix test to account for field trial #
Total comments: 4
Patch Set 4 : Implement Brandtr@ comments #
Total comments: 2
Patch Set 5 : Updated commentary as suggested by Brandtr@ #
Total comments: 2
Patch Set 6 : Implement Magjed comments #
Messages
Total messages: 36 (23 generated)
ilnik@webrtc.org changed reviewers: + brandtr@chromium.org, holmer@chromium.org, tommi@chromium.org
Description was changed from ========== Add content type extension to capabilities BUG=webrtc:7420 ========== to ========== Add content type extension to capabilities BUG=webrtc:7420 ==========
ilnik@webrtc.org changed reviewers: + brandtr@webrtc.org, stefan@webrtc.org, tommi@webrtc.org - brandtr@chromium.org, holmer@chromium.org, tommi@chromium.org
I have just briefly looked at this, but I think we should put this header extension behind a field trial until we have clear picture of where and when we should enable it. (For testing purposes, it's easy to enable a field trial in Chrome by supplying the appropriate command line flag.) Should the extension be added here too? https://cs.chromium.org/chromium/src/third_party/webrtc/config.cc?l=83&gsn=Is... Also, we should probably also upload something here: https://webrtc.org/experiments/rtp-hdrext/video-content-type https://codereview.webrtc.org/2817553004/diff/1/webrtc/media/engine/webrtcvid... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2817553004/diff/1/webrtc/media/engine/webrtcvid... webrtc/media/engine/webrtcvideoengine2.cc:506: capabilities.header_extensions.push_back( Will this addition launch the header extension in the default SDP?
On 2017/04/12 16:22:10, brandtr wrote: > I have just briefly looked at this, but I think we should put this header > extension behind a field trial until we have clear picture of where and when we > should enable it. > > (For testing purposes, it's easy to enable a field trial in Chrome by supplying > the appropriate command line flag.) > > Should the extension be added here too? > https://cs.chromium.org/chromium/src/third_party/webrtc/config.cc?l=83&gsn=Is... > It is added there. it's probably not imported to the chrome yet. See my pervious CL: https://codereview.webrtc.org/2816493002/diff/340001/webrtc/config.cc > Also, we should probably also upload something here: > https://webrtc.org/experiments/rtp-hdrext/video-content-type > Yes. By mistake it's uploaded to /content-type, not the /video-content-type. Will fix that. > https://codereview.webrtc.org/2817553004/diff/1/webrtc/media/engine/webrtcvid... > File webrtc/media/engine/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/2817553004/diff/1/webrtc/media/engine/webrtcvid... > webrtc/media/engine/webrtcvideoengine2.cc:506: > capabilities.header_extensions.push_back( > Will this addition launch the header extension in the default SDP? I will add the field trial guard very soon.
Looks good! Just one comment, and some nits :) https://codereview.webrtc.org/2817553004/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2817553004/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:52: bool IsVideoContentTypeExtensionEnabled() { Nits: 1) Add "FieldTrial" to the name of this function, so it's clear on line 511 that this extension is gated behind a field trial. 2) Above this function, add a comment describing what happens when this field trial is enabled. https://codereview.webrtc.org/2817553004/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:54: "WebRTC-VideoContentTypeExtension") == "Enabled"; Replace with a call to this function instead: https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/inclu.... Can explain offline.
https://codereview.webrtc.org/2817553004/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2817553004/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:52: bool IsVideoContentTypeExtensionEnabled() { On 2017/04/13 09:42:50, brandtr wrote: > Nits: > > 1) Add "FieldTrial" to the name of this function, so it's clear on line 511 that > this extension is gated behind a field trial. > 2) Above this function, add a comment describing what happens when this field > trial is enabled. Done. https://codereview.webrtc.org/2817553004/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:54: "WebRTC-VideoContentTypeExtension") == "Enabled"; On 2017/04/13 09:42:50, brandtr wrote: > Replace with a call to this function instead: > https://cs.chromium.org/chromium/src/third_party/webrtc/system_wrappers/inclu.... > > Can explain offline. Done.
lgtm https://codereview.webrtc.org/2817553004/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2817553004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:52: // If this field trial is enabled, we will report VideoContentType RTP extension But the effect of adding this extension to the capabilities means that it will show up in the default offer SDP, and therefore by default will be sent, if negotiated?
https://codereview.webrtc.org/2817553004/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/2817553004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2.cc:52: // If this field trial is enabled, we will report VideoContentType RTP extension On 2017/04/13 10:00:25, brandtr wrote: > But the effect of adding this extension to the capabilities means that it will > show up in the default offer SDP, and therefore by default will be sent, if > negotiated? > Yes. I have updated the comment to be more detailed.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/2817553004/#ps80001 (title: "Updated commentary as suggested by Brandtr@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/16132)
ilnik@webrtc.org changed reviewers: + magjed@webrtc.org
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.webrtc.org/2817553004/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2817553004/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:283: ASSERT_FALSE(extension.uri == RtpExtension::kVideoContentTypeUri); Use assert_eq instead
https://codereview.webrtc.org/2817553004/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/2817553004/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvideoengine2_unittest.cc:283: ASSERT_FALSE(extension.uri == RtpExtension::kVideoContentTypeUri); On 2017/04/24 09:12:23, magjed_webrtc wrote: > Use assert_eq instead Did you mean EXPECT_NE?
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/2817553004/#ps100001 (title: "Implement Magjed comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493035752179970,
"parent_rev": "30cda5ef9823c0ba6783abf68b1a2c355e7382be", "commit_rev":
"a244ec659d7973e78952065b332511d5533ca9e0"}
Message was sent while issue was closed.
Description was changed from ========== Add content type extension to capabilities BUG=webrtc:7420 ========== to ========== Add content type extension to capabilities BUG=webrtc:7420 Review-Url: https://codereview.webrtc.org/2817553004 Cr-Commit-Position: refs/heads/master@{#17839} Committed: https://chromium.googlesource.com/external/webrtc/+/a244ec659d7973e78952065b3... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/a244ec659d7973e78952065b3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
