Implement AndroidVideoBuffer::ToI420.

Implement AndroidVideoBuffer::ToI420.

BUG=webrtc:7749, webrtc:7760
Review-Url: https://codereview.webrtc.org/2991633002
Cr-Commit-Position: refs/heads/master@{#19284}
Committed: https://chromium.googlesource.com/external/webrtc/+/5ca60cc91cb21d4eded8b7ee401ee0b6cf280a2c

[sakal, 2017-07-25 12:04:26]

Description was changed from

==========
Implement AndroidVideoBuffer::ToI420.

BUG=webrtc:7749, webrtc:7760
==========

to

==========
Implement AndroidVideoBuffer::ToI420.

BUG=webrtc:7749, webrtc:7760
==========

[sakal, 2017-07-25 12:04:26]

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

[sakal, 2017-07-25 12:04:33]

PTAL

[sakal, 2017-07-26 12:01:00]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2017-07-27 14:45:01]

https://codereview.webrtc.org/2991633002/diff/40001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (right):

https://codereview.webrtc.org/2991633002/diff/40001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:30: class
AndroidVideoI420Buffer : public AndroidVideoBuffer,
Don't inherit from AndroidVideoBuffer, because you will get the diamond problem.

[sakal, 2017-07-28 09:01:11]

I also implemented caching of the values to avoid multiple ScopedLocalRefFrames.

https://codereview.webrtc.org/2991633002/diff/40001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (right):

https://codereview.webrtc.org/2991633002/diff/40001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:30: class
AndroidVideoI420Buffer : public AndroidVideoBuffer,
On 2017/07/27 14:45:01, magjed_webrtc wrote:
> Don't inherit from AndroidVideoBuffer, because you will get the diamond
problem.

Done.

[magjed_webrtc, 2017-07-28 09:45:05]

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (left):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:200: jmethodID j_retain_id,
Revert this change in this CL.

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (right):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:77: new
rtc::RefCountedObject<AndroidVideoBuffer>(jni,
I think it's clearer to duplicate the code you need from AndroidVideoBuffer,
i.e. the global jobject and the call to release in the dtor.

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:319:
jni->CallObjectMethod(*j_video_frame_buffer_, j_to_i420_id);
How about extracting j_i420_buffer here and passing it to the 
AndroidVideoI420Buffer ctor. The rest of the code can go inside the ctor.

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.h (left):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.h:131: // Holds a
VideoFrame.Buffer.
Keep this comment.

[sakal, 2017-07-28 11:08:48]

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (left):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:200: jmethodID j_retain_id,
On 2017/07/28 09:45:05, magjed_webrtc wrote:
> Revert this change in this CL.

Done.

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (right):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:319:
jni->CallObjectMethod(*j_video_frame_buffer_, j_to_i420_id);
On 2017/07/28 09:45:05, magjed_webrtc wrote:
> How about extracting j_i420_buffer here and passing it to the 
> AndroidVideoI420Buffer ctor. The rest of the code can go inside the ctor.

Done.

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.h (left):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.h:131: // Holds a
VideoFrame.Buffer.
On 2017/07/28 09:45:05, magjed_webrtc wrote:
> Keep this comment.

Done.

[sakal, 2017-07-28 11:08:49]

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (left):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:200: jmethodID j_retain_id,
On 2017/07/28 09:45:05, magjed_webrtc wrote:
> Revert this change in this CL.

Done.

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (right):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.cc:319:
jni->CallObjectMethod(*j_video_frame_buffer_, j_to_i420_id);
On 2017/07/28 09:45:05, magjed_webrtc wrote:
> How about extracting j_i420_buffer here and passing it to the 
> AndroidVideoI420Buffer ctor. The rest of the code can go inside the ctor.

Done.

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
File webrtc/sdk/android/src/jni/native_handle_impl.h (left):

https://codereview.webrtc.org/2991633002/diff/80001/webrtc/sdk/android/src/jn...
webrtc/sdk/android/src/jni/native_handle_impl.h:131: // Holds a
VideoFrame.Buffer.
On 2017/07/28 09:45:05, magjed_webrtc wrote:
> Keep this comment.

Done.

[magjed_webrtc, 2017-08-01 12:31:42]

lgtm

https://codereview.webrtc.org/2991633002/diff/120001/webrtc/sdk/android/src/j...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (right):

https://codereview.webrtc.org/2991633002/diff/120001/webrtc/sdk/android/src/j...
webrtc/sdk/android/src/jni/native_handle_impl.cc:328:
jni->CallObjectMethod(*j_video_frame_buffer_, j_to_i420_id);
I'm still worried about the asymmetry between where retain and release happens.
Move this into the ctor and comment that the retain happens in Java code.

[sakal, 2017-08-09 11:07:29]

https://codereview.webrtc.org/2991633002/diff/120001/webrtc/sdk/android/src/j...
File webrtc/sdk/android/src/jni/native_handle_impl.cc (right):

https://codereview.webrtc.org/2991633002/diff/120001/webrtc/sdk/android/src/j...
webrtc/sdk/android/src/jni/native_handle_impl.cc:328:
jni->CallObjectMethod(*j_video_frame_buffer_, j_to_i420_id);
On 2017/08/01 12:31:42, magjed_webrtc wrote:
> I'm still worried about the asymmetry between where retain and release
happens.
> Move this into the ctor and comment that the retain happens in Java code.

I don't like having this in the constructor because it unnecessarily limits
where the frame can come from. I changed this to be a static method
AndroidVideoI420Buffer::WrapReference and added comments.

[sakal, 2017-08-09 11:08:07]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2017-08-09 11:08:07]

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/2991633002/#ps180001 (title:
"Add comments.")

[commit-bot: I haz the power, 2017-08-09 11:08:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-09 12:25:50]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1502276887173390,
"parent_rev": "e9a40732f5c4da317145d4f12fc526a99ca2ff42", "commit_rev":
"5ca60cc91cb21d4eded8b7ee401ee0b6cf280a2c"}

[commit-bot: I haz the power, 2017-08-09 12:25:58]

Description was changed from

==========
Implement AndroidVideoBuffer::ToI420.

BUG=webrtc:7749, webrtc:7760
==========

to

==========
Implement AndroidVideoBuffer::ToI420.

BUG=webrtc:7749, webrtc:7760

Review-Url: https://codereview.webrtc.org/2991633002
Cr-Commit-Position: refs/heads/master@{#19284}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5ca60cc91cb21d4eded8b7ee4...
==========

[commit-bot: I haz the power, 2017-08-09 12:25:59]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/external/webrtc/+/5ca60cc91cb21d4eded8b7ee4...