Video capture Android: enable unittests for Camera2 API

Video capture Android: enable unittests for Camera2 API

This CL reenables a bunch of VideoCaptureDeviceTest
test cases for Android and for the "new" APIs

For this:
- adds a |mTestMode| to VideoCapture.java, interface
 of all camera java classes. VCCamera2.java tests this
 flag and adds a capture thread if it's set.
- the C++ correspondent classes get a method to configure
 the java side for testing, and a method to test if the
 underlying platform is using the new or the old APIs.

Also:
- Removes DeAllocateCameraWhileRunning because it was
 equivalent to CaptureWithSize test case.
- Cleans up/refactors some parts of the unit tests, adds
 a few more comments etc

BUG=626857
Committed: https://crrev.com/7c32b70068c65b7bb372bb52a33d70b8e6653129
Cr-Commit-Position: refs/heads/master@{#417015}

[mcasas, 2016-08-29 22:51:51]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-08-29 22:59:48]

Description was changed from

==========
Video capture Android: enable unittests for Camera2 API

BUG=626857
==========

to

==========
Video capture Android: enable unittests for Camera2 API

This CL reenables a bunch of VideoCaptureDeviceTest
test cases for Android and for the "new" APIs

For this:
- adds a |mTestMode| to VideoCapture.java, interface
 of all camera java classes. VCCamera2.java tests this
 flag and adds a capture thread if it's set.
- the C++ correspondent classes get a method to configure
 the java side for testing, and a method to test if the
 underlying platform is using the new or the old APIs.

Also:
- Removes DeAllocateCameraWhileRunning because it was
 equivalent to CaptureWithSize test case.
- Cleans up/refactors some parts of the unit tests, adds
 a few more comments etc


BUG=626857
==========

[mcasas, 2016-08-30 16:28:29]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-08-30 16:28:54]

mcasas@chromium.org changed reviewers:
+ jbudorick@chromium.org

[mcasas, 2016-08-30 16:28:54]

jbudorick@ please take a first look

[mcasas, 2016-09-01 18:13:30]

mcasas@chromium.org changed reviewers:
+ emircan@chromium.org

[mcasas, 2016-09-01 18:13:30]

jbudorick@ ping

emircan@ PTAL

