Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2024843002:
Refactor VideoCapturerAndroid tests in WebRTC. (Closed)
DescriptionRefactor VideoCapturerAndroid tests in WebRTC.
Camera1 tests are now separated from general CameraVideoCapturer tests.
Main motivation behind these changes is that Camera2 implementation can
be tested using the same tests.
CL also reduces code duplication on tests using textures.
BUG=webrtc:5519
Committed: https://crrev.com/79ede033f6253886502a01c2e0db0a1b28001d51
Cr-Commit-Position: refs/heads/master@{#13130}
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 17
Patch Set 3 : Fixes according to magjed's comments #Patch Set 4 : Make haveTwoCameras an abstract member function. #Patch Set 5 : Remove isCapturingToTexture from the interface. #Patch Set 6 : Remove dead code, change testCreateNonExistingCamera to expect IllegalArgumentException #Patch Set 7 : Suppress tests in the abstract class #
Total comments: 12
Patch Set 8 : Class annotation seems to be inherited by the subclasses, suppress individual tests instead #Patch Set 9 : Changes according to magjed's comments #2 #
Total comments: 7
Patch Set 10 : Similarity 20 % #Patch Set 11 : Return original order of tests to make the diff smaller #Patch Set 12 : Rename CameraVideoCapturerTest to CameraVideoCapturerTestBase #Patch Set 13 : Add missing return to testSwitchVideoCapturer #Patch Set 14 : Rebase #
Total comments: 3
Patch Set 15 : Refactor tests and use a fixture instead of a base class #Patch Set 16 : Increase amount of attempts in VideoCapturerAndroid to make Camera1CapturerUsingByteBufferTest#test… #
Total comments: 18
Patch Set 17 : Fixes according to perkj's comments #Patch Set 18 : Rename dispose #
Total comments: 8
Patch Set 19 : Fixes according to perkj's comments #2 #
Total comments: 6
Patch Set 20 : Fixes according to perkj's comments #3 #Patch Set 21 : Reorder imports to match Java style guide #Messages
Total messages: 38 (13 generated)
Description was changed from ========== Refactor VideoCapturerAndroid tests in WebRTC. Camera1 tests are now separated from general CameraVideoCapturer tests. Main motivation behind these changes is that Camera2 implementation can be tested using the same tests. CL also reduces code duplication on tests using textures. ========== to ========== Refactor VideoCapturerAndroid tests in WebRTC. Camera1 tests are now separated from general CameraVideoCapturer tests. Main motivation behind these changes is that Camera2 implementation can be tested using the same tests. CL also reduces code duplication on tests using textures. ==========
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
Magnus, please take a look.
https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:28: } catch (IllegalArgumentException e) { Do we really need to catch? We are not using any invalid device names? https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:40: VideoCapturerAndroid capturer = (VideoCapturerAndroid) No need to cast it to VideoCapturerAndroid? https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:50: if (!CameraVideoCapturerTestFixtures.HaveTwoCameras()) { Check if |deviceName| is null instead. And do the same for the front-facing test. https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:20: abstract protected CameraVideoCapturer createCapturer( If you add these methods: abstract protected String getNameOfFrontFacingDevice(); abstract protected String getNameOfBackFacingDevice(); and replace CameraEnumerationAndroid.getDeviceName(0) with "", can you make all tests common? https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:30: static public boolean HaveTwoCameras() { We shouldn't have Camera1 specific implementations in this file. How about making HaveTwoCameras() an abstract method in CameraVideoCapturerTest.java and only call the switchCamera() test if we have more than two cameras?
https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/VideoCapturer.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/VideoCapturer.java:109: boolean isCapturingToTexture(); I don't want to have this in the interface. Make it an abstract method in CameraVideoCapturerTest.java instead and pass it as a bool to the fixture.
https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:28: } catch (IllegalArgumentException e) { On 2016/05/31 13:04:51, magjed_webrtc wrote: > Do we really need to catch? We are not using any invalid device names? We do in testCreateNonExistingCamera. Should we remove this test? https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:40: VideoCapturerAndroid capturer = (VideoCapturerAndroid) On 2016/05/31 13:04:51, magjed_webrtc wrote: > No need to cast it to VideoCapturerAndroid? Done. https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:50: if (!CameraVideoCapturerTestFixtures.HaveTwoCameras()) { On 2016/05/31 13:04:51, magjed_webrtc wrote: > Check if |deviceName| is null instead. And do the same for the front-facing > test. Done. https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:20: abstract protected CameraVideoCapturer createCapturer( On 2016/05/31 13:04:51, magjed_webrtc wrote: > If you add these methods: > abstract protected String getNameOfFrontFacingDevice(); > abstract protected String getNameOfBackFacingDevice(); > and replace CameraEnumerationAndroid.getDeviceName(0) with "", can you make all > tests common? Not all but some. Done. https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:30: static public boolean HaveTwoCameras() { On 2016/05/31 13:04:51, magjed_webrtc wrote: > We shouldn't have Camera1 specific implementations in this file. How about > making HaveTwoCameras() an abstract method in CameraVideoCapturerTest.java and > only call the switchCamera() test if we have more than two cameras? Done. https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/java/src/org/w... File webrtc/api/java/src/org/webrtc/VideoCapturer.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/java/src/org/w... webrtc/api/java/src/org/webrtc/VideoCapturer.java:109: boolean isCapturingToTexture(); On 2016/05/31 13:08:47, magjed_webrtc wrote: > I don't want to have this in the interface. Make it an abstract method in > CameraVideoCapturerTest.java instead and pass it as a bool to the fixture. Done.
https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:28: } catch (IllegalArgumentException e) { On 2016/05/31 14:22:36, sakal wrote: > On 2016/05/31 13:04:51, magjed_webrtc wrote: > > Do we really need to catch? We are not using any invalid device names? > > We do in testCreateNonExistingCamera. Should we remove this test? Can you update the test to expect an IllegalArgumentException instead? https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:20: abstract protected CameraVideoCapturer createCapturer( On 2016/05/31 14:22:36, sakal wrote: > On 2016/05/31 13:04:51, magjed_webrtc wrote: > > If you add these methods: > > abstract protected String getNameOfFrontFacingDevice(); > > abstract protected String getNameOfBackFacingDevice(); > > and replace CameraEnumerationAndroid.getDeviceName(0) with "", can you make > all > > tests common? > > Not all but some. Done. What functions do you need to extract into abstract functions in order to make all tests common? I see Camera.open(), anything else? Can we use createCapturer() twice in order to achieve the same effect?
https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:28: } catch (IllegalArgumentException e) { On 2016/05/31 14:44:40, magjed_webrtc wrote: > On 2016/05/31 14:22:36, sakal wrote: > > On 2016/05/31 13:04:51, magjed_webrtc wrote: > > > Do we really need to catch? We are not using any invalid device names? > > > > We do in testCreateNonExistingCamera. Should we remove this test? > > Can you update the test to expect an IllegalArgumentException instead? Done. https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:20: abstract protected CameraVideoCapturer createCapturer( On 2016/05/31 14:44:40, magjed_webrtc wrote: > On 2016/05/31 14:22:36, sakal wrote: > > On 2016/05/31 13:04:51, magjed_webrtc wrote: > > > If you add these methods: > > > abstract protected String getNameOfFrontFacingDevice(); > > > abstract protected String getNameOfBackFacingDevice(); > > > and replace CameraEnumerationAndroid.getDeviceName(0) with "", can you make > > all > > > tests common? > > > > Not all but some. Done. > > What functions do you need to extract into abstract functions in order to make > all tests common? I see Camera.open(), anything else? Can we use > createCapturer() twice in order to achieve the same effect? I feel rewrite is best option for these tests. I am doing some refactoring on the tests but I'd rather do it in a separate CL.
magjed@webrtc.org changed reviewers: + perkj@webrtc.org
lgtm if you refactor the camera1 specific tests into common tests in a follow-up CL. https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/20001/webrtc/api/androidtests/s... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:20: abstract protected CameraVideoCapturer createCapturer( On 2016/06/01 08:39:49, sakal wrote: > On 2016/05/31 14:44:40, magjed_webrtc wrote: > > On 2016/05/31 14:22:36, sakal wrote: > > > On 2016/05/31 13:04:51, magjed_webrtc wrote: > > > > If you add these methods: > > > > abstract protected String getNameOfFrontFacingDevice(); > > > > abstract protected String getNameOfBackFacingDevice(); > > > > and replace CameraEnumerationAndroid.getDeviceName(0) with "", can you > make > > > all > > > > tests common? > > > > > > Not all but some. Done. > > > > What functions do you need to extract into abstract functions in order to make > > all tests common? I see Camera.open(), anything else? Can we use > > createCapturer() twice in order to achieve the same effect? > > I feel rewrite is best option for these tests. I am doing some refactoring on > the tests but I'd rather do it in a separate CL. Alright. https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:19: public class Camera1CapturerTest extends CameraVideoCapturerTest { Maybe rename this class to Camera1CapturerUsingByteBuffersTest for clarity. https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:145: if(deviceName == null) { nit: space between 'if' and '('. I'll give you a peer bonus if you fix 'git cl format' to work correctly for java files. Maybe it's simple and we just need to update some clang format configuration file. https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:158: public void testStartBackFacingVideoCapturer() throws InterruptedException { Maybe move the duplicated code into a helper function testStartVideoCapturer(String deviceName)? https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:286: surfaceTextureHelper.returnTextureFrame(); I think it's ok to call surfaceTextureHelper.returnTextureFrame() for byte buffer frames as well (it should be a no-op) if you want to remove the |isCapturingToTexture| argument. https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:492: static public void cameraCallsAfterStop( Can you move this function back to the position where it used to be to make the diff smaller? https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/java/src/org/... File webrtc/api/java/src/org/webrtc/VideoCapturer.java (right): https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/java/src/org/... webrtc/api/java/src/org/webrtc/VideoCapturer.java:107: void changeCaptureFormat(int width, int height, int framerate); Maybe this should be moved into the CameraVideoCapturer interface. I'm not sure if we should require e.g. screen capture to implement this (but I think it's possible for screen capture). We can move it if it becomes a problem.
https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:19: public class Camera1CapturerTest extends CameraVideoCapturerTest { On 2016/06/01 13:52:28, magjed_webrtc wrote: > Maybe rename this class to Camera1CapturerUsingByteBuffersTest for clarity. Hmm, then Camera1CapturerUsingTextures extending this would be quite confusing. Should we maybe create a new class instead that extends this class and make this an abstract class containing Camera1 specific tests? https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:145: if(deviceName == null) { On 2016/06/01 13:52:28, magjed_webrtc wrote: > nit: space between 'if' and '('. I'll give you a peer bonus if you fix 'git cl > format' to work correctly for java files. Maybe it's simple and we just need to > update some clang format configuration file. Done. I'll take a look at the 'git cl format' at some point if I have time. https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTest.java:158: public void testStartBackFacingVideoCapturer() throws InterruptedException { On 2016/06/01 13:52:28, magjed_webrtc wrote: > Maybe move the duplicated code into a helper function > testStartVideoCapturer(String deviceName)? Having the function name start with test causes trouble with the testing framework so I named it |startVideoCapturerWithNameTestHelper|. https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:286: surfaceTextureHelper.returnTextureFrame(); On 2016/06/01 13:52:28, magjed_webrtc wrote: > I think it's ok to call surfaceTextureHelper.returnTextureFrame() for byte > buffer frames as well (it should be a no-op) if you want to remove the > |isCapturingToTexture| argument. I actually did this in the follow-up CL. https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:492: static public void cameraCallsAfterStop( On 2016/06/01 13:52:28, magjed_webrtc wrote: > Can you move this function back to the position where it used to be to make the > diff smaller? Done. https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/java/src/org/... File webrtc/api/java/src/org/webrtc/VideoCapturer.java (right): https://codereview.webrtc.org/2024843002/diff/120001/webrtc/api/java/src/org/... webrtc/api/java/src/org/webrtc/VideoCapturer.java:107: void changeCaptureFormat(int width, int height, int framerate); On 2016/06/01 13:52:28, magjed_webrtc wrote: > Maybe this should be moved into the CameraVideoCapturer interface. I'm not sure > if we should require e.g. screen capture to implement this (but I think it's > possible for screen capture). We can move it if it becomes a problem. Acknowledged.
Can you please run git cl upload --similarity=20 ? I hope it will diff the files you renamed then. https://codereview.webrtc.org/2024843002/diff/160001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/160001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:86: public void testSwitchVideoCapturer() throws InterruptedException { Why is this needed if super already implement a method called testSwitchVideoCapturer() ? Does this mean that all these tests run twice? https://codereview.webrtc.org/2024843002/diff/160001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:132: // public void testCameraFreezedEventOnBufferStarvation() throws InterruptedException { Please fix. https://codereview.webrtc.org/2024843002/diff/160001/webrtc/build/android/tes... File webrtc/build/android/test_runner.py (right): https://codereview.webrtc.org/2024843002/diff/160001/webrtc/build/android/tes... webrtc/build/android/test_runner.py:30: from pylib.instrumentation import instrumentation_test_instance what is this doing?
https://codereview.webrtc.org/2024843002/diff/160001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/160001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:86: public void testSwitchVideoCapturer() throws InterruptedException { On 2016/06/08 08:48:10, perkj_webrtc wrote: > Why is this needed if super already implement a method called > testSwitchVideoCapturer() ? Does this mean that all these tests run twice? Test runner doesn't see the tests unless they are re-implemented like this. This just gets the test run once. https://codereview.webrtc.org/2024843002/diff/160001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:132: // public void testCameraFreezedEventOnBufferStarvation() throws InterruptedException { On 2016/06/08 08:48:10, perkj_webrtc wrote: > Please fix. The test doesn't really make sense for byte-buffer objects since they don't need to be returned like texture objects. I could move the test to Camera1CapturerUsingTexturesTest but then I would have to also copy it to Camera2CapturerTest in the future, which is not ideal. I think commenting the test out here is lesser evil than code duplication. https://codereview.webrtc.org/2024843002/diff/160001/webrtc/build/android/tes... File webrtc/build/android/test_runner.py (right): https://codereview.webrtc.org/2024843002/diff/160001/webrtc/build/android/tes... webrtc/build/android/test_runner.py:30: from pylib.instrumentation import instrumentation_test_instance On 2016/06/08 08:48:10, perkj_webrtc wrote: > what is this doing? The test runner tries to run the tests defined in CameraVideoCapturerTest. Since it is an abstract class they fail. I worked around this by adding Suppress annotation to the tests. Since we are loaning the Chromium test runner I can't really make changes to it in order to support this annotation. Instead, I just change one of the constants here to make it support it. I wonder if changing the CameraVideoCapturerTest class name not to end in Test would also make the test runner to ignore it. I'll try that since I think it is a cleaner solution if it works.
Patchset #12 (id:220001) has been deleted
https://codereview.webrtc.org/2024843002/diff/160001/webrtc/build/android/tes... File webrtc/build/android/test_runner.py (right): https://codereview.webrtc.org/2024843002/diff/160001/webrtc/build/android/tes... webrtc/build/android/test_runner.py:30: from pylib.instrumentation import instrumentation_test_instance On 2016/06/08 09:20:14, sakal wrote: > On 2016/06/08 08:48:10, perkj_webrtc wrote: > > what is this doing? > > The test runner tries to run the tests defined in CameraVideoCapturerTest. Since > it is an abstract class they fail. I worked around this by adding Suppress > annotation to the tests. Since we are loaning the Chromium test runner I can't > really make changes to it in order to support this annotation. Instead, I just > change one of the constants here to make it support it. > > I wonder if changing the CameraVideoCapturerTest class name not to end in Test > would also make the test runner to ignore it. I'll try that since I think it is > a cleaner solution if it works. Changing the name of the CameraVideoCapturerTest class in fact made it be ignored by the test runner. I think this is a cleaner solution.
Can we continue to use the delegate instead of both using inheritance and delegation? https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:147: // public void testCameraFreezedEventOnBufferStarvation() throws InterruptedException { Please fix https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestBase.java (right): https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestBase.java:30: Logging.w(TAG, "Skipping video capturer test because device name is null."); I think null deviceName should fail the test. What if getNameOfFrontFacingDevice() returns null. https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:18: import org.webrtc.CameraVideoCapturerTestFixtures; remove import org.webrtc.CameraVideoCapturerTestFixtures;
Patchset #15 (id:300001) has been deleted
Patchset #15 (id:320001) has been deleted
I've now integrated my test refactor branch into this CL. Sadly, that made it a little bit bigger. > Can we continue to use the delegate instead of both using inheritance and > delegation? Implemented now. > https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... > File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java (right): > > https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... > webrtc/api/androidtests/src/org/webrtc/Camera1CapturerTest.java:147: // public > void testCameraFreezedEventOnBufferStarvation() throws InterruptedException { > Please fix Removed the test completely from this file. > https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... > File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestBase.java > (right): > > https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... > webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestBase.java:30: > Logging.w(TAG, "Skipping video capturer test because device name is null."); > I think null deviceName should fail the test. What if > getNameOfFrontFacingDevice() returns null. I think it returns null for example if device doesn't have a front camera. Should we really fail the test in this case? > https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... > File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java > (right): > > https://codereview.webrtc.org/2024843002/diff/280001/webrtc/api/androidtests/... > webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:18: > import org.webrtc.CameraVideoCapturerTestFixtures; > remove import org.webrtc.CameraVideoCapturerTestFixtures; Done.
Looks very nice. Just some comment on naming. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerUsingTextureTest.java (right): https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerUsingTextureTest.java:171: @LargeTest Please check the definition of these test annotations with phoglund. I suspect that none of the tests are large tests. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:92: private Object frameLock = new Object(); final https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:93: private Object capturerStartLock = new Object(); final https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:95: private List<Long> timestamps = new ArrayList<Long>(); final https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:231: static private class RendererInstance { Can you add a description of this class? Also, a source and a track is used for more than just rendering. So maybe a more descriptive name or some explanation what they are used for. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:320: private void disposeRenderer(RendererInstance instance) { this dispose a source and a track. Not a renderer. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:442: final RendererInstance renderer = createRenderer(capturerInstance.capturer); maybe name createSourceTrackAndRenderer ? https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:442: final RendererInstance renderer = createRenderer(capturerInstance.capturer); Maybe rename RendererInstance to TestObjects or similar? TestObject is also a bad name but at least give a hint that is not just a renderer. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/java/android/... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:80: private final static int MAX_OPEN_CAMERA_ATTEMPTS = 5; please revert this change.
https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerUsingTextureTest.java (right): https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerUsingTextureTest.java:171: @LargeTest On 2016/06/10 08:07:20, perkj_webrtc wrote: > Please check the definition of these test annotations with phoglund. I suspect > that none of the tests are large tests. There doesn't seem to be clear definition about these sizes. The way I've done it: < 1 s -> SmallTest, 1 - 5 s -> MediumTest, > 5 s -> LargeTest https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:92: private Object frameLock = new Object(); On 2016/06/10 08:07:20, perkj_webrtc wrote: > final Done. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:93: private Object capturerStartLock = new Object(); On 2016/06/10 08:07:20, perkj_webrtc wrote: > final Done. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:95: private List<Long> timestamps = new ArrayList<Long>(); On 2016/06/10 08:07:20, perkj_webrtc wrote: > final Done. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:231: static private class RendererInstance { On 2016/06/10 08:07:20, perkj_webrtc wrote: > Can you add a description of this class? Also, a source and a track is used for > more than just rendering. So maybe a more descriptive name or some explanation > what they are used for. Done. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:320: private void disposeRenderer(RendererInstance instance) { On 2016/06/10 08:07:20, perkj_webrtc wrote: > this dispose a source and a track. Not a renderer. Renamed. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:442: final RendererInstance renderer = createRenderer(capturerInstance.capturer); On 2016/06/10 08:07:20, perkj_webrtc wrote: > Maybe rename RendererInstance to TestObjects or similar? TestObject is also a > bad name but at least give a hint that is not just a renderer. I don't want to name it TestObject because then TestObjectFactory doesn't produce TestObjects which would be highly confusing. I named it VideoSourceInstance. It is not a great name so if you have better ideas please comment. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:442: final RendererInstance renderer = createRenderer(capturerInstance.capturer); On 2016/06/10 08:07:21, perkj_webrtc wrote: > maybe name createSourceTrackAndRenderer ? Renamed to createVideoSourceInstance. https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/java/android/... File webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java (right): https://codereview.webrtc.org/2024843002/diff/360001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/VideoCapturerAndroid.java:80: private final static int MAX_OPEN_CAMERA_ATTEMPTS = 5; On 2016/06/10 08:07:21, perkj_webrtc wrote: > please revert this change. I implemented some changes to allow this to be reverted.
https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:233: * Class for collecting objects related to video source attached to capturer. Instance is created s/ "to a video source attached to a Or maybe something like Class used for collecting a VideoSource, a VideoTrack and a renderer. The class is used for testing local rendering from a capturer. https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:238: static private class VideoSourceInstance { class VideoTrackWithRenderer ? https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:313: private VideoSourceInstance createVideoSourceInstance(CameraVideoCapturer capturer, suggest createVideoTrackWithRenderer ? https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:344: * Class for collecting objects related to video source attached to capturer. I think you can remove this comment and the comments above about Class for... . They don't add any value an is wrong since these are methods that creates /dispose an instance of a certain class.
https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:233: * Class for collecting objects related to video source attached to capturer. Instance is created On 2016/06/10 11:57:37, perkj_webrtc wrote: > s/ "to a video source attached to a > > Or maybe something like > Class used for collecting a VideoSource, a VideoTrack and a renderer. The class > is used for testing local rendering from a capturer. Done. https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:238: static private class VideoSourceInstance { On 2016/06/10 11:57:36, perkj_webrtc wrote: > class VideoTrackWithRenderer ? Done. https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:313: private VideoSourceInstance createVideoSourceInstance(CameraVideoCapturer capturer, On 2016/06/10 11:57:36, perkj_webrtc wrote: > suggest createVideoTrackWithRenderer ? Done. Also changed name of the corresponding dispose method. https://codereview.webrtc.org/2024843002/diff/400001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:344: * Class for collecting objects related to video source attached to capturer. On 2016/06/10 11:57:37, perkj_webrtc wrote: > I think you can remove this comment and the comments above about Class for... . > They don't add any value an is wrong since these are methods that creates > /dispose an instance of a certain class. I don't even know these comments ended up here... Oops. Must have pressed wrong key while changing names of things.
https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerUsingByteBufferTest.java (right): https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerUsingByteBufferTest.java:62: public Object rawOpenCamera(String cameraId) { You use a String here. Why did you change getCameraId to return a String instead of int? Can we switch back? https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:610: final Object competingCamera = testObjectFactory.rawOpenCamera( I think this should be done in the other order. The test was written to test that even if another app has opened a camera, we should not fail. So please first open the "raw" camera and then the capturer to test. https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/java/android/... File webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:59: String getCurrentCameraId(); Can we remove this from the interface again. The test can maybe just always open the front camera ? Or can we retrieve the name instead? When a camera is created or enumerated we require a name that is a String. So this could be String getCurrentCameraName ?
I changed rawOpenCamera to take name instead of an id. For this reason I needed to move getCameraIndex to CameraEnumerationAndroid. Right now, I feel it best fits there. The methods will be moved somewhere else in the future though because they are camera1 specific. https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/Camera1CapturerUsingByteBufferTest.java (right): https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/Camera1CapturerUsingByteBufferTest.java:62: public Object rawOpenCamera(String cameraId) { On 2016/06/14 06:02:34, perkj_webrtc wrote: > You use a String here. Why did you change getCameraId to return a String instead > of int? Can we switch back? I use String here because in Camera2 API camera ids are strings. For the Nexus devices I've tried, they have been strings "0" for back camera and "1" for front camera but I don't think we should rely on that. https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/androidtests/... File webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java (right): https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/androidtests/... webrtc/api/androidtests/src/org/webrtc/CameraVideoCapturerTestFixtures.java:610: final Object competingCamera = testObjectFactory.rawOpenCamera( On 2016/06/14 06:02:35, perkj_webrtc wrote: > I think this should be done in the other order. The test was written to test > that even if another app has opened a camera, we should not fail. So please > first open the "raw" camera and then the capturer to test. Camera is not actually opened on createCapturer. This order is as it was and is necessary for the test to work. https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/java/android/... File webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java (right): https://codereview.webrtc.org/2024843002/diff/420001/webrtc/api/java/android/... webrtc/api/java/android/org/webrtc/CameraVideoCapturer.java:59: String getCurrentCameraId(); On 2016/06/14 06:02:35, perkj_webrtc wrote: > Can we remove this from the interface again. The test can maybe just always > open the front camera ? Or can we retrieve the name instead? When a camera is > created or enumerated we require a name that is a String. So this could be > String getCurrentCameraName ? Done.
lgtm But please add BUG= the bug number for implementing camera2.
Description was changed from ========== Refactor VideoCapturerAndroid tests in WebRTC. Camera1 tests are now separated from general CameraVideoCapturer tests. Main motivation behind these changes is that Camera2 implementation can be tested using the same tests. CL also reduces code duplication on tests using textures. ========== to ========== Refactor VideoCapturerAndroid tests in WebRTC. Camera1 tests are now separated from general CameraVideoCapturer tests. Main motivation behind these changes is that Camera2 implementation can be tested using the same tests. CL also reduces code duplication on tests using textures. BUG=webrtc:5519 ==========
The CQ bit was checked by sakal@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/2024843002/460001
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 sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2024843002/#ps460001 (title: "Reorder imports to match Java style guide")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2024843002/460001
Message was sent while issue was closed.
Description was changed from ========== Refactor VideoCapturerAndroid tests in WebRTC. Camera1 tests are now separated from general CameraVideoCapturer tests. Main motivation behind these changes is that Camera2 implementation can be tested using the same tests. CL also reduces code duplication on tests using textures. BUG=webrtc:5519 ========== to ========== Refactor VideoCapturerAndroid tests in WebRTC. Camera1 tests are now separated from general CameraVideoCapturer tests. Main motivation behind these changes is that Camera2 implementation can be tested using the same tests. CL also reduces code duplication on tests using textures. BUG=webrtc:5519 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Refactor VideoCapturerAndroid tests in WebRTC. Camera1 tests are now separated from general CameraVideoCapturer tests. Main motivation behind these changes is that Camera2 implementation can be tested using the same tests. CL also reduces code duplication on tests using textures. BUG=webrtc:5519 ========== to ========== Refactor VideoCapturerAndroid tests in WebRTC. Camera1 tests are now separated from general CameraVideoCapturer tests. Main motivation behind these changes is that Camera2 implementation can be tested using the same tests. CL also reduces code duplication on tests using textures. BUG=webrtc:5519 Committed: https://crrev.com/79ede033f6253886502a01c2e0db0a1b28001d51 Cr-Commit-Position: refs/heads/master@{#13130} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/79ede033f6253886502a01c2e0db0a1b28001d51 Cr-Commit-Position: refs/heads/master@{#13130} |
