Delete AndroidVideoCapturer::FrameFactory.

Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

Refactors AndroidVideoCapturer to make adaptation decision
earlier, so we can crop and rotate using
NV12ToI420Rotate.

BUG=webrtc:5682
Committed: https://crrev.com/47ac4620c810fa816837f9b83b8392f8c29cf076
Cr-Commit-Position: refs/heads/master@{#12895}

[nisse-webrtc, 2016-05-12 13:58:51]

nisse@webrtc.org changed reviewers:
+ magjed@google.com

[nisse-webrtc, 2016-05-12 13:58:51]

Please have a first look. I haven't tried to get cropping or aspect ratio right,
and intend to get back to that after https://codereview.webrtc.org/1965953003/
is landed.

[nisse-webrtc, 2016-05-16 09:29:00]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 09:29:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973873003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973873003/40001

[nisse-webrtc, 2016-05-16 09:33:24]

Updated after your (magjed's, that is) android cropping changes were landed.

I've also change the AdaptFrameResolution to return a bool (false to drop the
frame).

I didn't add crop_x and crop_y arguments to the VideoAdapter, The decision to do
a *center* crop now remains with the VideoCapturer. Not sure what makes most
sense.

[nisse-webrtc, 2016-05-16 09:44:16]

https://codereview.webrtc.org/1973873003/diff/40001/webrtc/common_video/video...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1973873003/diff/40001/webrtc/common_video/video...
webrtc/common_video/video_frame_buffer.cc:211: rtc::scoped_refptr<I420Buffer>
I420Buffer::CropAndScale(
And I realize this methods still needs a unittest.

[commit-bot: I haz the power, 2016-05-16 10:50:14]

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

[commit-bot: I haz the power, 2016-05-16 10:50:15]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)

[nisse-webrtc, 2016-05-16 12:23:17]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 12:23:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973873003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973873003/60001

[commit-bot: I haz the power, 2016-05-16 13:01:34]

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

[commit-bot: I haz the power, 2016-05-16 13:01:35]

Dry run: Try jobs failed on following builders:
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/12190)

[nisse-webrtc, 2016-05-16 13:08:21]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-16 13:08:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973873003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1973873003/60001

[commit-bot: I haz the power, 2016-05-16 14:24:12]

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

[commit-bot: I haz the power, 2016-05-16 14:24:13]

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

[nisse-webrtc, 2016-05-17 10:29:54]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-17 10:29:56]

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

[nisse-webrtc, 2016-05-17 10:31:33]

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

[nisse-webrtc, 2016-05-17 10:31:33]

+ perkj, please have a look.

[nisse-webrtc, 2016-05-17 11:12:30]

Description was changed from

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

BUG=webrtc:5682
==========

to

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

BUG=webrtc:5682
==========

[nisse-webrtc, 2016-05-17 11:12:30]

nisse@webrtc.org changed reviewers:
+ magjed@webrtc.org
- magjed@google.com

[commit-bot: I haz the power, 2016-05-17 11:42:48]

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

[commit-bot: I haz the power, 2016-05-17 11:42:49]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2016-05-17 12:12:40]

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
File webrtc/api/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:83: void
AndroidVideoCapturer::OnIncomingFrame(
Add two methods OnIncomingTexture and OnIncomingNV21 instead, and delegate from
androidvideocapturer_jni. You can use NV12ToI420Rotate directly instead of
rotating with GetCopyWithRotationApplied. Also, try to move the function
CropScaleAndRotate from AndroidTextureBuffer to NativeHandle instead.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/i420_...
File webrtc/common_video/i420_video_frame_unittest.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/i420_...
webrtc/common_video/i420_video_frame_unittest.cc:34: for (int x = 0; x < width;
x++)
nit: Add {}

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/i420_...
webrtc/common_video/i420_video_frame_unittest.cc:41: for (int x = 0; x <
chroma_width; x++)
nit: Add {}

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/video...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/video...
webrtc/common_video/video_frame_buffer.cc:221: RTC_CHECK_LE(crop_width +
offset_x, buffer->width());
Add:
RTC_CHECK_GE(offset_x, 0);
RTC_CHECK_GE(offset_y, 0);
for completeness.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.cc:231: width, height, crop_width, crop_height,
out_width, out_height))
nit: Add {}

