New method CreateScaledFrame in the VideoFrameFactory interface.

New method CreateScaledFrame in the VideoFrameFactory interface.

Intended to replace the CreateAliasedFrame methods. Implemented in
WebRtcVideoFrameFactory.

BUG=webrtc:5682

[nisse-webrtc, 2016-05-09 10:10:00]

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

[nisse-webrtc, 2016-05-09 10:10:01]

Please have a first look. Fails the test case
WebRtcVideoFrameFactoryTest.CreateScaledFrameApplyRotation

I think I have to implement cropping to make it work. But without any extra
arguments, just do a center crop if needed to preserve aspect ratio.

[magjed_webrtc, 2016-05-09 10:58:08]

https://codereview.webrtc.org/1960073002/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoframefactory.cc:17: // #include
"webrtc/common_video/include/video_frame_buffer.h"
remove if not used

https://codereview.webrtc.org/1960073002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoframefactory.cc:42: uint32_t format =
CanonicalFourCC(input_frame->fourcc);
Move this to the line above libyuv::ConvertToI420.

https://codereview.webrtc.org/1960073002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoframefactory.cc:47: // TODO(nisse): Do center
crop if needed to preserve aspect ratio.
I would like if you implemented this immediately. Maybe the name should be
changed as well? Also, if we want all implementations to center crop to preserve
aspect ratio, we should document it in the interface.

[nisse-webrtc, 2016-05-09 11:04:47]

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

[commit-bot: I haz the power, 2016-05-09 11:04:55]

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

[nisse-webrtc, 2016-05-09 11:06:01]

Done the simple things. Will try cropping next. Any idea on how to write a unit
test for that? Input frame containing some color gradient?

https://codereview.webrtc.org/1960073002/diff/1/webrtc/media/engine/webrtcvid...
File webrtc/media/engine/webrtcvideoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoframefactory.cc:17: // #include
"webrtc/common_video/include/video_frame_buffer.h"
On 2016/05/09 10:58:08, magjed_webrtc wrote:
> remove if not used

Done.

https://codereview.webrtc.org/1960073002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoframefactory.cc:42: uint32_t format =
CanonicalFourCC(input_frame->fourcc);
On 2016/05/09 10:58:08, magjed_webrtc wrote:
> Move this to the line above libyuv::ConvertToI420.

Done.

https://codereview.webrtc.org/1960073002/diff/1/webrtc/media/engine/webrtcvid...
webrtc/media/engine/webrtcvideoframefactory.cc:47: // TODO(nisse): Do center
crop if needed to preserve aspect ratio.
On 2016/05/09 10:58:08, magjed_webrtc wrote:
> I would like if you implemented this immediately. Maybe the name should be
> changed as well? Also, if we want all implementations to center crop to
preserve
> aspect ratio, we should document it in the interface.

Ok, I'll give it a try.

[commit-bot: I haz the power, 2016-05-09 11:48:45]

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

[commit-bot: I haz the power, 2016-05-09 11:48:46]

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

[nisse-webrtc, 2016-05-09 12:01:44]

Added cropping. Still missing a unit test.

[magjed_webrtc, 2016-05-09 13:44:55]

You should update AndroidVideoCapturer::FrameFactory in this CL as well.

https://codereview.webrtc.org/1960073002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframefactory.cc:38: if (aspect_diff > 0) {
This code will be the same for all implementations because the cropped
resolution can be calculated from the input frame resolution and
dst_width/height. How about making this crop calculation in
VideoCapturer::OnFrameCaptured and passing the cropped resolution as argument to
this function instead?

[nisse-webrtc, 2016-05-10 07:56:23]

Added a test case, and fixed a typo breaking vertical cropping.

I'll look at AndroidVideoCapturer::FrameFactory next.

https://codereview.webrtc.org/1960073002/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframefactory.cc:38: if (aspect_diff > 0) {
On 2016/05/09 13:44:55, magjed_webrtc wrote:
> This code will be the same for all implementations because the cropped
> resolution can be calculated from the input frame resolution and
> dst_width/height. How about making this crop calculation in
> VideoCapturer::OnFrameCaptured and passing the cropped resolution as argument
to
> this function instead?

It used to be a helper function in videocommon. I think the responsibility
belongs to the FrameFactory, but I agree we shouldn't duplicate it. 

It could be a helper method (possibly static) in the VideoFrameFactory base
class.

