Android VideoCapture : Use NV21 instead of YUV12 and clean up.

Use NV21 instead of YUV12 and clean up.

BUG=webrtc:5375
Committed: https://crrev.com/88518a22c62ccb7989a0e10d43bea1a63cdfcd09
Cr-Commit-Position: refs/heads/master@{#11079}

[perkj_webrtc, 2015-12-16 14:27:08]

Patchset #3 (id:40001) has been deleted

[perkj_webrtc, 2015-12-16 14:27:17]

Patchset #2 (id:20001) has been deleted

[perkj_webrtc, 2015-12-16 14:27:56]

perkj@webrtc.org changed reviewers:
+ magjed@webrtc.org

[perkj_webrtc, 2015-12-16 14:27:56]

Please?

[magjed_webrtc, 2015-12-17 08:02:42]

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/androidte...
File
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTestFixtures.java
(right):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTestFixtures.java:496:
capturer.surfaceHelper.returnTextureFrame();
Why is this moved?

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTestFixtures.java:553:
if (!capturer.isCapturingToTexture()) {
Use assertTrue("Not capturing to textures", capturer.isCapturingToTexture())
instead.

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/CameraEnumerationAndroid.java
(right):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/CameraEnumerationAndroid.java:91: //
multiple of 16 of the width and width and height are always even.
Why did you remove the android link? I'm not sure the stride is a multiple of 16
for NV21. The documentation says: "For YV12, the image buffer that is received
is not necessarily tightly packed, as there may be padding at the end of each
row of pixel data, as described in YV12". Sounds like this is not the case for
NV21? Please look it up and provide a reference.

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (left):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:139: if
(cameraStatistics.pendingFramesCount() == cameraStatistics.maxPendingFrames) {
Why not keep this distinction? The client can still hold the texture frame or
not.

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (right):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:681: // Note
that since data is directBuffer,  data.length >= captureFormat.frameSize.
I think there is a high risk of getting a frame with the old size after
changeCaptureFormat(), same problem dropNextFrame is used for. Please verify
this is not the case, or re-add:
private final Map<byte[], ByteBuffer> queuedBuffers = new
IdentityHashMap<byte[], ByteBuffer>();
...
for (int i = 0; i < NUMBER_OF_CAPTURE_BUFFERS; ++i) {
  final ByteBuffer buffer = ByteBuffer.allocateDirect(frameSize);
  queuedBuffers.put(buffer.array(), buffer);
  camera.addCallbackBuffer(buffer.array());
}
...
if (!queuedBuffers.containsKey(data)) {
  // Drop frame from previous configuration.
  return;
}

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/surfacetexturehelper_jni.h (right):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.h:52: // 1. Create an java
instance of SurfaceTextureHelper.
Why is this a good change? Shouldn't this C++ class own the Java
SurfaceTextureHelper as before?

[perkj_webrtc, 2015-12-17 12:42:44]

PTAL

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/androidte...
File
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTestFixtures.java
(right):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTestFixtures.java:496:
capturer.surfaceHelper.returnTextureFrame();
On 2015/12/17 08:02:42, magjed_webrtc wrote:
> Why is this moved?

Done.

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/androidte...
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTestFixtures.java:553:
if (!capturer.isCapturingToTexture()) {
On 2015/12/17 08:02:42, magjed_webrtc wrote:
> Use assertTrue("Not capturing to textures", capturer.isCapturingToTexture())
> instead.

Done.

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/CameraEnumerationAndroid.java
(right):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/CameraEnumerationAndroid.java:91: //
multiple of 16 of the width and width and height are always even.
On 2015/12/17 08:02:42, magjed_webrtc wrote:
> Why did you remove the android link? I'm not sure the stride is a multiple of
16
> for NV21. The documentation says: "For YV12, the image buffer that is received
> is not necessarily tightly packed, as there may be padding at the end of each
> row of pixel data, as described in YV12". Sounds like this is not the case for
> NV21? Please look it up and provide a reference.

Done.

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java (left):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoCapturerAndroid.java:139: if
(cameraStatistics.pendingFramesCount() == cameraStatistics.maxPendingFrames) {
On 2015/12/17 08:02:42, magjed_webrtc wrote:
> Why not keep this distinction? The client can still hold the texture frame or
> not.

Done.

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/surfacetexturehelper_jni.h (right):

https://codereview.webrtc.org/1530843002/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/surfacetexturehelper_jni.h:52: // 1. Create an java
instance of SurfaceTextureHelper.
On 2015/12/17 08:02:42, magjed_webrtc wrote:
> Why is this a good change? Shouldn't this C++ class own the Java
> SurfaceTextureHelper as before?

it holds the reference still. But see how it is used now both in the decoder and
capturer. 
It does not make sence to inject the SurfaceTextureHandler in
VideoCapturerAndroid from C++.

[magjed_webrtc, 2015-12-17 14:28:40]

lgtm

[perkj_webrtc, 2015-12-17 14:48:47]

Description was changed from

==========
Use NV21 instead of YUV12 and clean up.

BUG=
==========

to

==========
Use NV21 instead of YUV12 and clean up.

BUG=webrtc:5375
==========

[perkj_webrtc, 2015-12-17 14:49:02]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2015-12-17 14:49:07]

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

[commit-bot: I haz the power, 2015-12-17 16:49:31]

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

[commit-bot: I haz the power, 2015-12-17 16:49:32]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[perkj_webrtc, 2015-12-18 08:33:23]

The CQ bit was checked by perkj@webrtc.org

[commit-bot: I haz the power, 2015-12-18 08:33:31]

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

[commit-bot: I haz the power, 2015-12-18 08:37:09]

Description was changed from

==========
Use NV21 instead of YUV12 and clean up.

BUG=webrtc:5375
==========

to

==========
Use NV21 instead of YUV12 and clean up.

BUG=webrtc:5375
==========

[commit-bot: I haz the power, 2015-12-18 08:37:09]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-12-18 08:37:19]

Description was changed from

==========
Use NV21 instead of YUV12 and clean up.

BUG=webrtc:5375
==========

to

==========
Use NV21 instead of YUV12 and clean up.

BUG=webrtc:5375

Committed: https://crrev.com/88518a22c62ccb7989a0e10d43bea1a63cdfcd09
Cr-Commit-Position: refs/heads/master@{#11079}
==========

[commit-bot: I haz the power, 2015-12-18 08:37:20]

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