[jbudorick, 2016-09-01 18:46:42]

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/and...
File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java
(right):

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/and...
media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:37:
protected boolean mTestMode = true;
nit: name this more specifically, something like mUseBackgroundThreadForTesting
or something like that.

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
File media/capture/video/video_capture_device_unittest.cc (left):

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
media/capture/video/video_capture_device_unittest.cc:453:
TEST_F(VideoCaptureDeviceTest, DeAllocateCameraWhileRunning) {
?

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
media/capture/video/video_capture_device_unittest.cc:102: ASSERT_TRUE(!!data);
nit: what's with this change? !! appears to be very uncommon in chromium
tests...

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
media/capture/video/video_capture_device_unittest.cc:228: bool
EnumerateDeviceDescriptors() {
Why did you switch this to return a bool rather than just returning a null
unique_ptr or an empty instance of VideoCaptureDeviceDescriptors if any of the
devices are legacy or deprecated? Having device_descriptors_ assigned in here
seems more confusing than the previous implementation.

[mcasas, 2016-09-01 20:52:21]

Patchset #2 (id:60001) has been deleted

[mcasas, 2016-09-01 20:52:37]

PTAL

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/and...
File media/capture/video/android/java/src/org/chromium/media/VideoCapture.java
(right):

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/and...
media/capture/video/android/java/src/org/chromium/media/VideoCapture.java:37:
protected boolean mTestMode = true;
On 2016/09/01 18:46:41, jbudorick wrote:
> nit: name this more specifically, something like
mUseBackgroundThreadForTesting
> or something like that.

Done. Also, it should be false by default.

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
File media/capture/video/video_capture_device_unittest.cc (left):

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
media/capture/video/video_capture_device_unittest.cc:453:
TEST_F(VideoCaptureDeviceTest, DeAllocateCameraWhileRunning) {
On 2016/09/01 18:46:41, jbudorick wrote:
> ?

Like it says in the CL description...

"Removes DeAllocateCameraWhileRunning because it was
 equivalent to CaptureWithSize test case."

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
media/capture/video/video_capture_device_unittest.cc:102: ASSERT_TRUE(!!data);
On 2016/09/01 18:46:41, jbudorick wrote:
> nit: what's with this change? !! appears to be very uncommon in chromium
> tests...

Yeah, my bad; this comes from msvc compiler
having troubles with if (bla) where bla is
a pointer. If the bots are happy, so am I.

https://codereview.chromium.org/2286303003/diff/40001/media/capture/video/vid...
media/capture/video/video_capture_device_unittest.cc:228: bool
EnumerateDeviceDescriptors() {
On 2016/09/01 18:46:41, jbudorick wrote:
> Why did you switch this to return a bool rather than just returning a null
> unique_ptr or an empty instance of VideoCaptureDeviceDescriptors if any of the
> devices are legacy or deprecated? Having device_descriptors_ assigned in here
> seems more confusing than the previous implementation.

It didn't make a lot of sense to have a member 
method returning the list just to force the 
individual call sites to update a member 
variable; the member variable had to be tested 
then individually at those call sites, and that
was verbose and cluttered the test intentions.  

Actually, I'll even bring more code into this 
method and return true if there are usable 
|device_desciptors_|, and rename accordingly.

[mcasas, 2016-09-01 21:01:38]

Patchset #2 (id:80001) has been deleted

[emircan, 2016-09-02 22:00:23]

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.cc
(right):

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.cc:122: break;
Why is this change? This would push_back the format to supported list with wrong
|pixel_format|.

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.h (right):

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.h:38: void
ConfigureForTesting() { test_mode_ = true; }
Note that this needs to be called before CreateDevice().

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/vi...
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/vi...
media/capture/video/video_capture_device_unittest.cc:390: // captured
Fix indentation.

[mcasas, 2016-09-02 22:01:06]

jbudorick@ ping

emircan@ PTAL

[mcasas, 2016-09-02 22:27:07]

PTAL

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.cc
(right):

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.cc:122: break;
On 2016/09/02 22:00:23, emircan wrote:
> Why is this change? This would push_back the format to supported list with
wrong
> |pixel_format|.

Camera2 does not know the Pixel format at this stage,
so we're just not enumerating any formats on ToT
for Android VideoCaptureDevices. At the enumeration 
state, PIXEL_FORMAT_UNKNOWN is OK, it simply means
that the platform, well, doesn't know :)

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/an...
File media/capture/video/android/video_capture_device_factory_android.h (right):

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/an...
media/capture/video/android/video_capture_device_factory_android.h:38: void
ConfigureForTesting() { test_mode_ = true; }
On 2016/09/02 22:00:23, emircan wrote:
> Note that this needs to be called before CreateDevice().

Added a comment.

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/vi...
File media/capture/video/video_capture_device_unittest.cc (right):

https://codereview.chromium.org/2286303003/diff/100001/media/capture/video/vi...
media/capture/video/video_capture_device_unittest.cc:390: // captured
On 2016/09/02 22:00:23, emircan wrote:
> Fix indentation.

Done.

[emircan, 2016-09-06 17:35:53]

lgtm

[jbudorick, 2016-09-06 18:44:23]

https://codereview.chromium.org/2286303003/diff/120001/media/capture/video/an...
File
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/2286303003/diff/120001/media/capture/video/an...
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java:540:
thread.start();
Do we stop this thread at any point?

[mcasas, 2016-09-06 22:48:36]

PTAL

https://codereview.chromium.org/2286303003/diff/120001/media/capture/video/an...
File
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java
(right):

https://codereview.chromium.org/2286303003/diff/120001/media/capture/video/an...
media/capture/video/android/java/src/org/chromium/media/VideoCaptureCamera2.java:540:
thread.start();
On 2016/09/06 18:44:23, jbudorick wrote:
> Do we stop this thread at any point?

Now we do (its Looper, since Thread.quit()
and stop() are discouraged).

[jbudorick, 2016-09-06 22:51:23]

lgtm

[mcasas, 2016-09-06 23:00:42]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-09-06 23:00:43]

The patchset sent to the CQ was uploaded after l-g-t-m from emircan@chromium.org
Link to the patchset: https://codereview.chromium.org/2286303003/#ps140001
(title: "jbudorick@ comment: Stopping mMainHandler thread in test mode")

[commit-bot: I haz the power, 2016-09-06 23:01:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 23:13:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 23:13:27]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcasas, 2016-09-07 05:23:28]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-09-07 05:23:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 05:34:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 05:34:53]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mcasas, 2016-09-07 17:09:18]

Patchset #5 (id:160001) has been deleted

[mcasas, 2016-09-07 17:09:28]

Patchset #5 (id:180001) has been deleted

[mcasas, 2016-09-07 17:29:28]

