Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2405463002:
Testing of FileVideoCapturer (Closed)
DescriptionTesting of FileVideoCapturer.
Based on https://codereview.webrtc.org/2273573003/
BUG=webrtc:6545
Committed: https://crrev.com/9890a5861f6ecf9ba16f95225b88c2aeaf5da2a1
Cr-Commit-Position: refs/heads/master@{#14801}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Using Object.wait() and .notify() to wait for frames for FileVideoCapturerTest #
Total comments: 3
Patch Set 3 : Copied over reference_video_640x360_30fps.y4m.sha1 from chromium and use it instead of smaller vide… #Patch Set 4 : Cleaned up and changed which pixels are compared in FileVideoCaputurerTest #Patch Set 5 : Cleaned up and changed which pixels are compared in FileVideoCaputurerTest #
Total comments: 8
Patch Set 6 : Changed to truncated video file for FileVideoCapturerTest #Patch Set 7 : Merging to be up to date with master #
Total comments: 6
Patch Set 8 : Fixed review comments #
Total comments: 4
Patch Set 9 : y #
Total comments: 12
Patch Set 10 : Hashes frames instead of compare random positions for FileVideoCapturerTest #Patch Set 11 : Fixed bug in nativeI420ToNV21 and tests with small frame for FileVideoCapturerTest #Patch Set 12 : Merge branch 'master' into with_yuv_file_reader #
Total comments: 1
Patch Set 13 : Expected value is converted with I420 to NV21 for VideoFileRendererTest #Patch Set 14 : Workaround for too long line with string literal #Patch Set 15 : Merge branch 'master' into with_yuv_file_reader #
Messages
Total messages: 68 (34 generated)
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
Only reviewed FileVideoCapturerTest.java since I assume the rest is identical to https://codereview.webrtc.org/2273573003 and will be gone when that's submitted and this is rebased? https://codereview.webrtc.org/2405463002/diff/1/webrtc/examples/androidtests/... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/1/webrtc/examples/androidtests/... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:32: public class MockCapturerObserver implements VideoCapturer.CapturerObserver { nit: add blank line https://codereview.webrtc.org/2405463002/diff/1/webrtc/examples/androidtests/... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:33: //private ArrayList<byte[]> frameDatas = new ArrayList<byte[]>(); Remove commented code. https://codereview.webrtc.org/2405463002/diff/1/webrtc/examples/androidtests/... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:75: @SmallTest nit: add blank line https://codereview.webrtc.org/2405463002/diff/1/webrtc/examples/androidtests/... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:77: FileVideoCapturer fileVideoCapturer = new FileVideoCapturer("/storage/emulated/0/reference_video_640x360_30fps_4frames.y4m"); I think this should be either /sdcard/chromium_tests_root/reference_video_640x360_30fps_4frames.y4m or /storage/emulated/0/reference_video_640x360_30fps_4frames.y4m assuming you'll put the .sha1 file in the resources/ directory of the root of WebRTC's repo. See https://cs.chromium.org/chromium/src/third_party/webrtc/test/testsupport/file... for the C++ counterpart (using the same Android test framework to push files to devices) https://codereview.webrtc.org/2405463002/diff/1/webrtc/examples/androidtests/... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:81: Thread.sleep(1000); Is there another way to implement this instead of sleeping? What guarantee do you have that things have completed after 1 second? https://codereview.webrtc.org/2405463002/diff/1/webrtc/examples/androidtests/... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:94: final int FRAME_WIDTH = 640; move these constants up to line 77 so it's easier to track where they come from.
Using Object.wait() and .notify() to wait for frames for FileVideoCapturerTest, instead of Thread.wait(1000). How to put test video to device is not solved yet.
looks good. Just need to deal with the resource file now then. I explained how in the comment below. https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn#... webrtc/examples/BUILD.gn:100: 1) You'll need to upload your reference_video_640x360_30fps_4frames.y4m to Google storage as I told you how to do. 2) Add the .sha1 file into resources/ dir of WebRTC and include it in this CL 3) Add a data entry here for your resource file, e.g: data = [ '//resources/reference_video_640x360_30fps_4frames.y4m' ] Then it will be transferred to the device during test. https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:69: public synchronized ArrayList<Frame> getMinimunFramesBlocking( getMinimunFramesBlocking -> getMinimumFramesBlocking https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:97: catch (InterruptedException e) { move to line above, e.g. } catch (InterruptedException e) {
On 2016/10/12 01:00:23, kjellander_webrtc wrote: > looks good. Just need to deal with the resource file now then. I explained how > in the comment below. > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn > File webrtc/examples/BUILD.gn (right): > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn#... > webrtc/examples/BUILD.gn:100: > 1) You'll need to upload your reference_video_640x360_30fps_4frames.y4m to > Google storage as I told you how to do. > 2) Add the .sha1 file into resources/ dir of WebRTC and include it in this CL > 3) Add a data entry here for your resource file, e.g: > data = [ '//resources/reference_video_640x360_30fps_4frames.y4m' ] > > Then it will be transferred to the device during test. > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > File > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java > (right): > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:69: > public synchronized ArrayList<Frame> getMinimunFramesBlocking( > getMinimunFramesBlocking -> getMinimumFramesBlocking > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:97: > catch (InterruptedException e) { > move to line above, e.g. > } catch (InterruptedException e) { Now I have changed to use the non-truncated version of the video file. The Java code is the same regardless of if the truncated or the non-truncated version. It depends on if it is desirable to not copy over big files (83MB). Did not manage to get down src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m from src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m.sha1 when I run gclient sync in the webrtc directory. I copied over src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m.sha1 to resources/ instead.
Fixed review comments and changed video file used
Description was changed from ========== Testing of FileVideoCapturer with timer synchronization. Based on https://codereview.webrtc.org/2273573003/ ========== to ========== Testing of FileVideoCapturer with timer synchronization. Based on https://codereview.webrtc.org/2273573003/ ==========
Description was changed from ========== Testing of FileVideoCapturer with timer synchronization. Based on https://codereview.webrtc.org/2273573003/ ========== to ========== Testing of FileVideoCapturer. Based on https://codereview.webrtc.org/2273573003/ ==========
mandermo@webrtc.org changed reviewers: + sakal@webrtc.org
Unit testing of FileVideoCapturer. It is based on https://codereview.webrtc.org/2273573003, so look at FileVideoCapturerTest.java
On 2016/10/12 09:14:16, mandermo wrote: > On 2016/10/12 01:00:23, kjellander_webrtc wrote: > > looks good. Just need to deal with the resource file now then. I explained how > > in the comment below. > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn > > File webrtc/examples/BUILD.gn (right): > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn#... > > webrtc/examples/BUILD.gn:100: > > 1) You'll need to upload your reference_video_640x360_30fps_4frames.y4m to > > Google storage as I told you how to do. > > 2) Add the .sha1 file into resources/ dir of WebRTC and include it in this CL > > 3) Add a data entry here for your resource file, e.g: > > data = [ '//resources/reference_video_640x360_30fps_4frames.y4m' ] > > > > Then it will be transferred to the device during test. > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > File > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java > > (right): > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:69: > > public synchronized ArrayList<Frame> getMinimunFramesBlocking( > > getMinimunFramesBlocking -> getMinimumFramesBlocking > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:97: > > catch (InterruptedException e) { > > move to line above, e.g. > > } catch (InterruptedException e) { > > Now I have changed to use the non-truncated version of the video file. The Java > code is the same regardless of if the truncated or the non-truncated version. It > depends on if it is desirable to not copy over big files (83MB). Having a smaller file is certainly preferred since transferring 83MB takes time. If it doesn't add any value there's no point having a large file. > Did not manage to get down > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m > from > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m.sha1 > when I run gclient sync in the webrtc directory. You should never need to look in src/chromium - it's just there for shared (source code) dependencies we have. Who pointed you to that file? > I copied over > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m.sha1 > to resources/ instead. That's fine, we can use an existing file in google storage if preferred. But if a smaller (new) file speeds up the test execution, that would be preferred.
On 2016/10/12 09:14:16, mandermo wrote: > On 2016/10/12 01:00:23, kjellander_webrtc wrote: > > looks good. Just need to deal with the resource file now then. I explained how > > in the comment below. > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn > > File webrtc/examples/BUILD.gn (right): > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn#... > > webrtc/examples/BUILD.gn:100: > > 1) You'll need to upload your reference_video_640x360_30fps_4frames.y4m to > > Google storage as I told you how to do. > > 2) Add the .sha1 file into resources/ dir of WebRTC and include it in this CL > > 3) Add a data entry here for your resource file, e.g: > > data = [ '//resources/reference_video_640x360_30fps_4frames.y4m' ] > > > > Then it will be transferred to the device during test. > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > File > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java > > (right): > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:69: > > public synchronized ArrayList<Frame> getMinimunFramesBlocking( > > getMinimunFramesBlocking -> getMinimumFramesBlocking > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:97: > > catch (InterruptedException e) { > > move to line above, e.g. > > } catch (InterruptedException e) { > > Now I have changed to use the non-truncated version of the video file. The Java > code is the same regardless of if the truncated or the non-truncated version. It > depends on if it is desirable to not copy over big files (83MB). Having a smaller file is certainly preferred since transferring 83MB takes time. If it doesn't add any value there's no point having a large file. > Did not manage to get down > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m > from > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m.sha1 > when I run gclient sync in the webrtc directory. You should never need to look in src/chromium - it's just there for shared (source code) dependencies we have. Who pointed you to that file? > I copied over > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m.sha1 > to resources/ instead. That's fine, we can use an existing file in google storage if preferred. But if a smaller (new) file speeds up the test execution, that would be preferred.
https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/BUILD.gn#... webrtc/examples/BUILD.gn:109: "//resources/reference_video_640x360_30fps.y4m", Can you upload a smaller version if you're only comparing a few bytes (it looks like it in the test). https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:88: "7C543C7D7C6C86B31055787FA03F5CE7B0507D5AA42A7B5E867E4462977F99707F958DADA67F7D7B8F6B5535939BA98F2C307D73757C7A4A7A44897DEA84A37A7C647F7F7C6677B37AB71E9C7C5775497D986C49C2398D80827EDE707D7F876988782D72", Are these correct even with the existing file? If you said it is 83MB I would expect each frame to be a lot larger than these Base64 encoded byte strings?
On 2016/10/12 21:05:42, kjellander_webrtc wrote: > On 2016/10/12 09:14:16, mandermo wrote: > > On 2016/10/12 01:00:23, kjellander_webrtc wrote: > > > looks good. Just need to deal with the resource file now then. I explained > how > > > in the comment below. > > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn > > > File webrtc/examples/BUILD.gn (right): > > > > > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/BUILD.gn#... > > > webrtc/examples/BUILD.gn:100: > > > 1) You'll need to upload your reference_video_640x360_30fps_4frames.y4m to > > > Google storage as I told you how to do. > > > 2) Add the .sha1 file into resources/ dir of WebRTC and include it in this > CL > > > 3) Add a data entry here for your resource file, e.g: > > > data = [ '//resources/reference_video_640x360_30fps_4frames.y4m' ] > > > > > > Then it will be transferred to the device during test. > > > > > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > > File > > > > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java > > > (right): > > > > > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > > > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:69: > > > public synchronized ArrayList<Frame> getMinimunFramesBlocking( > > > getMinimunFramesBlocking -> getMinimumFramesBlocking > > > > > > > > > https://codereview.webrtc.org/2405463002/diff/10001/webrtc/examples/androidte... > > > > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:97: > > > catch (InterruptedException e) { > > > move to line above, e.g. > > > } catch (InterruptedException e) { > > > > Now I have changed to use the non-truncated version of the video file. The > Java > > code is the same regardless of if the truncated or the non-truncated version. > It > > depends on if it is desirable to not copy over big files (83MB). > > Having a smaller file is certainly preferred since transferring 83MB takes time. > If it doesn't add any value there's no point having a large file. > > > Did not manage to get down > > > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m > > from > > > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m.sha1 > > when I run gclient sync in the webrtc directory. > > You should never need to look in src/chromium - it's just there for shared > (source code) dependencies we have. Who pointed you to that file? I used reference_video_640x360_30fps.y4m for testing and run command locate and found same file there and thought I could depend on it. Now new file is uploaded. > > > I copied over > > > src/chromium/src/chrome/test/data/webrtc/resources/reference_video_640x360_30fps.y4m.sha1 > > to resources/ instead. > > That's fine, we can use an existing file in google storage if preferred. But if > a smaller (new) file speeds up the test execution, that would be preferred.
Uses same video file but with only the first four frames. https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/BUILD.gn#... webrtc/examples/BUILD.gn:109: "//resources/reference_video_640x360_30fps.y4m", On 2016/10/12 21:07:50, kjellander_webrtc wrote: > Can you upload a smaller version if you're only comparing a few bytes (it looks > like it in the test). I have uploaded a shorter version of the video with only four frames. https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:88: "7C543C7D7C6C86B31055787FA03F5CE7B0507D5AA42A7B5E867E4462977F99707F958DADA67F7D7B8F6B5535939BA98F2C307D73757C7A4A7A44897DEA84A37A7C647F7F7C6677B37AB71E9C7C5775497D986C49C2398D80827EDE707D7F876988782D72", On 2016/10/12 21:07:50, kjellander_webrtc wrote: > Are these correct even with the existing file? If you said it is 83MB I would > expect each frame to be a lot larger than these Base64 encoded byte strings? I use a new pseudo random generator for each individual frame. I use those to pick random positions in the frames. It is possible to remove characters in the end of the strings in expectedFrames with no other change to the code.
lgtm with comment addressed, but obviously this has to land after the main CL. https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/BUILD.gn#... webrtc/examples/BUILD.gn:109: "//resources/reference_video_640x360_30fps.y4m", On 2016/10/13 11:32:48, mandermo wrote: > On 2016/10/12 21:07:50, kjellander_webrtc wrote: > > Can you upload a smaller version if you're only comparing a few bytes (it > looks > > like it in the test). > > I have uploaded a shorter version of the video with only four frames. Great, thanks! https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:88: "7C543C7D7C6C86B31055787FA03F5CE7B0507D5AA42A7B5E867E4462977F99707F958DADA67F7D7B8F6B5535939BA98F2C307D73757C7A4A7A44897DEA84A37A7C647F7F7C6677B37AB71E9C7C5775497D986C49C2398D80827EDE707D7F876988782D72", On 2016/10/13 11:32:48, mandermo wrote: > On 2016/10/12 21:07:50, kjellander_webrtc wrote: > > Are these correct even with the existing file? If you said it is 83MB I would > > expect each frame to be a lot larger than these Base64 encoded byte strings? > > I use a new pseudo random generator for each individual frame. I use those to > pick random positions in the frames. It is possible to remove characters in the > end of the strings in expectedFrames with no other change to the code. Thanks for explaining. This information is suitable for putting in a comment next to this variable, IMO.
Please upload a version that doesn't include the changes introduced in the main CL.
As a tip for the future, you can base your CL on another CL that hasn't landed yet. Just track a local branch that contains the CL you want to depend on. You can depend on a local branch using: git branch -u local_branch
Merging to be up to date with master and not to be based on https://codereview.webrtc.org/2426003002/
https://codereview.webrtc.org/2405463002/diff/100001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/100001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:26: public class FileVideoCapturerTest extends InstrumentationTestCase { Have you looked into if it possible to make this a Robolectric test instead? It would be preferable if possible. https://codereview.webrtc.org/2405463002/diff/100001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:28: byte[] data; nit: make these public https://codereview.webrtc.org/2405463002/diff/100001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:64: public synchronized ArrayList<Frame> getFrames() { This is never actually used, please remove.
Fixed review comments and made comparison string shorter https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:88: "7C543C7D7C6C86B31055787FA03F5CE7B0507D5AA42A7B5E867E4462977F99707F958DADA67F7D7B8F6B5535939BA98F2C307D73757C7A4A7A44897DEA84A37A7C647F7F7C6677B37AB71E9C7C5775497D986C49C2398D80827EDE707D7F876988782D72", On 2016/10/13 14:27:49, kjellander_webrtc wrote: > On 2016/10/13 11:32:48, mandermo wrote: > > On 2016/10/12 21:07:50, kjellander_webrtc wrote: > > > Are these correct even with the existing file? If you said it is 83MB I > would > > > expect each frame to be a lot larger than these Base64 encoded byte strings? > > > > I use a new pseudo random generator for each individual frame. I use those to > > pick random positions in the frames. It is possible to remove characters in > the > > end of the strings in expectedFrames with no other change to the code. > > Thanks for explaining. This information is suitable for putting in a comment > next to this variable, IMO. Added a comment, should I make it more elaborate? Also cut down on number of characters, because I felt it did not give much extra security. https://codereview.webrtc.org/2405463002/diff/100001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/100001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:26: public class FileVideoCapturerTest extends InstrumentationTestCase { On 2016/10/19 10:59:32, sakal wrote: > Have you looked into if it possible to make this a Robolectric test instead? It > would be preferable if possible. The problem is that native code is used by FileVideoCapturer to do conversion from I420 to NV21. Then Robolectric can not be used. https://codereview.webrtc.org/2405463002/diff/100001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:28: byte[] data; On 2016/10/19 10:59:32, sakal wrote: > nit: make these public Done. https://codereview.webrtc.org/2405463002/diff/100001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:64: public synchronized ArrayList<Frame> getFrames() { On 2016/10/19 10:59:32, sakal wrote: > This is never actually used, please remove. Done.
still lgtm https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:88: "7C543C7D7C6C86B31055787FA03F5CE7B0507D5AA42A7B5E867E4462977F99707F958DADA67F7D7B8F6B5535939BA98F2C307D73757C7A4A7A44897DEA84A37A7C647F7F7C6677B37AB71E9C7C5775497D986C49C2398D80827EDE707D7F876988782D72", On 2016/10/19 17:02:03, mandermo wrote: > On 2016/10/13 14:27:49, kjellander_webrtc wrote: > > On 2016/10/13 11:32:48, mandermo wrote: > > > On 2016/10/12 21:07:50, kjellander_webrtc wrote: > > > > Are these correct even with the existing file? If you said it is 83MB I > > would > > > > expect each frame to be a lot larger than these Base64 encoded byte > strings? > > > > > > I use a new pseudo random generator for each individual frame. I use those > to > > > pick random positions in the frames. It is possible to remove characters in > > the > > > end of the strings in expectedFrames with no other change to the code. > > > > Thanks for explaining. This information is suitable for putting in a comment > > next to this variable, IMO. > > Added a comment, should I make it more elaborate? > > Also cut down on number of characters, because I felt it did not give much extra > security. Looks good. Thanks
Please investigate why bots are failing. https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:96: assertTrue(expectedFrames.length <= frameDatas.size()); ==? https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:98: fileVideoCapturer.stopCapture(); also call dispose?
On 2016/10/20 07:54:16, sakal wrote: > Please investigate why bots are failing. It looks like you haven't uploaded the file properly. You'll need to fix that and update the sha1 to the correct one. > > https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... > File > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java > (right): > > https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:96: > assertTrue(expectedFrames.length <= frameDatas.size()); > ==? > > https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:98: > fileVideoCapturer.stopCapture(); > also call dispose?
Description was changed from ========== Testing of FileVideoCapturer. Based on https://codereview.webrtc.org/2273573003/ ========== to ========== Testing of FileVideoCapturer. Based on https://codereview.webrtc.org/2273573003/ BUG=webrtc:6545 ==========
On 2016/10/20 08:56:58, kjellander_webrtc wrote: > On 2016/10/20 07:54:16, sakal wrote: > > Please investigate why bots are failing. > > It looks like you haven't uploaded the file properly. You'll need to fix that > and update the sha1 to the correct one. I have now uploaded the file for you. The sha1 was identical so you probably just failed the authentication part. I triggered new tryjobs too. > > > > > https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... > > File > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java > > (right): > > > > > https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:96: > > assertTrue(expectedFrames.length <= frameDatas.size()); > > ==? > > > > > https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... > > > webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:98: > > fileVideoCapturer.stopCapture(); > > also call dispose?
mandermo@webrtc.org changed reviewers: + magjed@webrtc.org
Fixed comments. +magjed because he is owner of file. https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:96: assertTrue(expectedFrames.length <= frameDatas.size()); On 2016/10/20 07:54:16, sakal wrote: > ==? Done. https://codereview.webrtc.org/2405463002/diff/120001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:98: fileVideoCapturer.stopCapture(); On 2016/10/20 07:54:16, sakal wrote: > also call dispose? Done.
https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:34: private ArrayList<Frame> frameDatas = new ArrayList<Frame>(); Make this variable final. https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:36: public void onCapturerStarted(boolean success) { Annotate every function with @Override. https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:37: // Empty on purpose assertTrue on success https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:41: // Empty on purpose Add a dot at the end of every sentence in comments. https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:60: public void onOutputFormatRequest(int width, int height, int framerate) { This function is no longer part of the interface, so remove it. https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:101: for (int i = 0; i < expectedFrames.length; ++i) { Since it's impossible to verify this manually anyway and you don't compare against the input, just compute a hash with e.g. MessageDigest digest = MessageDigest.getInstance("SHA-256"); byte[] hashBytes = digest.digest(frameDatas[i].data); and compare the hash against a pre-computed expected hash. This will reduce the code complexity of the test and we won't miss incorrect pixels at e.g. corners or borders. BTW, why can't you use a really small file, e.g. 4x4 and do a full equality test?
Updated based on review comments. https://codereview.chromium.org/2405463002/diff/140001/webrtc/examples/androi... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.chromium.org/2405463002/diff/140001/webrtc/examples/androi... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:34: private ArrayList<Frame> frameDatas = new ArrayList<Frame>(); On 2016/10/20 14:55:33, magjed_webrtc wrote: > Make this variable final. Done. https://codereview.chromium.org/2405463002/diff/140001/webrtc/examples/androi... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:36: public void onCapturerStarted(boolean success) { On 2016/10/20 14:55:33, magjed_webrtc wrote: > Annotate every function with @Override. Done. https://codereview.chromium.org/2405463002/diff/140001/webrtc/examples/androi... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:37: // Empty on purpose On 2016/10/20 14:55:33, magjed_webrtc wrote: > assertTrue on success Done. https://codereview.chromium.org/2405463002/diff/140001/webrtc/examples/androi... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:41: // Empty on purpose On 2016/10/20 14:55:33, magjed_webrtc wrote: > Add a dot at the end of every sentence in comments. Done. https://codereview.chromium.org/2405463002/diff/140001/webrtc/examples/androi... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:101: for (int i = 0; i < expectedFrames.length; ++i) { On 2016/10/20 14:55:33, magjed_webrtc wrote: > Since it's impossible to verify this manually anyway and you don't compare > against the input, just compute a hash with e.g. > MessageDigest digest = MessageDigest.getInstance("SHA-256"); > byte[] hashBytes = digest.digest(frameDatas[i].data); > and compare the hash against a pre-computed expected hash. This will reduce the > code complexity of the test and we won't miss incorrect pixels at e.g. corners > or borders. > > BTW, why can't you use a really small file, e.g. 4x4 and do a full equality > test? Now use hashing approach. Using small file is also a solution. Should I change?
https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/140001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:101: for (int i = 0; i < expectedFrames.length; ++i) { On 2016/10/24 18:08:02, mandermo wrote: > On 2016/10/20 14:55:33, magjed_webrtc wrote: > > Since it's impossible to verify this manually anyway and you don't compare > > against the input, just compute a hash with e.g. > > MessageDigest digest = MessageDigest.getInstance("SHA-256"); > > byte[] hashBytes = digest.digest(frameDatas[i].data); > > and compare the hash against a pre-computed expected hash. This will reduce > the > > code complexity of the test and we won't miss incorrect pixels at e.g. corners > > or borders. > > > > BTW, why can't you use a really small file, e.g. 4x4 and do a full equality > > test? > > Now use hashing approach. Using small file is also a solution. Should I change? Yes, a small file would be optimal. Can you do the same as in VideoFileRendererTest and have: String inputFile = "YUV4MPEG2 C420 W4 H4 Ip F30:1 A1:1\n" + "FRAME\n" + "THIS IS JUST SOME TEXT xFRAME\n" + "THE SECOND FRAME qwerty.FRAME\n" + "HERE IS THE THRID FRAME!"; String[] expectedFrames = { "THIS IS JUST SOME TEXT x", "THE SECOND FRAME qwerty.", "HERE IS THE THRID FRAME!"}; ?
Fixed a bug with how I handle java byte arrays in JNI in nativeI420ToNV21, which is exercised by the test through FileVideoCapturer. Changed test to use a video with 4x4 frames. The exact output is not human constructed but got from running the test. To get the same value as input the data needs to be converted back from NV12 to I420 (non-lossy conversion), which is not done now.
The CQ bit was checked by mandermo@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/...
It's easier to do it the other way around instead, i.e. convert the expected I420 frames to expected NV21 frames using the existing FileVideoCapturer.nativeI420ToNV21. https://codereview.webrtc.org/2405463002/diff/200001/webrtc/examples/androidt... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java (right): https://codereview.webrtc.org/2405463002/diff/200001/webrtc/examples/androidt... webrtc/examples/androidtests/src/org/appspot/apprtc/test/FileVideoCapturerTest.java:89: assertTrue(expectedFrames.length == frameDatas.size()); nit: Use assertEquals instead
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9557)
Expected value is converted with I420 to NV21.
lgtm
The CQ bit was checked by mandermo@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: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9574)
The CQ bit was checked by mandermo@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: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2494)
The CQ bit was checked by mandermo@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/...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2500)
The CQ bit was checked by mandermo@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: Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2509)
The CQ bit was checked by mandermo@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 mandermo@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org, magjed@webrtc.org, sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2405463002/#ps260001 (title: "Merge branch 'master' into with_yuv_file_reader")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Testing of FileVideoCapturer. Based on https://codereview.webrtc.org/2273573003/ BUG=webrtc:6545 ========== to ========== Testing of FileVideoCapturer. Based on https://codereview.webrtc.org/2273573003/ BUG=webrtc:6545 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Testing of FileVideoCapturer. Based on https://codereview.webrtc.org/2273573003/ BUG=webrtc:6545 ========== to ========== Testing of FileVideoCapturer. Based on https://codereview.webrtc.org/2273573003/ BUG=webrtc:6545 Committed: https://crrev.com/9890a5861f6ecf9ba16f95225b88c2aeaf5da2a1 Cr-Commit-Position: refs/heads/master@{#14801} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9890a5861f6ecf9ba16f95225b88c2aeaf5da2a1 Cr-Commit-Position: refs/heads/master@{#14801}
Message was sent while issue was closed.
Sorry I didn't catch it earlier but these tests should probably be moved under webrtc/api/androidtests since they don't test the AppRTCMobile itself. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
