Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2352063002:
Fix modules_unittests on iOS. (Closed)
|
Created:
4 years, 3 months ago by kthelgason Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, Andrew MacDonald, zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, mflodman, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1, magjed_webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix modules_unittests on iOS.
Some of the module tests were failing on iOS, causing them to be ignored
on the trybots. Specifically this CL:
- Skips some tests that should not run on the simulator
- Fixes iOS file system test helpers
- Fixes failing nalu parser unittest
BUG=webrtc:4752
R=henrika@webrtc.org, peah@webrtc.org, sprang@webrtc.org
TBR=tkchin@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/470c0887b334f1b859f2fd0055ab840370f5cc7e
Patch Set 1 #
Total comments: 9
Patch Set 2 : fix sizes #
Total comments: 2
Patch Set 3 : fix the fixed sizes #
Messages
Total messages: 30 (11 generated)
kthelgason@webrtc.org changed reviewers: + henrika@webrtc.org, tkchin@webrtc.org
Description was changed from ========== Fix modules_unittests on iOS. Some of the module tests were failing on iOS, causing them to be ignored on the trybots. Specifically this CL: - Skips some tests that should not run on the simulator - Fixes iOS file system test helpers - Fixes failing nalu parser unittest BUG=webrtc:4752 ========== to ========== Fix modules_unittests on iOS. Some of the module tests were failing on iOS, causing them to be ignored on the trybots. Specifically this CL: - Skips some tests that should not run on the simulator - Fixes iOS file system test helpers - Fixes failing nalu parser unittest BUG=webrtc:4752 ==========
Thanks for fixing. LGTM but I am not an owner in modules/audio_processing or modules/video_coding. Just in case, add owners who work on those folders as well.
kthelgason@webrtc.org changed reviewers: + peah@webrtc.org, sprang@webrtc.org
Add some more reviewers for good measure
lgtm https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:270: NaluType second_nalu_type = ParseNaluType(annexb_buffer[9]); nit: Use named constants for offsets.
On 2016/09/21 08:05:33, språng wrote: > lgtm > > https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... > File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): > > https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... > webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:270: NaluType > second_nalu_type = ParseNaluType(annexb_buffer[9]); > nit: Use named constants for offsets. Awesome! lgtm
https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_unittest.cc:407: // On iOS test data is flat in the project root dir would it be better to configure the build so that it is not flat in the project root dir to avoid needing this? https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:270: NaluType second_nalu_type = ParseNaluType(annexb_buffer[9]); On 2016/09/21 08:05:33, språng wrote: > nit: Use named constants for offsets. I don't follow this one 0-3: start code 4-5: access unit delimiter 6-9: start code Shouldn't 10 be correct? https://codereview.webrtc.org/2352063002/diff/1/webrtc/test/testsupport/iosfi... File webrtc/test/testsupport/iosfileutils.mm (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/test/testsupport/iosfi... webrtc/test/testsupport/iosfileutils.mm:28: NSString* NSStringFromStdString(const std::string& stdString) { fyi these should be eventually deleted and usage should shift to utility classes in webrtc/sdk/objc
https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_unittest.cc:407: // On iOS test data is flat in the project root dir On 2016/09/27 11:01:47, tkchin_webrtc wrote: > would it be better to configure the build so that it is not flat in the project > root dir to avoid needing this? Probably. I assume there is a reason things are this way though? https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:270: NaluType second_nalu_type = ParseNaluType(annexb_buffer[9]); On 2016/09/27 11:01:47, tkchin_webrtc wrote: > On 2016/09/21 08:05:33, språng wrote: > > nit: Use named constants for offsets. > > I don't follow this one > 0-3: start code > 4-5: access unit delimiter > 6-9: start code > > Shouldn't 10 be correct? At least the way the test is set up it's: 0-3: start code 4: nalu type 5-8: start code Perhaps it's the test that's wrong, not the implementation. https://codereview.webrtc.org/2352063002/diff/1/webrtc/test/testsupport/iosfi... File webrtc/test/testsupport/iosfileutils.mm (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/test/testsupport/iosfi... webrtc/test/testsupport/iosfileutils.mm:28: NSString* NSStringFromStdString(const std::string& stdString) { On 2016/09/27 11:01:47, tkchin_webrtc wrote: > fyi these should be eventually deleted and usage should shift to utility classes > in webrtc/sdk/objc Yes, I agree. I think there is a bug open for deleting this. I'll open one if not.
https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/audio_processi... File webrtc/modules/audio_processing/audio_processing_unittest.cc (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/audio_processi... webrtc/modules/audio_processing/audio_processing_unittest.cc:407: // On iOS test data is flat in the project root dir On 2016/09/27 11:10:31, kthelgason wrote: > On 2016/09/27 11:01:47, tkchin_webrtc wrote: > > would it be better to configure the build so that it is not flat in the > project > > root dir to avoid needing this? > > Probably. I assume there is a reason things are this way though? Not that I know of. Probably was just easier when the test files were first added. Not important for this CL, feel free to file a cleanup bug. https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2352063002/diff/1/webrtc/modules/video_coding/c... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:270: NaluType second_nalu_type = ParseNaluType(annexb_buffer[9]); On 2016/09/27 11:10:31, kthelgason wrote: > On 2016/09/27 11:01:47, tkchin_webrtc wrote: > > On 2016/09/21 08:05:33, språng wrote: > > > nit: Use named constants for offsets. > > > > I don't follow this one > > 0-3: start code > > 4-5: access unit delimiter > > 6-9: start code > > > > Shouldn't 10 be correct? > > At least the way the test is set up it's: > 0-3: start code > 4: nalu type > 5-8: start code > > Perhaps it's the test that's wrong, not the implementation. Can you double check the h264 spec to see how long auds are? Not sure which is incorrect here.
On 2016/09/27 13:51:56, tkchin_webrtc wrote: > On 2016/09/27 11:10:31, kthelgason wrote: > > Probably. I assume there is a reason things are this way though? > > Not that I know of. Probably was just easier when the test files were first > added. Not important for this CL, feel free to file a cleanup bug. Ack. > On 2016/09/27 11:10:31, kthelgason wrote: > > Perhaps it's the test that's wrong, not the implementation. > > Can you double check the h264 spec to see how long auds are? Not sure which is > incorrect here. If I'm parsing the spec right (a big if) an aud is just 3 bits and trailing bits to byte-alignment.
Patchset #2 (id:20001) has been deleted
Updated. PTAL
Updated. PTAL
https://codereview.webrtc.org/2352063002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2352063002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:267: // Start code + access unit delimiter + start code = 4 + 1 + 4 = 9. I looked and I think that 10 is correct start code + nalu type byte + aud byte + start code = 4 + 1 + 1 + 4 = 10
https://codereview.webrtc.org/2352063002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc (right): https://codereview.webrtc.org/2352063002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_nalu.cc:267: // Start code + access unit delimiter + start code = 4 + 1 + 4 = 9. On 2016/09/28 10:35:02, tkchin_webrtc wrote: > I looked and I think that 10 is correct > start code + nalu type byte + aud byte + start code = 4 + 1 + 1 + 4 = 10 You're right, I forgot to count the type byte. Adding 1-digit numbers can be hard! The actual aud byte is missing from the test. I'll change it.
Patchset #3 (id:60001) has been deleted
Can I get one last final lgtm?
Description was changed from ========== Fix modules_unittests on iOS. Some of the module tests were failing on iOS, causing them to be ignored on the trybots. Specifically this CL: - Skips some tests that should not run on the simulator - Fixes iOS file system test helpers - Fixes failing nalu parser unittest BUG=webrtc:4752 ========== to ========== Fix modules_unittests on iOS. Some of the module tests were failing on iOS, causing them to be ignored on the trybots. Specifically this CL: - Skips some tests that should not run on the simulator - Fixes iOS file system test helpers - Fixes failing nalu parser unittest BUG=webrtc:4752 TBR=tkchin@webrtc.org ==========
The CQ bit was checked by kthelgason@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org, peah@webrtc.org, henrika@webrtc.org Link to the patchset: https://codereview.webrtc.org/2352063002/#ps80001 (title: "fix the fixed sizes")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Fix modules_unittests on iOS. Some of the module tests were failing on iOS, causing them to be ignored on the trybots. Specifically this CL: - Skips some tests that should not run on the simulator - Fixes iOS file system test helpers - Fixes failing nalu parser unittest BUG=webrtc:4752 TBR=tkchin@webrtc.org ========== to ========== Fix modules_unittests on iOS. Some of the module tests were failing on iOS, causing them to be ignored on the trybots. Specifically this CL: - Skips some tests that should not run on the simulator - Fixes iOS file system test helpers - Fixes failing nalu parser unittest BUG=webrtc:4752 R=henrika@webrtc.org, peah@webrtc.org, sprang@webrtc.org TBR=tkchin@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/470c0887b334f1b859f2fd005... ==========
Message was sent while issue was closed.
Description was changed from ========== Fix modules_unittests on iOS. Some of the module tests were failing on iOS, causing them to be ignored on the trybots. Specifically this CL: - Skips some tests that should not run on the simulator - Fixes iOS file system test helpers - Fixes failing nalu parser unittest BUG=webrtc:4752 R=henrika@webrtc.org, peah@webrtc.org, sprang@webrtc.org TBR=tkchin@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/470c0887b334f1b859f2fd005... ========== to ========== Fix modules_unittests on iOS. Some of the module tests were failing on iOS, causing them to be ignored on the trybots. Specifically this CL: - Skips some tests that should not run on the simulator - Fixes iOS file system test helpers - Fixes failing nalu parser unittest BUG=webrtc:4752 R=henrika@webrtc.org, peah@webrtc.org, sprang@webrtc.org TBR=tkchin@webrtc.org Committed: https://crrev.com/470c0887b334f1b859f2fd0055ab840370f5cc7e Cr-Commit-Position: refs/heads/master@{#14472} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) manually as 470c0887b334f1b859f2fd0055ab840370f5cc7e (presubmit successful).
Message was sent while issue was closed.
On 2016/10/03 11:13:40, kthelgason wrote: > Committed patchset #3 (id:80001) manually as > 470c0887b334f1b859f2fd0055ab840370f5cc7e (presubmit successful). So did this fix all the failures? Then we should be able to add it into https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/ios/t... right? Just do that in a follow-up CL (only need to run the iOS simulator trybots for that one).
Message was sent while issue was closed.
On 2016/10/03 11:23:13, kjellander_webrtc wrote: > So did this fix all the failures? Then we should be able to add it into > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/ios/t... > right? Yes, at least in theory! > Just do that in a follow-up CL (only need to run the iOS simulator > trybots for that one). I will send you a CL to review soon. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
