Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2520933002:
iOS AppRTCMobile: Fix SDP video codec reordering for multiple H264 profiles (Closed)
|
Created:
4 years, 1 month ago by magjed_webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptioniOS AppRTCMobile: Fix SDP video codec reordering for multiple H264 profiles
BUG=webrtc:6738
Committed: https://crrev.com/e60f020456982c8f56dc1567e3b4cd49d356c5aa
Cr-Commit-Position: refs/heads/master@{#15198}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Make code cleaner and add more tests #
Total comments: 2
Patch Set 3 : Rename h264Desc #
Messages
Total messages: 20 (11 generated)
Description was changed from ========== iOS AppRTCMobile: Fix SDP video codec reordering BUG=webrtc:6738 ========== to ========== iOS AppRTCMobile: Fix SDP video codec reordering BUG=webrtc:6738 ==========
magjed@webrtc.org changed reviewers: + denicija@webrtc.org
Description was changed from ========== iOS AppRTCMobile: Fix SDP video codec reordering BUG=webrtc:6738 ========== to ========== iOS AppRTCMobile: Fix SDP video codec reordering for multiple H264 profiles BUG=webrtc:6738 ==========
Daniela - please take a look.
https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/ARDSDPUtils.m (right): https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ARDSDPUtils.m:37: NSRegularExpression *regex = I know this is not part of this CL, but perhaps in future we can consider using NSScanner for this portion? I think it would increase the readability without decreasing (if not increasing) performance. https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ARDSDPUtils.m:56: if (mLineIndex == -1) { can we extract the check for the "m=video" even before the loop? For instance, perhaps some nicer form of the following nested if* if (![sdpString hasPrefix:@"m=video"]) { if (![sdpString containsString:@"\nm=video"]) { //handle the use case when no m=video line has been found } } *this if nest is purely to demonstrate the logic I'm suggesting :) https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ARDSDPUtils.m:74: [newMLineParts addObjectsFromArray:codecPayloadTypes]; Perhaps we can extend the usage of methods of NSMutableArray further and perhaps get rid of the origPartIndex and the second for loop. Something like *: if (origMLineParts.count > 3) { NSMutableArray *newMLineParts = [NSMutableArray arrayWithArray:origMLineParts]; [newMLineParts removeObjectsInArray:codecPayloadTypes]; [newMLineParts insertObjects:codecPayloadTypes atIndexes:[...]]; } I don't know the usage of insertObjects:atIndexes: by heart, but this link should provide enough info on the usage https://developer.apple.com/reference/foundation/nsmutablearray/1416482-inser... * this is not working code, purely to demonstrate the logic :) https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:361: Perhaps few more tests can be added. For instance one for the use case when the m=video string is missing, and few to actually test the order of the preferred codecs.
Patchset #2 (id:20001) has been deleted
https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/ARDSDPUtils.m (right): https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ARDSDPUtils.m:37: NSRegularExpression *regex = On 2016/11/21 16:23:00, denicija-webrtc wrote: > I know this is not part of this CL, but perhaps in future we can consider using > NSScanner for this portion? > I think it would increase the readability without decreasing (if not increasing) > performance. Yeah maybe. I'm not familiar with NSScanner so I'm not sure how the code would end up looking. I guess it's similar to Scanner in Java, and in that case I actually prefer regex parsing. https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ARDSDPUtils.m:56: if (mLineIndex == -1) { On 2016/11/21 16:23:00, denicija-webrtc wrote: > can we extract the check for the "m=video" even before the loop? > For instance, perhaps some nicer form of the following nested if* > if (![sdpString hasPrefix:@"m=video"]) { > if (![sdpString containsString:@"\nm=video"]) { > //handle the use case when no m=video line has been found > } > } > > *this if nest is purely to demonstrate the logic I'm suggesting :) Done, I extracted finding the "m=video" line out from this for-loop. https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/ARDSDPUtils.m:74: [newMLineParts addObjectsFromArray:codecPayloadTypes]; On 2016/11/21 16:23:00, denicija-webrtc wrote: > Perhaps we can extend the usage of methods of NSMutableArray further and perhaps > get rid of the origPartIndex and the second for loop. > Something like *: > if (origMLineParts.count > 3) { > NSMutableArray *newMLineParts = > [NSMutableArray arrayWithArray:origMLineParts]; > [newMLineParts removeObjectsInArray:codecPayloadTypes]; > [newMLineParts insertObjects:codecPayloadTypes atIndexes:[...]]; > } > I don't know the usage of insertObjects:atIndexes: by heart, but this link > should provide enough info on the usage > https://developer.apple.com/reference/foundation/nsmutablearray/1416482-inser... > > * this is not working code, purely to demonstrate the logic :) Done, I tried my best to make it cleaner. https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2520933002/diff/1/webrtc/examples/objc/AppRTCMo... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:361: On 2016/11/21 16:23:00, denicija-webrtc wrote: > Perhaps few more tests can be added. > For instance one for the use case when the m=video string is missing, and few to > actually test the order of the preferred codecs. Done, I added some more tests.
denicija@google.com changed reviewers: + denicija@google.com
LGTM with one tiny remark that can be addressed if you see fit. https://codereview.webrtc.org/2520933002/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2520933002/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:369: RTCSessionDescription *h264Desc = Now that the test is more generic, perhaps this should be renamed from h264Desc?
https://codereview.webrtc.org/2520933002/diff/40001/webrtc/examples/objc/AppR... File webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm (right): https://codereview.webrtc.org/2520933002/diff/40001/webrtc/examples/objc/AppR... webrtc/examples/objc/AppRTCMobile/tests/ARDAppClientTest.mm:369: RTCSessionDescription *h264Desc = On 2016/11/22 15:05:30, denicija-google wrote: > Now that the test is more generic, perhaps this should be renamed from h264Desc? Thanks, missed that.
magjed@webrtc.org changed reviewers: - denicija@google.com
Btw, you accidentally stamped this CL with your corp account. I have separate profiles in Chrome to help me avoid doing that.
The CQ bit was checked by magjed@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from denicija@google.com Link to the patchset: https://codereview.webrtc.org/2520933002/#ps60001 (title: "Rename h264Desc")
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": 60001, "attempt_start_ts": 1479827741509270,
"parent_rev": "ec31458b089269119ad81d795bdb19dac70dbe10", "commit_rev":
"570061b7be68eb967198d3d09227f15293558432"}
Message was sent while issue was closed.
Description was changed from ========== iOS AppRTCMobile: Fix SDP video codec reordering for multiple H264 profiles BUG=webrtc:6738 ========== to ========== iOS AppRTCMobile: Fix SDP video codec reordering for multiple H264 profiles BUG=webrtc:6738 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== iOS AppRTCMobile: Fix SDP video codec reordering for multiple H264 profiles BUG=webrtc:6738 ========== to ========== iOS AppRTCMobile: Fix SDP video codec reordering for multiple H264 profiles BUG=webrtc:6738 Committed: https://crrev.com/e60f020456982c8f56dc1567e3b4cd49d356c5aa Cr-Commit-Position: refs/heads/master@{#15198} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e60f020456982c8f56dc1567e3b4cd49d356c5aa Cr-Commit-Position: refs/heads/master@{#15198} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
