Android: Change camera fps range selection

Android: Change camera fps range selection

This CL changes the logic in
CameraEnumerationAndroid.getClosestSupportedFramerateRange() to prefer
fps ranges with a low lower bound so the camera can adjust for
brightness conditions.

To test the functionality of the fps range selection, JUnit tests are
added. This required a new target in api_tests.gyp. JUnit tests are
preferable over instrumentation tests
(libjingle_peerconnection_android_unittest) because they are faster and
simpler.

R=kjellander@webrtc.org, sakal@webrtc.org

Committed: https://crrev.com/b4ddb5c3d3706b1c02437f6a538576f3552ab908
Cr-Commit-Position: refs/heads/master@{#12964}

[magjed_webrtc, 2016-05-27 12:32:37]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-05-27 12:34:12]

Description was changed from

==========
Android: Change camera fps range selection

This CL changes the logic in
CameraEnumerationAndroid.getClosestSupportedFramerateRange() to prefer
fps ranges with a low lower bound so the camera can adjust for
brightness conditions.

To test the functionality of the fps range selection, junit tests are
added. This required a new target in api_tests.gyp.
==========

to

==========
Android: Change camera fps range selection

This CL changes the logic in
CameraEnumerationAndroid.getClosestSupportedFramerateRange() to prefer
fps ranges with a low lower bound so the camera can adjust for
brightness conditions.

To test the functionality of the fps range selection, JUnit tests are
added. This required a new target in api_tests.gyp. JUnit tests are
preferable over instrumentation tests
(libjingle_peerconnection_android_unittest) because they are faster and
simpler.
==========

[magjed_webrtc, 2016-05-27 12:41:40]

Patchset #1 (id:20001) has been deleted

[magjed_webrtc, 2016-05-27 12:42:16]

magjed@webrtc.org changed reviewers:
+ kjellander@webrtc.org, sakal@webrtc.org

[magjed_webrtc, 2016-05-27 12:42:56]

Please take a look.

[sakal, 2016-05-27 13:10:47]

lgtm

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/java/android/o...
File webrtc/api/java/android/org/webrtc/CameraEnumerationAndroid.java (right):

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/java/android/o...
webrtc/api/java/android/org/webrtc/CameraEnumerationAndroid.java:211:
MIN_FPS_THRESHOLD, MIN_FPS_LOW_VALUE_WEIGHT, MIN_FPS_HIGH_VALUE_WEIGHT)
nit: 11 space indentation?

[kjellander_webrtc, 2016-05-30 08:31:53]

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp
File webrtc/api/api_tests.gyp (right):

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:156: 'target_name': 'android_junit_test',
Please name this android_junit_tests to conform with established convention
(libjingle_peerconnection_android_unittest above is one of few violations).

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:156: 'target_name': 'android_junit_test',
Since it's not called api_android_junit_tests, I expect it to contain Android
junit tests for all of WebRTC. 
That's actually something I'd prefer, since I want to have fewer test
executables overall. But in that case, it should be moved to a higher level,
like
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/webrtc_test....

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:170: 'wrapper_script_name': 'helper/<(_target_name)',
Does compilation of this target generate a run_ script in
out/{Debug,Release}/bin/run_android_junit_test, that works to run
out-of-the-box?
If so, that's great (easier to add to the bots).

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:173: '../../build/host_jar.gypi',
sort alphabetically.

[magjed_webrtc, 2016-05-30 12:47:07]

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp
File webrtc/api/api_tests.gyp (right):

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:156: 'target_name': 'android_junit_test',
On 2016/05/30 08:31:53, kjellander_webrtc wrote:
> Please name this android_junit_tests to conform with established convention
> (libjingle_peerconnection_android_unittest above is one of few violations). 

Done.

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:156: 'target_name': 'android_junit_test',
On 2016/05/30 08:31:53, kjellander_webrtc wrote:
> Since it's not called api_android_junit_tests, I expect it to contain Android
> junit tests for all of WebRTC. 
> That's actually something I'd prefer, since I want to have fewer test
> executables overall. But in that case, it should be moved to a higher level,
> like
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/webrtc_test....

Done, I moved the build target to webrtc_tests.gypi and moved the androidjunit
folder from webrtc/api to webrtc/. Do I need to add a case for
'test_isolation_mode != "noop"' as well?

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:170: 'wrapper_script_name': 'helper/<(_target_name)',
On 2016/05/30 08:31:53, kjellander_webrtc wrote:
> Does compilation of this target generate a run_ script in
> out/{Debug,Release}/bin/run_android_junit_test, that works to run
> out-of-the-box?
> If so, that's great (easier to add to the bots).

Yes, a working run script is generated :)

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:173: '../../build/host_jar.gypi',
On 2016/05/30 08:31:53, kjellander_webrtc wrote:
> sort alphabetically.

Done.

[kjellander_webrtc, 2016-05-30 12:57:28]

lgtm

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp
File webrtc/api/api_tests.gyp (right):

