Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1481963002:
Add header extension filtering for WebRtcVoiceEngine/MediaChannel. (Closed)
DescriptionAdd header extension filtering for WebRtcVoiceEngine/MediaChannel.
Rework filtering functionality to be reused for both Audio+Video.
BUG=webrtc:4690, webrtc:4254
Patch Set 1 #Patch Set 2 : Not being silent anymore. #Patch Set 3 : remove comment #Patch Set 4 : Simpler implementation of set<>, requiring no heap allocs. #
Total comments: 22
Patch Set 5 : comments #
Total comments: 1
Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : comment #Patch Set 8 : rebase #Patch Set 9 : two more test cases #
Messages
Total messages: 29 (11 generated)
solenberg@webrtc.org changed reviewers: + stefan@webrtc.org
Stefan, before you dive in, let's hash something out: Does it really make sense that WViE2 fails SetSendRtpHeaderExtensions() if any one of them was invalid? This data is coming from the SDP, in unvalidated form. Wouldn't it be better to do what we can, but log any anomalies?
On 2015/11/27 16:04:49, the sun wrote: > Stefan, before you dive in, let's hash something out: > > Does it really make sense that WViE2 fails SetSendRtpHeaderExtensions() if any > one of them was invalid? This data is coming from the SDP, in unvalidated form. > Wouldn't it be better to do what we can, but log any anomalies? Oh, and once we've worked that out, I'll add the appropriate tests to webrtcmediaengine_unittest.cc.
On 2015/11/27 16:06:09, the sun wrote: > On 2015/11/27 16:04:49, the sun wrote: > > Stefan, before you dive in, let's hash something out: > > > > Does it really make sense that WViE2 fails SetSendRtpHeaderExtensions() if any > > one of them was invalid? This data is coming from the SDP, in unvalidated > form. > > Wouldn't it be better to do what we can, but log any anomalies? > > Oh, and once we've worked that out, I'll add the appropriate tests to > webrtcmediaengine_unittest.cc. After offline discussions on Friday I think the conclusion was that it does make sense to fail if the extensions are invalid since webrtc has told the user what extensions it actually supports.
On 2015/11/30 10:16:16, stefan-webrtc (holmer) wrote: > On 2015/11/27 16:06:09, the sun wrote: > > On 2015/11/27 16:04:49, the sun wrote: > > > Stefan, before you dive in, let's hash something out: > > > > > > Does it really make sense that WViE2 fails SetSendRtpHeaderExtensions() if > any > > > one of them was invalid? This data is coming from the SDP, in unvalidated > > form. > > > Wouldn't it be better to do what we can, but log any anomalies? > > > > Oh, and once we've worked that out, I'll add the appropriate tests to > > webrtcmediaengine_unittest.cc. > > After offline discussions on Friday I think the conclusion was that it does make > sense to fail if the extensions are invalid since webrtc has told the user what > extensions it actually supports. Based on this, do you want me to wait with reviewing?
On 2015/11/30 10:16:41, stefan-webrtc (holmer) wrote: > On 2015/11/30 10:16:16, stefan-webrtc (holmer) wrote: > > On 2015/11/27 16:06:09, the sun wrote: > > > On 2015/11/27 16:04:49, the sun wrote: > > > > Stefan, before you dive in, let's hash something out: > > > > > > > > Does it really make sense that WViE2 fails SetSendRtpHeaderExtensions() if > > any > > > > one of them was invalid? This data is coming from the SDP, in unvalidated > > > form. > > > > Wouldn't it be better to do what we can, but log any anomalies? > > > > > > Oh, and once we've worked that out, I'll add the appropriate tests to > > > webrtcmediaengine_unittest.cc. > > > > After offline discussions on Friday I think the conclusion was that it does > make > > sense to fail if the extensions are invalid since webrtc has told the user > what > > extensions it actually supports. > > Based on this, do you want me to wait with reviewing? No, wait with review, I'll ping you when there is test coverage etc.
On 2015/11/30 10:43:41, the sun wrote: > On 2015/11/30 10:16:41, stefan-webrtc (holmer) wrote: > > On 2015/11/30 10:16:16, stefan-webrtc (holmer) wrote: > > > On 2015/11/27 16:06:09, the sun wrote: > > > > On 2015/11/27 16:04:49, the sun wrote: > > > > > Stefan, before you dive in, let's hash something out: > > > > > > > > > > Does it really make sense that WViE2 fails SetSendRtpHeaderExtensions() > if > > > any > > > > > one of them was invalid? This data is coming from the SDP, in > unvalidated > > > > form. > > > > > Wouldn't it be better to do what we can, but log any anomalies? > > > > > > > > Oh, and once we've worked that out, I'll add the appropriate tests to > > > > webrtcmediaengine_unittest.cc. > > > > > > After offline discussions on Friday I think the conclusion was that it does > > make > > > sense to fail if the extensions are invalid since webrtc has told the user > > what > > > extensions it actually supports. > > > > Based on this, do you want me to wait with reviewing? > > No, wait with review, I'll ping you when there is test coverage etc. PTAL
Description was changed from ========== Add header extension filtering for WebRtcVoiceEngine/MediaChannel. Rework filtering functionality to be reused for both Audio+Video. BUG=webrtc:4690 ========== to ========== Add header extension filtering for WebRtcVoiceEngine/MediaChannel. Rework filtering functionality to be reused for both Audio+Video. BUG=webrtc:4690 ==========
solenberg@webrtc.org changed reviewers: + tommi@webrtc.org - stefan@webrtc.org
On 2015/11/30 16:21:54, the sun wrote: > On 2015/11/30 10:43:41, the sun wrote: > > On 2015/11/30 10:16:41, stefan-webrtc (holmer) wrote: > > > On 2015/11/30 10:16:16, stefan-webrtc (holmer) wrote: > > > > On 2015/11/27 16:06:09, the sun wrote: > > > > > On 2015/11/27 16:04:49, the sun wrote: > > > > > > Stefan, before you dive in, let's hash something out: > > > > > > > > > > > > Does it really make sense that WViE2 fails > SetSendRtpHeaderExtensions() > > if > > > > any > > > > > > one of them was invalid? This data is coming from the SDP, in > > unvalidated > > > > > form. > > > > > > Wouldn't it be better to do what we can, but log any anomalies? > > > > > > > > > > Oh, and once we've worked that out, I'll add the appropriate tests to > > > > > webrtcmediaengine_unittest.cc. > > > > > > > > After offline discussions on Friday I think the conclusion was that it > does > > > make > > > > sense to fail if the extensions are invalid since webrtc has told the user > > > what > > > > extensions it actually supports. > > > > > > Based on this, do you want me to wait with reviewing? > > > > No, wait with review, I'll ping you when there is test coverage etc. > > PTAL Changing reviewer to tommi@ because of OWNERs requirement for libjingle_tests.gyp and talk/media/base.
tommi@webrtc.org changed reviewers: + stefan@webrtc.org
rs lgtm for libjingle_tests.gyp and talk/media/base. Stefan, can you take a look at the rest?
https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.cc (right): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:75: // Remove mutually exclusive extensions with lower priority. Maybe comment on that extension_prios are sorted in decreasing priority? https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:89: } Remove {}, or keep if you prefer. :) https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:133: }); Why do we have to sort? https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:135: // When sending, remove unnecessary extensions. I don't see anything taking sending or receiving into account below? https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.h (right): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.h:61: bool (*supported)(const std::string&), Comment on what the parameters are. Especially supported and filter_redundant_extensions. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine_unittest.cc (right): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:66: bool AscendingNameOrder(const std::vector<webrtc::RtpExtension>& extensions) { HasAscendingNameOrder or Verify..., maybe? https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:97: TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_1) { SameNameOverlappingIds? https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:103: TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_2) { Does this test anything different from the previous? https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:112: extensions.back().id /= 2; Just want to verify: this is supposed to test that two extensions with different names can't have the same ID? Would be good to name the test to reflect that. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2.cc (left): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1511: << RtpExtensionsToString(extensions); There might be a point to keep this since all API methods of this class seem to start with a LOG?
https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.cc (right): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:75: // Remove mutually exclusive extensions with lower priority. On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > Maybe comment on that extension_prios are sorted in decreasing priority? Done. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:89: } On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > Remove {}, or keep if you prefer. :) Acknowledged. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:133: }); On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > Why do we have to sort? Done. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:135: // When sending, remove unnecessary extensions. On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > I don't see anything taking sending or receiving into account below? Done. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.h (right): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.h:61: bool (*supported)(const std::string&), On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > Comment on what the parameters are. Especially supported and > filter_redundant_extensions. Done. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine_unittest.cc (right): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:66: bool AscendingNameOrder(const std::vector<webrtc::RtpExtension>& extensions) { On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > HasAscendingNameOrder or Verify..., maybe? Done. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:97: TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_1) { On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > SameNameOverlappingIds? True, better use different names. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:103: TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_2) { On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > Does this test anything different from the previous? I was thinking start/end of set would be good to test. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:112: extensions.back().id /= 2; On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > Just want to verify: this is supposed to test that two extensions with different > names can't have the same ID? Would be good to name the test to reflect that. I've removed this specific test. Doesn't "OverlappingIds" tell you that for the previous two? https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2.cc (left): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2.cc:1511: << RtpExtensionsToString(extensions); On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > There might be a point to keep this since all API methods of this class seem to > start with a LOG? I've added log statements in Set[Recv|Send]Parameters instead.
https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine_unittest.cc (right): https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:103: TEST(WebRtcMediaEngineTest, ValidateRtpExtensions_OverlappingIds_2) { On 2015/12/01 16:34:45, the sun wrote: > On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > > Does this test anything different from the previous? > > I was thinking start/end of set would be good to test. Acknowledged. https://codereview.webrtc.org/1481963002/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine_unittest.cc:112: extensions.back().id /= 2; On 2015/12/01 16:34:45, the sun wrote: > On 2015/12/01 15:36:57, stefan-webrtc (holmer) wrote: > > Just want to verify: this is supposed to test that two extensions with > different > > names can't have the same ID? Would be good to name the test to reflect that. > > I've removed this specific test. Doesn't "OverlappingIds" tell you that for the > previous two? Acknowledged. https://codereview.webrtc.org/1481963002/diff/80001/talk/media/base/mediachan... File talk/media/base/mediachannel.h (right): https://codereview.webrtc.org/1481963002/diff/80001/talk/media/base/mediachan... talk/media/base/mediachannel.h:939: virtual std::string ToString() const { Nice! https://codereview.webrtc.org/1481963002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcmediaengine.cc (right): https://codereview.webrtc.org/1481963002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcmediaengine.cc:130: // specified in a different order. I guess it could be mentioned that this basically is a prerequisite for unique. https://codereview.webrtc.org/1481963002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcmediaengine.cc:141: }); In theory I think you could filter out redundant extensions here too if you made sure to take mutual exclusiveness and priorities into account when sorting and in this lambda. May not be an improvement though, what do you think?
https://codereview.webrtc.org/1481963002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcmediaengine.cc (right): https://codereview.webrtc.org/1481963002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcmediaengine.cc:130: // specified in a different order. On 2015/12/02 09:26:25, stefan-webrtc (holmer) wrote: > I guess it could be mentioned that this basically is a prerequisite for unique. Done. https://codereview.webrtc.org/1481963002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcmediaengine.cc:141: }); On 2015/12/02 09:26:25, stefan-webrtc (holmer) wrote: > In theory I think you could filter out redundant extensions here too if you made > sure to take mutual exclusiveness and priorities into account when sorting and > in this lambda. May not be an improvement though, what do you think? I guess, theoretically, but given how unique works (by comparing consecutive elements), it would require a more intricate implementation of the sorting predicate. It would expand the test space quite a bit so I vote no but am intrigued by the idea... :)
lgtm
The CQ bit was checked by solenberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1481963002/#ps160001 (title: "two more test cases")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1481963002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1481963002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_msan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/6355)
The CQ bit was checked by solenberg@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1481963002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1481963002/160001
Message was sent while issue was closed.
Description was changed from ========== Add header extension filtering for WebRtcVoiceEngine/MediaChannel. Rework filtering functionality to be reused for both Audio+Video. BUG=webrtc:4690 ========== to ========== Add header extension filtering for WebRtcVoiceEngine/MediaChannel. Rework filtering functionality to be reused for both Audio+Video. BUG=webrtc:4690 Committed: https://crrev.com/7e4e01a4413fa98644b94ab9d8a9dccc664f39f2 Cr-Commit-Position: refs/heads/master@{#10869} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7e4e01a4413fa98644b94ab9d8a9dccc664f39f2 Cr-Commit-Position: refs/heads/master@{#10869}
Message was sent while issue was closed.
Description was changed from ========== Add header extension filtering for WebRtcVoiceEngine/MediaChannel. Rework filtering functionality to be reused for both Audio+Video. BUG=webrtc:4690 Committed: https://crrev.com/7e4e01a4413fa98644b94ab9d8a9dccc664f39f2 Cr-Commit-Position: refs/heads/master@{#10869} ========== to ========== Add header extension filtering for WebRtcVoiceEngine/MediaChannel. Rework filtering functionality to be reused for both Audio+Video. BUG=webrtc:4690 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add header extension filtering for WebRtcVoiceEngine/MediaChannel. Rework filtering functionality to be reused for both Audio+Video. BUG=webrtc:4690 ========== to ========== Add header extension filtering for WebRtcVoiceEngine/MediaChannel. Rework filtering functionality to be reused for both Audio+Video. BUG=webrtc:4690, webrtc:4254 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