Patchset #5 (id:200001) has been deleted

[mcasas, 2016-09-07 17:30:25]

The CQ bit was checked by mcasas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 17:30:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 18:43:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 18:43:39]

Dry run: This issue passed the CQ dry run.

[mcasas, 2016-09-07 18:53:33]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2016-09-07 18:53:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
emircan@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2286303003/#ps220001
(title: "Addressing mem leak. Rebase")

[commit-bot: I haz the power, 2016-09-07 18:54:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 18:58:50]

Description was changed from

==========
Video capture Android: enable unittests for Camera2 API

This CL reenables a bunch of VideoCaptureDeviceTest
test cases for Android and for the "new" APIs

For this:
- adds a |mTestMode| to VideoCapture.java, interface
 of all camera java classes. VCCamera2.java tests this
 flag and adds a capture thread if it's set.
- the C++ correspondent classes get a method to configure
 the java side for testing, and a method to test if the
 underlying platform is using the new or the old APIs.

Also:
- Removes DeAllocateCameraWhileRunning because it was
 equivalent to CaptureWithSize test case.
- Cleans up/refactors some parts of the unit tests, adds
 a few more comments etc


BUG=626857
==========

to

==========
Video capture Android: enable unittests for Camera2 API

This CL reenables a bunch of VideoCaptureDeviceTest
test cases for Android and for the "new" APIs

For this:
- adds a |mTestMode| to VideoCapture.java, interface
 of all camera java classes. VCCamera2.java tests this
 flag and adds a capture thread if it's set.
- the C++ correspondent classes get a method to configure
 the java side for testing, and a method to test if the
 underlying platform is using the new or the old APIs.

Also:
- Removes DeAllocateCameraWhileRunning because it was
 equivalent to CaptureWithSize test case.
- Cleans up/refactors some parts of the unit tests, adds
 a few more comments etc


BUG=626857
==========

[commit-bot: I haz the power, 2016-09-07 18:58:51]

Committed patchset #5 (id:220001)

[commit-bot: I haz the power, 2016-09-07 19:00:03]

Description was changed from

==========
Video capture Android: enable unittests for Camera2 API

This CL reenables a bunch of VideoCaptureDeviceTest
test cases for Android and for the "new" APIs

For this:
- adds a |mTestMode| to VideoCapture.java, interface
 of all camera java classes. VCCamera2.java tests this
 flag and adds a capture thread if it's set.
- the C++ correspondent classes get a method to configure
 the java side for testing, and a method to test if the
 underlying platform is using the new or the old APIs.

Also:
- Removes DeAllocateCameraWhileRunning because it was
 equivalent to CaptureWithSize test case.
- Cleans up/refactors some parts of the unit tests, adds
 a few more comments etc


BUG=626857
==========

to

==========
Video capture Android: enable unittests for Camera2 API

This CL reenables a bunch of VideoCaptureDeviceTest
test cases for Android and for the "new" APIs

For this:
- adds a |mTestMode| to VideoCapture.java, interface
 of all camera java classes. VCCamera2.java tests this
 flag and adds a capture thread if it's set.
- the C++ correspondent classes get a method to configure
 the java side for testing, and a method to test if the
 underlying platform is using the new or the old APIs.

Also:
- Removes DeAllocateCameraWhileRunning because it was
 equivalent to CaptureWithSize test case.
- Cleans up/refactors some parts of the unit tests, adds
 a few more comments etc

BUG=626857

Committed: https://crrev.com/7c32b70068c65b7bb372bb52a33d70b8e6653129
Cr-Commit-Position: refs/heads/master@{#417015}
==========

[commit-bot: I haz the power, 2016-09-07 19:00:05]

Patchset 5 (id:??) landed as
https://crrev.com/7c32b70068c65b7bb372bb52a33d70b8e6653129
Cr-Commit-Position: refs/heads/master@{#417015}

[aelias_OOO_until_Jul13, 2016-09-08 01:27:00]

A revert of this CL (patchset #5 id:220001) has been created in
https://codereview.chromium.org/2320873002/ by aelias@chromium.org.

The reason for reverting is: Caused WebViewLayoutTest#testMediaStreamApi and
MediaAccessPermissionRequestTest#testGrantAccess to consistently fail.

Confirmed locally by reverting patch, commenting out @DisableIf and running:

ninja -C out/Release android_webview_test_apk android_webview_apk &&
out/Release/bin/run_android_webview_test_apk -f
MediaAccessPermissionRequestTest.*

BUG=644910.