https://codereview.webrtc.org/2013413002/diff/60001/webrtc/api/api_tests.gyp#...
webrtc/api/api_tests.gyp:156: 'target_name': 'android_junit_test',
On 2016/05/30 12:47:07, magjed_webrtc wrote:
> On 2016/05/30 08:31:53, kjellander_webrtc wrote:
> > Since it's not called api_android_junit_tests, I expect it to contain
Android
> > junit tests for all of WebRTC. 
> > That's actually something I'd prefer, since I want to have fewer test
> > executables overall. But in that case, it should be moved to a higher level,
> > like
> >
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/webrtc_test....
> 
> Done, I moved the build target to webrtc_tests.gypi and moved the androidjunit
> folder from webrtc/api to webrtc/. Do I need to add a case for
> 'test_isolation_mode != "noop"' as well?

No that stuff is only needed for tests that run on swarming and/or Android
devices. And it's only needed if the test depends on any resource files (like
audio/video files), so it can wait until that's true.

[magjed_webrtc, 2016-05-30 14:41:25]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-05-30 14:41:26]

The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2013413002/#ps80001 (title:
"Move from api to higher level")

[commit-bot: I haz the power, 2016-05-30 14:41:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2013413002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2013413002/80001

[commit-bot: I haz the power, 2016-05-30 14:46:21]

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

[commit-bot: I haz the power, 2016-05-30 14:46:22]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/5951)

[magjed_webrtc, 2016-05-31 08:18:22]

I'm adding me and sakal@ as owners of the new 	webrtc/androidjunit directory.

[magjed_webrtc, 2016-05-31 08:19:07]

Description was changed from

==========
Android: Change camera fps range selection

This CL changes the logic in
CameraEnumerationAndroid.getClosestSupportedFramerateRange() to prefer
fps ranges with a low lower bound so the camera can adjust for
brightness conditions.

To test the functionality of the fps range selection, JUnit tests are
added. This required a new target in api_tests.gyp. JUnit tests are
preferable over instrumentation tests
(libjingle_peerconnection_android_unittest) because they are faster and
simpler.
==========

to

==========
Android: Change camera fps range selection

This CL changes the logic in
CameraEnumerationAndroid.getClosestSupportedFramerateRange() to prefer
fps ranges with a low lower bound so the camera can adjust for
brightness conditions.

To test the functionality of the fps range selection, JUnit tests are
added. This required a new target in api_tests.gyp. JUnit tests are
preferable over instrumentation tests
(libjingle_peerconnection_android_unittest) because they are faster and
simpler.

R=kjellander@webrtc.org, sakal@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/b4ddb5c3d3706b1c02437f6a5...
==========

[commit-bot: I haz the power, 2016-05-31 08:19:07]

Description was changed from

==========
Android: Change camera fps range selection

This CL changes the logic in
CameraEnumerationAndroid.getClosestSupportedFramerateRange() to prefer
fps ranges with a low lower bound so the camera can adjust for
brightness conditions.

To test the functionality of the fps range selection, JUnit tests are
added. This required a new target in api_tests.gyp. JUnit tests are
preferable over instrumentation tests
(libjingle_peerconnection_android_unittest) because they are faster and
simpler.
==========

to

==========
Android: Change camera fps range selection

This CL changes the logic in
CameraEnumerationAndroid.getClosestSupportedFramerateRange() to prefer
fps ranges with a low lower bound so the camera can adjust for
brightness conditions.

To test the functionality of the fps range selection, JUnit tests are
added. This required a new target in api_tests.gyp. JUnit tests are
preferable over instrumentation tests
(libjingle_peerconnection_android_unittest) because they are faster and
simpler.

R=kjellander@webrtc.org, sakal@webrtc.org

Committed: https://crrev.com/b4ddb5c3d3706b1c02437f6a538576f3552ab908
Cr-Commit-Position: refs/heads/master@{#12964}
==========

[commit-bot: I haz the power, 2016-05-31 08:19:08]

Patchset 4 (id:??) landed as
https://crrev.com/b4ddb5c3d3706b1c02437f6a538576f3552ab908
Cr-Commit-Position: refs/heads/master@{#12964}

[magjed_webrtc, 2016-05-31 08:19:09]

Committed patchset #4 (id:100001) manually as
b4ddb5c3d3706b1c02437f6a538576f3552ab908 (presubmit successful).

[magjed_webrtc, 2016-05-31 08:44:17]

A revert of this CL (patchset #4 id:100001) has been created in
https://codereview.webrtc.org/2021233002/ by magjed@webrtc.org.

The reason for reverting is: Breaks chromium fyi:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build...
on step 'generate_build_files':
gyp:
/b/build/slave/Mac_Builder/build/src/third_party/build/android/test_runner.gypi
not found.

[kjellander_webrtc, 2016-06-06 19:49:31]

I didn't pay attention to that this CL added a new directory directly in webrtc/
Can we please move it into webrtc/api instead?
I also would prefer renaming it to just "junit" to align better with Chromium's
hierarchy. See https://cs.chromium.org/chromium/src/base/android for an example.