Or split into a (protected?) method (say, CreateScaledCroppedFrame) to be
implemented in subclasses, and the public CreateScaledFrame implemented in the
base class.

What subclasses of VideoFrameFactory exists? E.g., is the cropping logic
reusable for texture frames?

I'm also considering moving the libyuv calls (ConvertToI420 and I420Scale) to
the I420Buffer class.

[nisse-webrtc, 2016-05-11 06:56:33]

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

[commit-bot: I haz the power, 2016-05-11 06:59:43]

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

[commit-bot: I haz the power, 2016-05-11 07:10:53]

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

[commit-bot: I haz the power, 2016-05-11 07:10:54]

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

[perkj_webrtc, 2016-05-11 10:05:17]

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/common_video/incl...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame_buffer.h:134: int crop_width,
please document what the arguments means.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/common_video/vide...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/common_video/vide...
webrtc/common_video/video_frame_buffer.cc:408: // TODO(nisse): No longer used in
webrtc.
Remove? Or update the todo to say that it should be removed when fixed in chrome
or ....

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videoc...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videoc...
webrtc/media/base/videocapturer.cc:241: // TODO(nisse): Reorganize frame factory
methods, deleting crop
remove this todo?

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
webrtc/media/base/videoframefactory.cc:19: // TODO(nisse): Delete when Chrome's
subclass is updated.
Is this really the method you should delete?

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
File webrtc/media/base/videoframefactory.h (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
webrtc/media/base/videoframefactory.h:56: return NULL;
return nullptr;

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
webrtc/media/base/videoframefactory.h:77: int crop_width,
please document what the arguments mean.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframefactory.cc:86: if
(!frame->Init(aliased_frame, width, height, apply_rotation_)) {
It looks like |aliased_frame| always is I420? No need to ConvertToI420 above
then.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframefactory.h (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframefactory.h:32: VideoFrame*
CreateAliasedFrame(const CapturedFrame* aliased_frame,
Can you remove CreateAliasedFrame here an only use the new method?

https://codereview.webrtc.org/1960073002/diff/140001/webrtc/common_video/vide...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1960073002/diff/140001/webrtc/common_video/vide...
webrtc/common_video/video_frame_buffer.cc:211: rtc::scoped_refptr<I420Buffer>
I420Buffer::CropAndScale(
Add unit test for this method please.

[nisse-webrtc, 2016-05-12 12:23:07]

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/common_video/incl...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame_buffer.h:134: int crop_width,
On 2016/05/11 10:05:17, perkj_webrtc wrote:
> please document what the arguments means.

Done.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/common_video/vide...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/common_video/vide...
webrtc/common_video/video_frame_buffer.cc:408: // TODO(nisse): No longer used in
webrtc.
On 2016/05/11 10:05:17, perkj_webrtc wrote:
> Remove? Or update the todo to say that it should be removed when fixed in
chrome
> or .... 

I'm updating the comment. It is actually used, in
AndroidVideoCapturer::FrameFactory::CreateAliasedFrame. And I don't want to
delete the CreateAliasedFrame methods in this cl, since chrome overrides them.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videoc...
File webrtc/media/base/videocapturer.cc (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videoc...
webrtc/media/base/videocapturer.cc:241: // TODO(nisse): Reorganize frame factory
methods, deleting crop
On 2016/05/11 10:05:17, perkj_webrtc wrote:
> remove this todo? 

Done.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
webrtc/media/base/videoframefactory.cc:19: // TODO(nisse): Delete when Chrome's
subclass is updated.
On 2016/05/11 10:05:17, perkj_webrtc wrote:
> Is this really the method you should delete?

It's not. Good catch.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
File webrtc/media/base/videoframefactory.h (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/base/videof...
webrtc/media/base/videoframefactory.h:77: int crop_width,
On 2016/05/11 10:05:17, perkj_webrtc wrote:
> please document what the arguments mean.

I'm adding a comment. I'm also renaming the method, for consistency with
I420Buffer::CropAndScale.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframefactory.cc:48: uint32_t format =
CanonicalFourCC(input_frame->fourcc);
But calling CanonicalFourCC seems unnecessary, that's the first thing done by
ConvertToI420 (but with a different incarnation of that function, I guess).

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframefactory.cc:86: if
(!frame->Init(aliased_frame, width, height, apply_rotation_)) {
On 2016/05/11 10:05:17, perkj_webrtc wrote:
> It looks like |aliased_frame| always is I420? No need to ConvertToI420 above
> then. 

In the (common) case that the CapturedFrame holds I420 data, the ConvertToI420
call boils down to a call to I420Rotate, which in turn boils down to I420Copy.
And we do want a copy.

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframefactory.h (right):

https://codereview.webrtc.org/1960073002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframefactory.h:32: VideoFrame*
CreateAliasedFrame(const CapturedFrame* aliased_frame,
On 2016/05/11 10:05:17, perkj_webrtc wrote:
> Can you remove CreateAliasedFrame here an only use the new method?

Not at the moment. Chrome's WebRtcVideoCapturerAdapter::MediaVideoFrameFactory
overrides it, both the 3-argument and 5-argument flavors.

https://codereview.webrtc.org/1960073002/diff/140001/webrtc/common_video/vide...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1960073002/diff/140001/webrtc/common_video/vide...
webrtc/common_video/video_frame_buffer.cc:211: rtc::scoped_refptr<I420Buffer>
I420Buffer::CropAndScale(
On 2016/05/11 10:05:17, perkj_webrtc wrote:
> Add unit test for this method please. 

Ok. Something similar to the corresponding WebRtcVideoFrameFactoryTest test
should do.

[perkj_webrtc, 2016-05-16 06:48:47]

https://codereview.webrtc.org/1960073002/diff/140001/webrtc/common_video/vide...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1960073002/diff/140001/webrtc/common_video/vide...
webrtc/common_video/video_frame_buffer.cc:211: rtc::scoped_refptr<I420Buffer>
I420Buffer::CropAndScale(
On 2016/05/12 12:23:07, nisse-webrtc wrote:
> On 2016/05/11 10:05:17, perkj_webrtc wrote:
> > Add unit test for this method please. 
> 
> Ok. Something similar to the corresponding WebRtcVideoFrameFactoryTest test
> should do.

Test still missing in webrtcvideoframe_unittests and/or videoframe_unittest.h

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/api/androidvideoc...
File webrtc/api/androidvideocapturer.cc (right):

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/api/androidvideoc...
webrtc/api/androidvideocapturer.cc:77: // TODO(nisse): Must handle texture
frames too.
I think you have to do this now. CreateAliasedFrame handles textures but since
you override CreateCroppedAndScaledFrame it will not be called?

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/api/androidvideoc...
webrtc/api/androidvideocapturer.cc:99:
frame->GetCopyWithRotationApplied()->Copy())
nit: Should there be a TODO here to fix GetCopyWithRotationApplied()->Copy()) ?
ie- should GetCopyWithRotationApplied() return a new I420Buffer  so you don't
need to call Copy? The capturer  could potentially cash the rotated buffer if it
has multiple sinks. 

(or we we just pretend that we have not seen this funny thing at the
moment.....)

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/common_video/incl...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame_buffer.h:131: // lower left corner at
|crop_x|, |crop_y|, and returns a new frame
lower left? -really ? isn't this upper left?

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/common_video/vide...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/common_video/vide...
webrtc/common_video/video_frame_buffer.cc:228: // TODO(nisse): Duplicated in
ShallowCrop.
remove this todo. I am fine with this duplication.

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/media/base/videof...
File webrtc/media/base/videoframefactory.cc (right):

https://codereview.webrtc.org/1960073002/diff/160001/webrtc/media/base/videof...
webrtc/media/base/videoframefactory.cc:32: // TODO(nisse): Old code carried a
comment saying that MJPG can
You can add a DCHECK here if you want to check that input_frame is not MJPG. But
we don't support MJPG at this level anymore anyway, MGJP has already been
supported to I420 before reaching libjingle.

[nisse-webrtc, 2016-06-09 11:17:36]

Description was changed from

==========
New method CreateScaledFrame in the VideoFrameFactory interface.

Intended to replace the CreateAliasedFrame methods. Implemented in
WebRtcVideoFrameFactory.

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

to

==========
New method CreateScaledFrame in the VideoFrameFactory interface.

Intended to replace the CreateAliasedFrame methods. Implemented in
WebRtcVideoFrameFactory.

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

[nisse-webrtc, 2016-06-09 11:17:36]

nisse@webrtc.org changed reviewers:
- magjed@webrtc.org, perkj@webrtc.org