[magjed_webrtc, 2016-05-17 12:19:12]

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
File webrtc/api/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:127: adapted =
I420Buffer::CropAndScale(buffer, crop_x, crop_y,
Use a frame pool instead of allocating new memory every time.

[nisse-webrtc, 2016-05-18 11:13:21]

Description was changed from

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

BUG=webrtc:5682
==========

to

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

Refactors AndroidVideoCapturer to make adaptation decision
earlier, so we can crop and rotate using 
NV12ToI420Rotate. 

In addition, eliminates a thread hop on the frame input path, which should no
longer be needed.


BUG=webrtc:5682
==========

[nisse-webrtc, 2016-05-18 11:16:34]

Reorganized as suggested. Now passes some initial tests, using
run_AppRtcDemoTest.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
File webrtc/api/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:83: void
AndroidVideoCapturer::OnIncomingFrame(
On 2016/05/17 12:12:39, magjed_webrtc wrote:
> Add two methods OnIncomingTexture and OnIncomingNV21 instead, and delegate
from
> androidvideocapturer_jni. You can use NV12ToI420Rotate directly instead of
> rotating with GetCopyWithRotationApplied. Also, try to move the function
> CropScaleAndRotate from AndroidTextureBuffer to NativeHandle instead.

Done. (But named OnNV21Frame and OnTextureFrame).

Actually, I think we can choose whether to do this adaptation in
AndroidVideoCapturer or AndroidVideoCApturerJni, it uses only public
VideoCapturer methods, I think.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:127: adapted =
I420Buffer::CropAndScale(buffer, crop_x, crop_y,
On 2016/05/17 12:19:12, magjed_webrtc wrote:
> Use a frame pool instead of allocating new memory every time.

Haven't done this yet. Would the pool be an additional argument? I'd like the
CropAndScale method to return an I420Buffer, but that doesn't quite work with
the current frame pool.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/i420_...
File webrtc/common_video/i420_video_frame_unittest.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/i420_...
webrtc/common_video/i420_video_frame_unittest.cc:34: for (int x = 0; x < width;
x++)
On 2016/05/17 12:12:40, magjed_webrtc wrote:
> nit: Add {}

Done.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/i420_...
webrtc/common_video/i420_video_frame_unittest.cc:41: for (int x = 0; x <
chroma_width; x++)
On 2016/05/17 12:12:40, magjed_webrtc wrote:
> nit: Add {}

Done.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/video...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/common_video/video...
webrtc/common_video/video_frame_buffer.cc:221: RTC_CHECK_LE(crop_width +
offset_x, buffer->width());
On 2016/05/17 12:12:40, magjed_webrtc wrote:
> Add:
> RTC_CHECK_GE(offset_x, 0);
> RTC_CHECK_GE(offset_y, 0);
> for completeness.

Done.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/media/base/videoca...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/media/base/videoca...
webrtc/media/base/videocapturer.cc:231: width, height, crop_width, crop_height,
out_width, out_height))
On 2016/05/17 12:12:40, magjed_webrtc wrote:
> nit: Add {}

Done.

[magjed_webrtc, 2016-05-18 11:41:59]

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
File webrtc/api/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:83: void
AndroidVideoCapturer::OnIncomingFrame(
On 2016/05/18 11:16:33, nisse-webrtc wrote:
> On 2016/05/17 12:12:39, magjed_webrtc wrote:
> > Add two methods OnIncomingTexture and OnIncomingNV21 instead, and delegate
> from
> > androidvideocapturer_jni. You can use NV12ToI420Rotate directly instead of
> > rotating with GetCopyWithRotationApplied. Also, try to move the function
> > CropScaleAndRotate from AndroidTextureBuffer to NativeHandle instead.
> 
> Done. (But named OnNV21Frame and OnTextureFrame).
> 
> Actually, I think we can choose whether to do this adaptation in
> AndroidVideoCapturer or AndroidVideoCApturerJni, it uses only public
> VideoCapturer methods, I think.

If you can choose, I prefer to keep the conversion here in
AndroidVideoCapturerJni and only use public methods on VideoCapturer.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:127: adapted =
I420Buffer::CropAndScale(buffer, crop_x, crop_y,
On 2016/05/18 11:16:33, nisse-webrtc wrote:
> On 2016/05/17 12:19:12, magjed_webrtc wrote:
> > Use a frame pool instead of allocating new memory every time.
> 
> Haven't done this yet. Would the pool be an additional argument? I'd like the
> CropAndScale method to return an I420Buffer, but that doesn't quite work with
> the current frame pool.

I think it's best if the output I420Buffer is an additional argument instead of
the pool, because I don't think I420Buffer should depend on any specific pool
implementation.

https://codereview.webrtc.org/1973873003/diff/120001/webrtc/api/java/jni/nati...
File webrtc/api/java/jni/native_handle_impl.h (right):

https://codereview.webrtc.org/1973873003/diff/120001/webrtc/api/java/jni/nati...
webrtc/api/java/jni/native_handle_impl.h:25: struct Matrix {
struct's are not allowed to have complex methods. So either make Matrix a class,
or make the methods static like Rotate(const float[16] in, float[16] out);

https://codereview.webrtc.org/1973873003/diff/120001/webrtc/api/java/jni/nati...
webrtc/api/java/jni/native_handle_impl.h:46: NativeHandleImpl Crop(
Do we really need Crop and Rotate for NativeHandleImpl?

[nisse-webrtc, 2016-05-18 12:17:52]

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
File webrtc/api/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:83: void
AndroidVideoCapturer::OnIncomingFrame(
On 2016/05/18 11:41:59, magjed_webrtc wrote:
> On 2016/05/18 11:16:33, nisse-webrtc wrote:
> > On 2016/05/17 12:12:39, magjed_webrtc wrote:
> > > Add two methods OnIncomingTexture and OnIncomingNV21 instead, and delegate
> > from
> > > androidvideocapturer_jni. You can use NV12ToI420Rotate directly instead of
> > > rotating with GetCopyWithRotationApplied. Also, try to move the function
> > > CropScaleAndRotate from AndroidTextureBuffer to NativeHandle instead.
> > 
> > Done. (But named OnNV21Frame and OnTextureFrame).
> > 
> > Actually, I think we can choose whether to do this adaptation in
> > AndroidVideoCapturer or AndroidVideoCApturerJni, it uses only public
> > VideoCapturer methods, I think.
> 
> If you can choose, I prefer to keep the conversion here in
> AndroidVideoCapturerJni and only use public methods on VideoCapturer.

I'll give it a try.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:127: adapted =
I420Buffer::CropAndScale(buffer, crop_x, crop_y,
On 2016/05/18 11:41:59, magjed_webrtc wrote:
> On 2016/05/18 11:16:33, nisse-webrtc wrote:
> > On 2016/05/17 12:19:12, magjed_webrtc wrote:
> > > Use a frame pool instead of allocating new memory every time.
> > 
> > Haven't done this yet. Would the pool be an additional argument? I'd like
the
> > CropAndScale method to return an I420Buffer, but that doesn't quite work
with
> > the current frame pool.
> 
> I think it's best if the output I420Buffer is an additional argument instead
of
> the pool, because I don't think I420Buffer should depend on any specific pool
> implementation.

I really dislike destination arguments accessed via MutableDataX()...

I think we should strive to have code modifying pixel data limited to
I420Buffers which the modifying code owns, with the special case of newly
constructed frame buffers. Eventually, I'd like to remove MutableData from the
VideoFrameBuffer base class.

Have to think a bit about the options here.

BTW, I guess we should try to also reuse the buffer used for the unscaled data?

https://codereview.webrtc.org/1973873003/diff/120001/webrtc/api/java/jni/nati...
File webrtc/api/java/jni/native_handle_impl.h (right):

https://codereview.webrtc.org/1973873003/diff/120001/webrtc/api/java/jni/nati...
webrtc/api/java/jni/native_handle_impl.h:25: struct Matrix {
On 2016/05/18 11:41:59, magjed_webrtc wrote:
> struct's are not allowed to have complex methods. So either make Matrix a
class,
> or make the methods static like Rotate(const float[16] in, float[16] out);

I'll make it a class, then. Can it still use the name NativeHandleImpl::Matrix,
or webrtc_jni::Matrix, or something else?

https://codereview.webrtc.org/1973873003/diff/120001/webrtc/api/java/jni/nati...
webrtc/api/java/jni/native_handle_impl.h:46: NativeHandleImpl Crop(
On 2016/05/18 11:41:59, magjed_webrtc wrote:
> Do we really need Crop and Rotate for NativeHandleImpl?

No, these are leftovers and should be deleted.

[nisse-webrtc, 2016-05-18 13:54:58]

New version, with the adaptiation code moved to AndroidVideoCapturerJni.

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
File webrtc/api/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1973873003/diff/80001/webrtc/api/androidvideoca...
webrtc/api/androidvideocapturer.cc:127: adapted =
I420Buffer::CropAndScale(buffer, crop_x, crop_y,
On 2016/05/18 12:17:52, nisse-webrtc wrote:
> On 2016/05/18 11:41:59, magjed_webrtc wrote:
> > On 2016/05/18 11:16:33, nisse-webrtc wrote:
> > > On 2016/05/17 12:19:12, magjed_webrtc wrote:
> > > > Use a frame pool instead of allocating new memory every time.
> > > 
> > > Haven't done this yet. Would the pool be an additional argument? I'd like
> the
> > > CropAndScale method to return an I420Buffer, but that doesn't quite work
> with
> > > the current frame pool.
> > 
> > I think it's best if the output I420Buffer is an additional argument instead
> of
> > the pool, because I don't think I420Buffer should depend on any specific
pool
> > implementation.
> 
> I really dislike destination arguments accessed via MutableDataX()...
> 
> I think we should strive to have code modifying pixel data limited to
> I420Buffers which the modifying code owns, with the special case of newly
> constructed frame buffers. Eventually, I'd like to remove MutableData from the
> VideoFrameBuffer base class.
> 
> Have to think a bit about the options here.
> 
> BTW, I guess we should try to also reuse the buffer used for the unscaled
data?

I now use buffer_pool_ for the unscaled frame, similar to the old code. I guess
we'd need a separate pool for the scaled frame?

[magjed_webrtc, 2016-05-18 15:45:30]

https://codereview.webrtc.org/1973873003/diff/140001/webrtc/api/java/jni/andr...
File webrtc/api/java/jni/androidvideocapturer_jni.cc (right):

https://codereview.webrtc.org/1973873003/diff/140001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidvideocapturer_jni.cc:203: crop_x &= ~1;
The same is true for crop_y.

https://codereview.webrtc.org/1973873003/diff/140001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidvideocapturer_jni.cc:207: uv_plane + width * crop_y +
crop_x, width,
crop_y is not used correctly, it should be roughly half that.
uv_stride = (width + 1) / 2 * 2;
uv_plane + uv_stride * (crop_y / 2) + crop_x

https://codereview.webrtc.org/1973873003/diff/140001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidvideocapturer_jni.cc:216: // TODO(nisse): Use
buffer_pool_ here?
Yes, use a buffer pool here.

[nisse-webrtc, 2016-05-18 16:34:17]

https://codereview.webrtc.org/1973873003/diff/140001/webrtc/api/java/jni/andr...
File webrtc/api/java/jni/androidvideocapturer_jni.cc (right):

https://codereview.webrtc.org/1973873003/diff/140001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidvideocapturer_jni.cc:203: crop_x &= ~1;
On 2016/05/18 15:45:30, magjed_webrtc wrote:
> The same is true for crop_y.

Done.

https://codereview.webrtc.org/1973873003/diff/140001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidvideocapturer_jni.cc:207: uv_plane + width * crop_y +
crop_x, width,
On 2016/05/18 15:45:30, magjed_webrtc wrote:
> crop_y is not used correctly, it should be roughly half that.
> uv_stride = (width + 1) / 2 * 2;
> uv_plane + uv_stride * (crop_y / 2) + crop_x

If crop_y is forced to be even (fixed above), then it should be correct with

uv_width = (width + 1) / 2;
uv_plane + uv_width * crop_y + crop_x

Right?

https://codereview.webrtc.org/1973873003/diff/140001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidvideocapturer_jni.cc:216: // TODO(nisse): Use
buffer_pool_ here?
On 2016/05/18 15:45:30, magjed_webrtc wrote:
> Yes, use a buffer pool here.

Two buffer pools it is, then. But it would be nice to land
https://codereview.webrtc.org/1988183002/, so that the pool can return an
I420Buffer. Which can reasonably have a scaling method.

Done.

[nisse-webrtc, 2016-05-19 08:02:04]

On 2016/05/18 16:34:17, nisse-webrtc wrote:
> Two buffer pools it is, then. But it would be nice to land
> https://codereview.webrtc.org/1988183002/, so that the pool can return an
> I420Buffer. Which can reasonably have a scaling method.
> 
> Done.

I just realized this makes the static CropAndScale method unused. 
Should be deleted before landing, if no other uses turns up.

[nisse-webrtc, 2016-05-23 08:49:14]

Description was changed from

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

Refactors AndroidVideoCapturer to make adaptation decision
earlier, so we can crop and rotate using 
NV12ToI420Rotate. 

In addition, eliminates a thread hop on the frame input path, which should no
longer be needed.


BUG=webrtc:5682
==========

to

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

Refactors AndroidVideoCapturer to make adaptation decision
earlier, so we can crop and rotate using 
NV12ToI420Rotate. 

BUG=webrtc:5682
==========

[nisse-webrtc, 2016-05-23 08:49:43]

Please have another look now.

[nisse-webrtc, 2016-05-23 13:50:17]

On 2016/05/23 08:49:43, nisse-webrtc wrote:
> Please have another look now.

The crash on patch set 12 looks as follows:

bfd06:  05-23 12:24:18.085   203 26447 I mm-camera: isp_pipeline_util_reset:
module id = 22, action = 0xb6e677ed
bfd06:  05-23 12:24:18.088   203 26415 I mm-camera: ispif_streamoff: session_id
= 2, active_streams = 0
bfd06:  05-23 12:24:18.088   203 26415 I mm-camera-sensor:
module_sensor_module_process_event:2159 ide 20002 MCT_EVENT_CONTROL_STREAMOFF
bfd06:  05-23 12:24:18.090   203 26415 I mm-camera: mct_pipeline_process_set:
Stream on/off returned
bfd06:  05-23 12:24:22.831   308 26423 E QCamera3HWI: int
qcamera::QCamera3HardwareInterface::processCaptureRequest(camera3_capture_request_t*):
Unblocked on timeout!!!!
bfd06:  05-23 12:24:22.831   308 26423 E Camera3-Device: Camera 1: threadLoop:
RequestThread: Unable to submit capture request 5 to HAL device: No such device
(-19)
bfd06:  05-23 12:24:22.831   308 26423 E Camera2ClientBase: Error condition 1
reported by HAL, requestId -1
bfd06:  05-23 12:24:23.067   308 26423 D CameraTraces: Process trace saved. Use
dumpsys media.camera to view.
bfd06:  05-23 12:24:24.831 26248 26269 E org.webrtc.Logging:
VideoCapturerAndroid: Camera stop timeout
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid: VideoCapturerAndroid stacks trace:
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid: android.hardware.Camera._stopPreview(Native Method)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid: android.hardware.Camera.stopPreview(Camera.java:732)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid:
org.webrtc.VideoCapturerAndroid.stopCaptureOnCameraThread(VideoCapturerAndroid.java:543)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid:
org.webrtc.VideoCapturerAndroid.access$900(VideoCapturerAndroid.java:43)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid:
org.webrtc.VideoCapturerAndroid$7.run(VideoCapturerAndroid.java:503)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid: android.os.Handler.handleCallback(Handler.java:739)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid: android.os.Handler.dispatchMessage(Handler.java:95)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid: android.os.Looper.loop(Looper.java:135)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid: android.os.HandlerThread.run(HandlerThread.java:61)
bfd06:  05-23 12:24:24.837 26248 26269 I org.webrtc.Logging:
VideoCapturerAndroid: stopCapture done
bfd06:  05-23 12:24:24.837 26248 26270 I org.webrtc.Logging:
VideoCapturerAndroid: release
bfd06:  05-23 12:24:24.838 26248 26270 W System.err:
java.lang.IllegalStateException: dispose() called while camera is running
bfd06:  05-23 12:24:24.838 26248 26270 W System.err: 	at
org.webrtc.VideoCapturerAndroid.dispose(VideoCapturerAndroid.java:273)
bfd06:  05-23 12:24:24.838 26248 26270 E rtc     : 
bfd06:  05-23 12:24:24.838 26248 26270 E rtc     : 
bfd06:  05-23 12:24:24.838 26248 26270 E rtc     : #
bfd06:  05-23 12:24:24.838 26248 26270 E rtc     : # Fatal error in
../../webrtc/api/java/jni/androidvideocapturer_jni.cc, line 55
bfd06:  05-23 12:24:24.838 26248 26270 E rtc     : # last system error: 0
bfd06:  05-23 12:24:24.838 26248 26270 E rtc     : # Check failed:
!jni()->ExceptionCheck()
bfd06:  05-23 12:24:24.838 26248 26270 E rtc     : # error during
VideoCapturer.dispose()
bfd06:  05-23 12:24:24.838 26248 26270 E rtc     : #
bfd06:  --------- beginning of crash
bfd06:  05-23 12:24:24.838 26248 26270 F libc    : Fatal signal 6 (SIGABRT),
code -6 in tid 26270 (signaling_threa)

Stack Trace:
  RELADDR   FUNCTION                                                            
               FILE:LINE
  0003a4d0  tgkill+12                                                           
               /system/lib/libc.so
  000173c1  pthread_kill+52                                                     
               /system/lib/libc.so
  00017fd3  raise+10                                                            
               /system/lib/libc.so
  00014795  __libc_android_abort+36                                             
               /system/lib/libc.so
  00012f44  abort+4                                                             
               /system/lib/libc.so
  000a448f  rtc::FatalMessage::~FatalMessage()+210                              
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  0007051d  webrtc_jni::AndroidVideoCapturerJni::~AndroidVideoCapturerJni()+504 
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00075f11 
rtc::RefCountedObject<webrtc_jni::AndroidVideoCapturerJni>::~RefCountedObject()+12
 /data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00075f21 
rtc::RefCountedObject<webrtc_jni::AndroidVideoCapturerJni>::~RefCountedObject()+4
  /data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00076045 
rtc::RefCountedObject<webrtc_jni::AndroidVideoCapturerJni>::Release() const+38  
   /data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00066729  webrtc::AndroidVideoCapturer::~AndroidVideoCapturer()+160           
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00066771  webrtc::AndroidVideoCapturer::~AndroidVideoCapturer()+4             
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00051881  webrtc::VideoCapturerTrackSource::~VideoCapturerTrackSource()+68    
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  000518c5 
rtc::RefCountedObject<webrtc::VideoCapturerTrackSource>::~RefCountedObject()+36 
   /data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  000518f5 
rtc::RefCountedObject<webrtc::VideoCapturerTrackSource>::~RefCountedObject()+4  
   /data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  000502a3  rtc::RefCountedObject<webrtc::VideoCapturerTrackSource>::Release()
const+38        
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00040ad9  webrtc::VideoTrackProxy::Release_s()+12                             
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00036903  webrtc::internal::SynchronousMethodCall::OnMessage(rtc::Message*)+12
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  002419d3  rtc::MessageQueue::Dispatch(rtc::Message*)+86                       
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  0024f06d  rtc::Thread::ProcessMessages(int)+88                                
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  0024f63d  rtc::Thread::PreRun(void*)+54                                       
              
/data/app/org.webrtc.test-1/lib/arm/libjingle_peerconnection_so.so
  00016baf  __pthread_start(void*)+30                                           
               /system/lib/libc.so
  00014af3  __start_thread+6                                                    
               /system/lib/libc.so

Java stderr from crashing pid (may identify underlying Java exception):
java.lang.IllegalStateException: dispose() called while camera is running
  at org.webrtc.VideoCapturerAndroid.dispose(VideoCapturerAndroid.java:273)

So the C++ AndroidVideoCapturer is destroyed for some reason, but leaving the
java camera thread running. 

Possibly triggered by some error from the camera driver. Not sure what to make
of this.

[magjed_webrtc, 2016-05-25 12:36:40]

lgtm

https://codereview.webrtc.org/1973873003/diff/180001/webrtc/api/java/jni/andr...
File webrtc/api/java/jni/androidvideocapturer_jni.cc (right):

https://codereview.webrtc.org/1973873003/diff/180001/webrtc/api/java/jni/andr...
webrtc/api/java/jni/androidvideocapturer_jni.cc:206: int uv_width = (width +
1)/2;
nit: (width + 1) / 2;

[nisse-webrtc, 2016-05-25 12:53:06]

nisse@webrtc.org changed reviewers:
+ tommi@webrtc.org

[nisse-webrtc, 2016-05-25 12:53:07]

Tommi, I need owner's approval for the changes to

    webrtc/api/androidvideocapturer.cc
    webrtc/api/androidvideocapturer.h

Can you have a look?

[tommi, 2016-05-25 14:22:33]

lgtm

[nisse-webrtc, 2016-05-25 14:24:30]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-05-25 14:24:30]

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/1973873003/#ps240001 (title:
"Address nit. Delete left-over include.")

[commit-bot: I haz the power, 2016-05-25 14:24:34]

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

[commit-bot: I haz the power, 2016-05-25 15:47:05]

Description was changed from

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

Refactors AndroidVideoCapturer to make adaptation decision
earlier, so we can crop and rotate using 
NV12ToI420Rotate. 

BUG=webrtc:5682
==========

to

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

Refactors AndroidVideoCapturer to make adaptation decision
earlier, so we can crop and rotate using 
NV12ToI420Rotate. 

BUG=webrtc:5682
==========

[commit-bot: I haz the power, 2016-05-25 15:47:07]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-25 15:47:08]

Description was changed from

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

Refactors AndroidVideoCapturer to make adaptation decision
earlier, so we can crop and rotate using 
NV12ToI420Rotate. 

BUG=webrtc:5682
==========

to

==========
Delete AndroidVideoCapturer::FrameFactory.

Splits VideoCapturer::OnFrameCaptured into helper methods,
which enables use of the VideoAdaptation logic without
using a frame factory.

Refactors AndroidVideoCapturer to make adaptation decision
earlier, so we can crop and rotate using
NV12ToI420Rotate.

BUG=webrtc:5682

Committed: https://crrev.com/47ac4620c810fa816837f9b83b8392f8c29cf076
Cr-Commit-Position: refs/heads/master@{#12895}
==========

[commit-bot: I haz the power, 2016-05-25 15:47:10]

Patchset 13 (id:??) landed as
https://crrev.com/47ac4620c810fa816837f9b83b8392f8c29cf076
Cr-Commit-Position: refs/heads/master@{#12895}