Android VideoCapturer: Send ByteBuffer instead of byte[]

Android VideoCapturer: Send ByteBuffer instead of byte[]

The purpose with this CL is to replace GetByteArrayElements() and ReleaseByteArrayElements() with GetDirectBufferAddress().

R=hbos@webrtc.org

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

[magjed_webrtc, 2015-09-28 10:01:09]

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

[magjed_webrtc, 2015-09-28 10:01:19]

hbos - Please take a look.

[hbos, 2015-09-28 12:35:51]

lgtm

https://codereview.webrtc.org/1372813002/diff/1/talk/app/webrtc/androidtests/...
File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java
(right):

https://codereview.webrtc.org/1372813002/diff/1/talk/app/webrtc/androidtests/...
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:112:
frameSize = frame.capacity();
Should limit() be used instead of capacity()?
Or maybe not if we are doing JNI magic with GetDirectBufferCapacity...?

[magjed_webrtc, 2015-09-28 14:42:20]

Committed patchset #1 (id:1) manually as
cb3649b40b3fd6d5bbb0a92003b717e46ce90924 (presubmit successful).

[commit-bot: I haz the power, 2015-09-28 14:42:23]

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

[magjed_webrtc, 2015-09-28 14:42:42]

https://codereview.webrtc.org/1372813002/diff/1/talk/app/webrtc/androidtests/...
File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java
(right):

https://codereview.webrtc.org/1372813002/diff/1/talk/app/webrtc/androidtests/...
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:112:
frameSize = frame.capacity();
On 2015/09/28 12:35:51, hbos wrote:
> Should limit() be used instead of capacity()?
> Or maybe not if we are doing JNI magic with GetDirectBufferCapacity...?

I know for sure that capacity() is exactly like previous |length|. I'm not sure
if limit() is set at all. We just send a byte[] to the camera, so I don't think
the marker in the ByteBuffer is touched at all.

[hbos, 2015-09-28 14:50:55]

On 2015/09/28 14:42:42, magjed_webrtc wrote:
>
https://codereview.webrtc.org/1372813002/diff/1/talk/app/webrtc/androidtests/...
> File talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java
> (right):
> 
>
https://codereview.webrtc.org/1372813002/diff/1/talk/app/webrtc/androidtests/...
> talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:112:
> frameSize = frame.capacity();
> On 2015/09/28 12:35:51, hbos wrote:
> > Should limit() be used instead of capacity()?
> > Or maybe not if we are doing JNI magic with GetDirectBufferCapacity...?
> 
> I know for sure that capacity() is exactly like previous |length|. I'm not
sure
> if limit() is set at all. We just send a byte[] to the camera, so I don't
think
> the marker in the ByteBuffer is touched at all.

I believe you're only meant to use 0-limit bytes, where limit <= capacity.
(Analogous to the difference between "size" and "capacity" of a dynamic array.)
limit = capacity because nobody does silly things with our buffers so it works,
and this will probably continue to work.

You might have a situation though where a buffer is allocated of size x and then
frames change size or whatever and you only need y (y<x) bytes, and you end up
reusing the same buffers. This won't happen with the current code but you could
conceive of somebody setting the limit instead of re-allocating the buffer.

But then there is the JNI stuff too where it creates a byte buffer for you in
which case limit is probably capacity again?

I dunno, food for thought.

[hbos, 2015-09-28 14:52:57]

On 2015/09/28 14:50:55, hbos wrote:
> On 2015/09/28 14:42:42, magjed_webrtc wrote:
> >
>
https://codereview.webrtc.org/1372813002/diff/1/talk/app/webrtc/androidtests/...
> > File
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java
> > (right):
> > 
> >
>
https://codereview.webrtc.org/1372813002/diff/1/talk/app/webrtc/androidtests/...
> >
talk/app/webrtc/androidtests/src/org/webrtc/VideoCapturerAndroidTest.java:112:
> > frameSize = frame.capacity();
> > On 2015/09/28 12:35:51, hbos wrote:
> > > Should limit() be used instead of capacity()?
> > > Or maybe not if we are doing JNI magic with GetDirectBufferCapacity...?
> > 
> > I know for sure that capacity() is exactly like previous |length|. I'm not
> sure
> > if limit() is set at all. We just send a byte[] to the camera, so I don't
> think
> > the marker in the ByteBuffer is touched at all.
> 
> I believe you're only meant to use 0-limit bytes, where limit <= capacity.
> (Analogous to the difference between "size" and "capacity" of a dynamic
array.)
> limit = capacity because nobody does silly things with our buffers so it
works,
> and this will probably continue to work.
> 
> You might have a situation though where a buffer is allocated of size x and
then
> frames change size or whatever and you only need y (y<x) bytes, and you end up
> reusing the same buffers. This won't happen with the current code but you
could
> conceive of somebody setting the limit instead of re-allocating the buffer.
> 
> But then there is the JNI stuff too where it creates a byte buffer for you in
> which case limit is probably capacity again?
> 
> I dunno, food for thought.

We don't use limit though so you could just ignore this and say it doesn't
apply. I just started thinking about it when I saw the word "capacity()"...

[magjed_webrtc, 2015-09-29 08:13:32]

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

The reason for reverting is: The top row in the video stream from the camera is
messed up. The byte[] pointer is not the same as GetDirectBufferAddress()
apparently..