Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415563002:
Testing of VideoFileRenderer with byte frames (Closed)
DescriptionTesting of VideoFileRenderer with byte frames
BUG=webrtc:6545
Committed: https://crrev.com/a8bec8d8e74e95998d3a18d7b7fed41b4c570e37
Cr-Commit-Position: refs/heads/master@{#14781}
Patch Set 1 #Patch Set 2 : Cleanup in VideoFileRendererTest #Patch Set 3 : Changed path for output video file for VideoFileRendererTest #
Total comments: 8
Patch Set 4 : Merging to be up to date with master #
Total comments: 6
Patch Set 5 : Added test for video header for VideoFileRenderer and fixed review comments #
Total comments: 16
Patch Set 6 : Re-design of VideoFileRendererTest #
Messages
Total messages: 45 (23 generated)
mandermo@webrtc.org changed reviewers: + kjellander@webrtc.org
This the test of the writting of video frames. Only look at VideoFileRendererTest and the corresponding BUILD.gn file.
mandermo@webrtc.org changed reviewers: + sakal@webrtc.org
Unit testing of VideoFileRenderer. Based on https://codereview.webrtc.org/2273573003. I use /storage/emulated/0/ for temporary file now. What is better location?
On 2016/10/12 17:50:28, mandermo wrote: > Unit testing of VideoFileRenderer. Based on > https://codereview.webrtc.org/2273573003. > > I use /storage/emulated/0/ for temporary file now. What is better location? If you're just writing output, I guess it doesn't matter if you ensure to clean up the temporary file at the end of the test. Otherwise /sdcard/chromium_tests_root/ is the default dir used by the test framework, and it's cleaned before each run IIRC.
Putting the output file in /sdcard/chromium_tests_root/ instead.
https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java (right): https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:33: int inputWidth = 32; Why is input width different than output? It seems like testing scaling would be a separate test case. https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:36: Random random = new Random(frameIdx * 42); Where does 42 come from? https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:58: Random verificationRandom = new Random(23423); Please explain where this number comes from and/or define it as a final int to make the code more readable. https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:59: String expectedData = "A5CC23AF39B91850A58176866D597446A49A9BA2C29F65B87B"; Same here.
Please upload a version that doesn't include the changes introduced in the main CL.
Merging to be up to date with master.
On 2016/10/19 09:22:38, mandermo wrote: > Merging to be up to date with master. Great; much cleaner! Now please respond to my comments (in the UI, please don't use the messages text-field for replying since they won't show up when you view the code that way)
https://codereview.webrtc.org/2415563002/diff/60001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java (right): https://codereview.webrtc.org/2415563002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:23: public class VideoFileRendererTest extends InstrumentationTestCase { Try to make it a Robolectric test if possible. File reading and egl stuff might be problematic but please at least investigate. https://codereview.webrtc.org/2415563002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:46: buffer.put(planeBytes); nit: Seems there is some unnecessary double allocation here but since it is a test it doesn't really matter. https://codereview.webrtc.org/2415563002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:59: String expectedData = "A5CC23AF39B91850A58176866D597446A49A9BA2C29F65B87B"; I am assuming you ran this once and got this hash and now you are just verifying it stays the same? I would prefer at least some human readable testing as well. At least the headers should be easy to verify.
Added test for video header for VideoFileRenderer and fixed review comments https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java (right): https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:33: int inputWidth = 32; On 2016/10/13 14:25:12, kjellander_webrtc wrote: > Why is input width different than output? It seems like testing scaling would be > a separate test case. There is no separate code paths for scaling, so thought I could settle with one test case. https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:36: Random random = new Random(frameIdx * 42); On 2016/10/13 14:25:12, kjellander_webrtc wrote: > Where does 42 come from? Cleaned up and commented. https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:58: Random verificationRandom = new Random(23423); On 2016/10/13 14:25:12, kjellander_webrtc wrote: > Please explain where this number comes from and/or define it as a final int to > make the code more readable. Made it cleaner and commented. https://codereview.webrtc.org/2415563002/diff/40001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:59: String expectedData = "A5CC23AF39B91850A58176866D597446A49A9BA2C29F65B87B"; On 2016/10/13 14:25:12, kjellander_webrtc wrote: > Same here. Done. https://codereview.webrtc.org/2415563002/diff/60001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java (right): https://codereview.webrtc.org/2415563002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:23: public class VideoFileRendererTest extends InstrumentationTestCase { On 2016/10/19 11:13:35, sakal wrote: > Try to make it a Robolectric test if possible. File reading and egl stuff might > be problematic but please at least investigate. The problem that native code is called in VideoFileRenderer for scaling. https://codereview.webrtc.org/2415563002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:46: buffer.put(planeBytes); On 2016/10/19 11:13:35, sakal wrote: > nit: Seems there is some unnecessary double allocation here but since it is a > test it doesn't really matter. As you said, it doesn't matter. https://codereview.webrtc.org/2415563002/diff/60001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:59: String expectedData = "A5CC23AF39B91850A58176866D597446A49A9BA2C29F65B87B"; On 2016/10/19 11:13:35, sakal wrote: > I am assuming you ran this once and got this hash and now you are just verifying > it stays the same? I would prefer at least some human readable testing as well. > At least the headers should be easy to verify. Added some testing for the header now.
lgtm but sakal knows this code much better...
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/...
Description was changed from ========== Testing of VideoFileRenderer with byte frames ========== to ========== Testing of VideoFileRenderer with byte frames BUG=webrtc:6545 ==========
The CQ bit was unchecked by mandermo@webrtc.org
The CQ bit was checked by mandermo@webrtc.org
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 mandermo@webrtc.org
The CQ bit was checked by mandermo@webrtc.org
The CQ bit was unchecked by mandermo@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
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.
mandermo@webrtc.org changed reviewers: + magjed@webrtc.org
+magjed because he is owner of the .java file.
https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java (right): https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:37: Random random = new Random(frameIdx); You only need to seed once. Please declare the Random outside the for-loop and use a static seed like 'final Random random = new Random(SEED);'. https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:42: int[] yuvStrides = {inputWidth, inputWidth / 2, inputWidth / 2}; Move this variable closer to where you use it. https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:44: byte[] planeBytes = new byte[planeSizes[i]]; You can write this whole for loop body simpler with fewer allocations like this: yuvPlanes[i] = ByteBuffer.allocateDirect(planeSizes[i]); Random.nextBytes(yuvPlanes[i].array()); https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:46: ByteBuffer buffer = ByteBuffer.allocateDirect(inputFrameSize); This should be planeSizes[i] instead of inputFrameSize, but it will go away if you fix my comment above. https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:62: final String expectedData = "275E6B837FB545456E49A397DD139B68B84C9760806D798063"; What's this? Why isn't the expected data based on the input yuvPlanes? Just loop over every pixel and compare it against the input. I think the scaling will cause trouble because of the bilinear filtering. Either use same input and output size and try to do perfect pixel matching, or make the input image a smooth gradient and compare +/- a small range when comparing pixels. https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:87: assertEquals("YUV4MPEG2 C420 W64 H64 Ip F30:1 A1:1", builder.toString()); It feels more natural to check the header before checking the pixel data. https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:88: } You need to clean up after the test and remove /sdcard/chromium_tests_root/testvideoout.y4m.
https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java (right): https://codereview.webrtc.org/2415563002/diff/80001/webrtc/examples/androidte... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:73: for (;;) { It looks like you have implemented RandomAccessFile.readLine(), can you use that function directly instead?
Redesign with pixel by pixel comparison. https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... File webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java (right): https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:37: Random random = new Random(frameIdx); On 2016/10/20 14:21:07, magjed_webrtc wrote: > You only need to seed once. Please declare the Random outside the for-loop and > use a static seed like 'final Random random = new Random(SEED);'. Done. https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:42: int[] yuvStrides = {inputWidth, inputWidth / 2, inputWidth / 2}; On 2016/10/20 14:21:07, magjed_webrtc wrote: > Move this variable closer to where you use it. Done. https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:44: byte[] planeBytes = new byte[planeSizes[i]]; On 2016/10/20 14:21:08, magjed_webrtc wrote: > You can write this whole for loop body simpler with fewer allocations like this: > yuvPlanes[i] = ByteBuffer.allocateDirect(planeSizes[i]); > Random.nextBytes(yuvPlanes[i].array()); Done. https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:46: ByteBuffer buffer = ByteBuffer.allocateDirect(inputFrameSize); On 2016/10/20 14:21:08, magjed_webrtc wrote: > This should be planeSizes[i] instead of inputFrameSize, but it will go away if > you fix my comment above. Done. https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:62: final String expectedData = "275E6B837FB545456E49A397DD139B68B84C9760806D798063"; On 2016/10/20 14:21:07, magjed_webrtc wrote: > What's this? Why isn't the expected data based on the input yuvPlanes? Just loop > over every pixel and compare it against the input. I think the scaling will > cause trouble because of the bilinear filtering. Either use same input and > output size and try to do perfect pixel matching, or make the input image a > smooth gradient and compare +/- a small range when comparing pixels. Now it is re-designed and compares pixel by pixel. https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:73: for (;;) { On 2016/10/20 14:41:03, magjed_webrtc wrote: > It looks like you have implemented RandomAccessFile.readLine(), can you use that > function directly instead? Done. https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:87: assertEquals("YUV4MPEG2 C420 W64 H64 Ip F30:1 A1:1", builder.toString()); On 2016/10/20 14:21:08, magjed_webrtc wrote: > It feels more natural to check the header before checking the pixel data. Done. https://codereview.chromium.org/2415563002/diff/80001/webrtc/examples/android... webrtc/examples/androidtests/src/org/appspot/apprtc/test/VideoFileRendererTest.java:88: } On 2016/10/20 14:21:08, magjed_webrtc wrote: > You need to clean up after the test and remove > /sdcard/chromium_tests_root/testvideoout.y4m. Done.
lgtm. It would be nice to test the texture path as well, but we can do that in a later CL.
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, sakal@webrtc.org Link to the patchset: https://codereview.webrtc.org/2415563002/#ps100001 (title: "Re-design of VideoFileRendererTest")
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 VideoFileRenderer with byte frames BUG=webrtc:6545 ========== to ========== Testing of VideoFileRenderer with byte frames BUG=webrtc:6545 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Testing of VideoFileRenderer with byte frames BUG=webrtc:6545 ========== to ========== Testing of VideoFileRenderer with byte frames BUG=webrtc:6545 Committed: https://crrev.com/a8bec8d8e74e95998d3a18d7b7fed41b4c570e37 Cr-Commit-Position: refs/heads/master@{#14781} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/a8bec8d8e74e95998d3a18d7b7fed41b4c570e37 Cr-Commit-Position: refs/heads/master@{#14781} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
