VideoCapturerAndroid, handle cvo correctly

This cl change VideoCaptureAndroid to handle CVO the same way when capturing to texture as  when using ordinary byte buffers.

Ie, rotation is applied in C++ in the VideoFrameFactory is  apply_rotation_ is set. If not, rotation is sent in RTP.

BUG=webrtc:4993
R=nisse@chromium.org

Committed: https://crrev.com/71f5a9a37750d6ccea110028e3154ee90334ba6d
Cr-Commit-Position: refs/heads/master@{#10986}

[perkj_webrtc, 2015-12-07 21:31:22]

Description was changed from

==========
Handle cvo. WIP

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

to

==========
This cl change VideoCaptureAndroid to handle CVO the same way when capturing to
texture as  when using ordinary byte buffers.

Ie, rotation is applied in C++ in the VideoFrameFactory is  apply_rotation_ is
set. If not, rotation is sent in RTP.

BUG=webrtc:4993
==========

[perkj_webrtc, 2015-12-07 21:34:10]

perkj@webrtc.org changed reviewers:
+ nisse@chromium.org

[perkj_webrtc, 2015-12-07 21:34:10]

Can you please review?

[nisse-chromium (ooo August 14), 2015-12-08 08:46:35]

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/androidvideocapturer_jni.h (right):

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/androidvideocapturer_jni.h:63: void OnTextureFrame(int
width, int height, int rotation, int64_t timestamp_ns,
The rotation has to be one of the values in webrtc::VideoRotation, right?

It's a bit unclear in this interface if it is intended as an arbitrary angle
(and if so, what unit and direction), which happens to be only partially
implemented, or if it should be treated more like an enum.

I think it needs a comment, and some error handling in case an unexpected value
is received.

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/native_handle_impl.cc (left):

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:82: // called that happens and
when it finishes, the reference count to |this|
Keep this comment, if it is still relevant?

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/native_handle_impl.cc (right):

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:55: void
InPlaceMultiplyMatrix(float a[16], const float b[16]) {
Please explain operation. Is it A <-- A * B or A <-- B * A (in math. matrix
notation)? Is it using row-major or column-major order?

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:83: ab_ofs += 4;
Move these updates up to the for(;;...). Maybe rename *_ofs to *_row (if
matrices are row-major).

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:88: 16*sizeof(float));
Replace the size expression with sizeof(ab). (Unfortunately, sizeof(a) won't
work, since a really is a pointer; the argument array syntax is only sugar).

Regarding the whole function, I see that we only touch one half of the matrix,
so maybe something simpler and more explicit would actually be shorter. Like, an
outer switch, and then in each case, loop over the columns of a, read out the
two affected values, write back rotated values in place.

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:129:
AndroidTextureBuffer::CropScaleAndRotate(int cropped_input_width,
Maybe we could come up with a better name. TransformFrame or something?

Maybe drop the crop-related arguments, until that is actually implemented. It's
not clear to me how they're intended to be used, and for cropping, won't you
also need some offsets for positioning of the rectangle you cut out?

[perkj_chrome, 2015-12-09 21:00:40]

perkj@chromium.org changed reviewers:
+ perkj@chromium.org

[perkj_chrome, 2015-12-09 21:00:40]

PTAL

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/androidvideocapturer_jni.h (right):

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/androidvideocapturer_jni.h:63: void OnTextureFrame(int
width, int height, int rotation, int64_t timestamp_ns,
On 2015/12/08 08:46:34, nisse-chromium wrote:
> The rotation has to be one of the values in webrtc::VideoRotation, right?
> 
> It's a bit unclear in this interface if it is intended as an arbitrary angle
> (and if so, what unit and direction), which happens to be only partially
> implemented, or if it should be treated more like an enum.
> 
> I think it needs a comment, and some error handling in case an unexpected
value
> is received.

It is the same as OnMemoryBufferFrame and  is checked in
AndroidVideoCapturer::FrameFactory::UpdateCapturedFrame

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/native_handle_impl.cc (left):

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:82: // called that happens and
when it finishes, the reference count to |this|
On 2015/12/08 08:46:34, nisse-chromium wrote:
> Keep this comment, if it is still relevant?

Done.

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/native_handle_impl.cc (right):

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:55: void
InPlaceMultiplyMatrix(float a[16], const float b[16]) {
On 2015/12/08 08:46:34, nisse-chromium wrote:
> Please explain operation. Is it A <-- A * B or A <-- B * A (in math. matrix
> notation)? Is it using row-major or column-major order?

not relevant now?

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:83: ab_ofs += 4;
On 2015/12/08 08:46:34, nisse-chromium wrote:
> Move these updates up to the for(;;...). Maybe rename *_ofs to *_row (if
> matrices are row-major).

Acknowledged.

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:88: 16*sizeof(float));
On 2015/12/08 08:46:34, nisse-chromium wrote:
> Replace the size expression with sizeof(ab). (Unfortunately, sizeof(a) won't
> work, since a really is a pointer; the argument array syntax is only sugar).
> 
> Regarding the whole function, I see that we only touch one half of the matrix,
> so maybe something simpler and more explicit would actually be shorter. Like,
an
> outer switch, and then in each case, loop over the columns of a, read out the
> two affected values, write back rotated values in place.

Done.

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:129:
AndroidTextureBuffer::CropScaleAndRotate(int cropped_input_width,
On 2015/12/08 08:46:34, nisse-chromium wrote:
> Maybe we could come up with a better name. TransformFrame or something?
> 
> Maybe drop the crop-related arguments, until that is actually implemented.
It's
> not clear to me how they're intended to be used, and for cropping, won't you
> also need some offsets for positioning of the rectangle you cut out?

If you don't object too much I will call this ScaleAndRotate in that case.

[nisse-chromium (ooo August 14), 2015-12-10 09:04:57]

lgtm

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/androidvideocapturer_jni.h (right):

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/androidvideocapturer_jni.h:63: void OnTextureFrame(int
width, int height, int rotation, int64_t timestamp_ns,
On 2015/12/09 21:00:40, perkj wrote:
> On 2015/12/08 08:46:34, nisse-chromium wrote:
> > The rotation has to be one of the values in webrtc::VideoRotation, right?
> > 
> > It's a bit unclear in this interface if it is intended as an arbitrary angle
> > (and if so, what unit and direction), which happens to be only partially
> > implemented, or if it should be treated more like an enum.
> > 
> > I think it needs a comment, and some error handling in case an unexpected
> value
> > is received.
> 
> It is the same as OnMemoryBufferFrame and  is checked in
> AndroidVideoCapturer::FrameFactory::UpdateCapturedFrame

I see.

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/native_handle_impl.cc (right):

https://codereview.chromium.org/1493913007/diff/40001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:129:
AndroidTextureBuffer::CropScaleAndRotate(int cropped_input_width,
On 2015/12/09 21:00:40, perkj wrote:
> On 2015/12/08 08:46:34, nisse-chromium wrote:
> > Maybe we could come up with a better name. TransformFrame or something?
> > 
> > Maybe drop the crop-related arguments, until that is actually implemented.
> It's
> > not clear to me how they're intended to be used, and for cropping, won't you
> > also need some offsets for positioning of the rectangle you cut out?
> 
> If you don't object too much I will call this ScaleAndRotate in that case.

Good enough.

https://codereview.chromium.org/1493913007/diff/60001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/native_handle_impl.cc (right):

https://codereview.chromium.org/1493913007/diff/60001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:63: { -a[4], -a[5], -a[6], -a[7],
I was surprised by the transformation of the last row, I had expected that
rotation only applies to the coefficients related to x and y. At least the
second to last row is unchanged.

Proposed comment:

/* Texture coordinates are in the range 0 to 1. The transformation of the last
row is needed for proper translation, e.g, to mirror x, we don't replace x by
-x, but by 1-x. */

If you want to, you could eliminate the memcpy and do a loop over the columns.
E.g, for the 180 case, something like

for (int i = 0; i < 4; i++)
{ float a0, a1, a3;
  a0   = a[i]; a1     = a[4+i]; a3      = a[12+i];
  a[i] = -a0;  a[4+1] = -a1;    a[12+i] = a0 + a1 + a3;
}

You judge whether or not that makes the code simpler. As far as I understand,
the function is not performance critical, so we don't need to eliminate the
memcpy as a micro optimization.

[perkj_chrome, 2015-12-10 13:22:07]

https://codereview.chromium.org/1493913007/diff/60001/talk/app/webrtc/java/jn...
File talk/app/webrtc/java/jni/native_handle_impl.cc (right):

https://codereview.chromium.org/1493913007/diff/60001/talk/app/webrtc/java/jn...
talk/app/webrtc/java/jni/native_handle_impl.cc:63: { -a[4], -a[5], -a[6], -a[7],
On 2015/12/10 09:04:57, nisse-chromium wrote:
> I was surprised by the transformation of the last row, I had expected that
> rotation only applies to the coefficients related to x and y. At least the
> second to last row is unchanged.
> 
> Proposed comment:
> 
> /* Texture coordinates are in the range 0 to 1. The transformation of the last
> row is needed for proper translation, e.g, to mirror x, we don't replace x by
> -x, but by 1-x. */
> 
> If you want to, you could eliminate the memcpy and do a loop over the columns.
> E.g, for the 180 case, something like
> 
> for (int i = 0; i < 4; i++)
> { float a0, a1, a3;
>   a0   = a[i]; a1     = a[4+i]; a3      = a[12+i];
>   a[i] = -a0;  a[4+1] = -a1;    a[12+i] = a0 + a1 + a3;
> }
> 
> You judge whether or not that makes the code simpler. As far as I understand,
> the function is not performance critical, so we don't need to eliminate the
> memcpy as a micro optimization.

Acknowledged.

[perkj_chrome, 2015-12-10 13:22:39]

The CQ bit was checked by perkj@chromium.org

[perkj_chrome, 2015-12-10 13:22:39]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@chromium.org
Link to the patchset: https://codereview.chromium.org/1493913007/#ps80001
(title: "Added comment suggested by nisse.")

[commit-bot: I haz the power, 2015-12-10 13:22:54]

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

[commit-bot: I haz the power, 2015-12-10 15:23:27]

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

[commit-bot: I haz the power, 2015-12-10 15:23:27]

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

[perkj_webrtc, 2015-12-10 17:24:51]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2015-12-10 17:24:51]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@chromium.org
Link to the patchset: https://codereview.webrtc.org/1493913007/#ps100001 (title:
"Rebased")

[commit-bot: I haz the power, 2015-12-10 17:25:02]

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

[commit-bot: I haz the power, 2015-12-10 19:25:19]

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

[commit-bot: I haz the power, 2015-12-10 19:25:20]

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

[perkj_webrtc, 2015-12-11 08:32:54]

Description was changed from

==========
This cl change VideoCaptureAndroid to handle CVO the same way when capturing to
texture as  when using ordinary byte buffers.

Ie, rotation is applied in C++ in the VideoFrameFactory is  apply_rotation_ is
set. If not, rotation is sent in RTP.

BUG=webrtc:4993
==========

to

==========
This cl change VideoCaptureAndroid to handle CVO the same way when capturing to
texture as  when using ordinary byte buffers.

Ie, rotation is applied in C++ in the VideoFrameFactory is  apply_rotation_ is
set. If not, rotation is sent in RTP.

BUG=webrtc:4993
R=nisse@chromium.org

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

[perkj_webrtc, 2015-12-11 08:32:58]

Committed patchset #6 (id:100001) manually as
71f5a9a37750d6ccea110028e3154ee90334ba6d (presubmit successful).

[commit-bot: I haz the power, 2015-12-11 08:33:00]

Description was changed from

==========
This cl change VideoCaptureAndroid to handle CVO the same way when capturing to
texture as  when using ordinary byte buffers.

Ie, rotation is applied in C++ in the VideoFrameFactory is  apply_rotation_ is
set. If not, rotation is sent in RTP.

BUG=webrtc:4993
R=nisse@chromium.org

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

to

==========
This cl change VideoCaptureAndroid to handle CVO the same way when capturing to
texture as  when using ordinary byte buffers.

Ie, rotation is applied in C++ in the VideoFrameFactory is  apply_rotation_ is
set. If not, rotation is sent in RTP.

BUG=webrtc:4993
R=nisse@chromium.org

Committed: https://crrev.com/71f5a9a37750d6ccea110028e3154ee90334ba6d
Cr-Commit-Position: refs/heads/master@{#10986}
==========

[commit-bot: I haz the power, 2015-12-11 08:33:01]

Patchset 6 (id:??) landed as
https://crrev.com/71f5a9a37750d6ccea110028e3154ee90334ba6d
Cr-Commit-Position: refs/heads/master@{#10986}