Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1429753003:
Filter overlapping RTP header extensions. (Closed)
|
Created:
5 years, 1 month ago by stefan-webrtc Modified:
5 years, 1 month ago Reviewers:
pbos-webrtc CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFilter overlapping RTP header extensions.
This removes unnecessary RTP header extension overhead since only one of
these extensions is used at a time.
BUG=webrtc:4254
R=pbos@webrtc.org
Committed: https://crrev.com/bbaf3633c54e3d49aa4c762b8eaa34e09de01c45
Cr-Commit-Position: refs/heads/master@{#10455}
Patch Set 1 #Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #
Total comments: 2
Patch Set 4 : comments addressed. #
Total comments: 2
Patch Set 5 : fix mem issue #Patch Set 6 : size_t conversion fix #
Messages
Total messages: 36 (12 generated)
stefan@webrtc.org changed reviewers: + pbos@webrtc.org
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429753003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429753003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/10294)
.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429753003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429753003/20001
https://codereview.webrtc.org/1429753003/diff/20001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.cc (right): https://codereview.webrtc.org/1429753003/diff/20001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:79: CreateBweExtensionPriorities(); Static initializer, not permitted. Can you make this a POD struct {const char*, int} instead? Scanning through a map with 3 elements shouldn't be more effective either way. https://codereview.webrtc.org/1429753003/diff/20001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.h (right): https://codereview.webrtc.org/1429753003/diff/20001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.h:53: std::vector<RtpHeaderExtension> FilterOverlappingRtpExtensions( s/Overlapping/Redundant imo.
https://codereview.webrtc.org/1429753003/diff/20001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.cc (right): https://codereview.webrtc.org/1429753003/diff/20001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:79: CreateBweExtensionPriorities(); On 2015/10/29 14:16:22, pbos-webrtc wrote: > Static initializer, not permitted. Can you make this a POD struct {const char*, > int} instead? Scanning through a map with 3 elements shouldn't be more effective > either way. Done. https://codereview.webrtc.org/1429753003/diff/20001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.h (right): https://codereview.webrtc.org/1429753003/diff/20001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.h:53: std::vector<RtpHeaderExtension> FilterOverlappingRtpExtensions( On 2015/10/29 14:16:22, pbos-webrtc wrote: > s/Overlapping/Redundant imo. Done.
https://codereview.webrtc.org/1429753003/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1429753003/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:1222: } Can you add one that picks abssendtime over toffset? I think you can parameterize this test as: (std::vector<std::string> extensions, std::string expected_extension)
then lgtm!
On 2015/10/29 15:04:18, pbos-webrtc wrote: > then lgtm! Shouldn't this be added for webrtcvoiceengine.cc as well?
comments addressed.
https://codereview.webrtc.org/1429753003/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1429753003/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:1222: } On 2015/10/29 15:04:01, pbos-webrtc wrote: > Can you add one that picks abssendtime over toffset? I think you can > parameterize this test as: (std::vector<std::string> extensions, std::string > expected_extension) Done.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429753003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429753003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/10269)
yeeeep https://codereview.webrtc.org/1429753003/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.cc (right): https://codereview.webrtc.org/1429753003/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:75: const size_t kBweExtensionPrioritiesLength = sizeof(kBweExtensionPriorities); = ARRAY_SIZE(kBweExtensionPriorities), this is now sizeof(const char*) * 3
fix mem issue
https://codereview.webrtc.org/1429753003/diff/60001/talk/media/webrtc/webrtcm... File talk/media/webrtc/webrtcmediaengine.cc (right): https://codereview.webrtc.org/1429753003/diff/60001/talk/media/webrtc/webrtcm... talk/media/webrtc/webrtcmediaengine.cc:75: const size_t kBweExtensionPrioritiesLength = sizeof(kBweExtensionPriorities); On 2015/10/29 15:36:04, pbos-webrtc wrote: > = ARRAY_SIZE(kBweExtensionPriorities), this is now sizeof(const char*) * 3 Done.
The CQ bit was checked by stefan@webrtc.org
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/1429753003/#ps80001 (title: "fix mem issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429753003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429753003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/10306)
size_t conversion fix
The CQ bit was checked by stefan@webrtc.org
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/1429753003/#ps100001 (title: "size_t conversion fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1429753003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1429753003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as bbaf3633c54e3d49aa4c762b8eaa34e09de01c45 (presubmit successful).
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/bbaf3633c54e3d49aa4c762b8eaa34e09de01c45 Cr-Commit-Position: refs/heads/master@{#10455} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
