Move MutableDataY{,U,V} methods to I420Buffer only.

Move MutableDataY{,U,V} methods to I420Buffer only.

Deleted from the VideoFrameBuffer base class.

BUG=webrtc:5921
Committed: https://crrev.com/5539ef6c03c273f39fadae41ace47fdc11ac6d60
Cr-Commit-Position: refs/heads/master@{#14317}

[nisse-webrtc, 2016-08-25 08:30:32]

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

[nisse-webrtc, 2016-08-25 08:30:32]

Hi, this is a somewhat crude attempt to get rid of the
VideoFrameBuffer::MutableData* methods (moved to I420Buffer only).

It could be split in pieces, to make it easier and review and clean up. E.g,
more of the video_processing_unittest code should perhaps be reorganized to work
with VideoFrameBuffer rather than VideoFrame. Feel free to suggest pieces.

For methods taking VideoFrameBuffer and I420Buffer as arguments, I've used
rtc::scoped_refptr everywhere. Not sure if that's right, it makes proper use of
const harder.

kwiberg has suggested that rtc::scoped_refptr should be used only for methods
that does anything with object ownership (see his comments on
https://codereview.webrtc.org/2084893002/), which makes a lot of sense to me,
but it could be debated, I guess.

[nisse-webrtc, 2016-08-25 08:30:40]

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

[commit-bot: I haz the power, 2016-08-25 08:30:46]

Dry run: 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, 2016-08-25 08:36:55]

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

[commit-bot: I haz the power, 2016-08-25 08:36:56]

Dry run: Try jobs failed on following builders:
  ios_api_framework on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
  mac_gn_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/5588)

[kwiberg-webrtc, 2016-08-25 09:25:23]

On 2016/08/25 08:30:32, nisse-webrtc wrote:

> kwiberg has suggested that rtc::scoped_refptr should be used only for methods
> that does anything with object ownership (see his comments on
> https://codereview.webrtc.org/2084893002/), which makes a lot of sense to me,
> but it could be debated, I guess.

Herb Sutter explains the same thing here:
https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/.
The whole thing is worth reading, but the ones most relevant here are 3a, 3e,
and 3f.

(That article talks about std::shared_ptr, but rtc::scoped_refptr should be
treated just the same. The only relevant differences are that you can have more
than one reference count per object with shared_ptr, and that you can convert a
raw pointer to a scoped_refptr without creating a second reference count, but
since both of those things should never ever be done, there is no practical
difference in recommended usage.)

[magjed_webrtc, 2016-08-25 11:12:56]

> Herb Sutter explains the same thing here:
> https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/.
> The whole thing is worth reading, but the ones most relevant here are 3a, 3e,
> and 3f.
> 
> (That article talks about std::shared_ptr, but rtc::scoped_refptr should be
> treated just the same. The only relevant differences are that you can have
more
> than one reference count per object with shared_ptr, and that you can convert
a
> raw pointer to a scoped_refptr without creating a second reference count, but
> since both of those things should never ever be done, there is no practical
> difference in recommended usage.)

I would also prefer sending objects as const-ref to functions that are lifetime
policy agnostic, but neither WebRTC or Chromium does this, and I guess they have
some good reason? Is there any official discussion about this? I remember tommi@
had arguments for always using scoped_refptr.

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
File webrtc/common_video/libyuv/include/webrtc_libyuv.h (right):

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
webrtc/common_video/libyuv/include/webrtc_libyuv.h:105: const
rtc::scoped_refptr<I420Buffer>& dst_buffer);
If dst_buffer is an output argument, you need to use a pointer to a
scoped_refptr. (Or a pointer to an I420Buffer if we will allow that).

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
File webrtc/common_video/libyuv/libyuv_unittest.cc (right):

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
webrtc/common_video/libyuv/libyuv_unittest.cc:111: psnr = I420PSNR(&orig_frame_,
&res_i420_frame);
Can you use VideoFrameBuffers as input here instead?

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
webrtc/common_video/libyuv/libyuv_unittest.cc:115: {
The diff is kind of broken and becomes unnecessarily large because of these new
'{'. Can remove them again so it's easier to review?

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
webrtc/common_video/libyuv/libyuv_unittest.cc:239: if
(PrintVideoFrame(res_i420_frame, output_file) < 0) {
It would be nice if you can make functions like PrintVideoFrame work with
VideoFrameBuffers directly.

[nisse-webrtc, 2016-08-25 11:26:26]

On 2016/08/25 11:12:56, magjed_webrtc wrote:

> I would also prefer sending objects as const-ref to functions that are
lifetime
> policy agnostic, but neither WebRTC or Chromium does this, and I guess they
have
> some good reason? Is there any official discussion about this? I remember
tommi@
> had arguments for always using scoped_refptr.

I'd say we let tommi@ and kwiberg@ settle this, by any type of argument or
battle 
of their choosing. As soon as tommi is back.

[nisse-webrtc, 2016-08-25 11:31:35]

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
File webrtc/common_video/libyuv/include/webrtc_libyuv.h (right):

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
webrtc/common_video/libyuv/include/webrtc_libyuv.h:105: const
rtc::scoped_refptr<I420Buffer>& dst_buffer);
On 2016/08/25 11:12:56, magjed_webrtc wrote:
> If dst_buffer is an output argument, you need to use a pointer to a
> scoped_refptr. (Or a pointer to an I420Buffer if we will allow that).

I think the const is ok here; the scoped_refptr itself is not modified in any
way, but the object it points to have it's pixel buffers overwritten.

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
File webrtc/common_video/libyuv/libyuv_unittest.cc (right):

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
webrtc/common_video/libyuv/libyuv_unittest.cc:111: psnr = I420PSNR(&orig_frame_,
&res_i420_frame);
On 2016/08/25 11:12:56, magjed_webrtc wrote:
> Can you use VideoFrameBuffers as input here instead?

I think the problem is the PrintVideoFrame method. If I fix it to accept a
VideoFrameBuffer, as you suggest in a different comment, then I can use
VideoFrameBuffer for everything here, delete the res_i420frame variables, and
the braces.

I can give it a try.

[magjed_webrtc, 2016-08-25 11:49:08]

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
File webrtc/common_video/libyuv/include/webrtc_libyuv.h (right):

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
webrtc/common_video/libyuv/include/webrtc_libyuv.h:105: const
rtc::scoped_refptr<I420Buffer>& dst_buffer);
On 2016/08/25 11:31:35, nisse-webrtc wrote:
> On 2016/08/25 11:12:56, magjed_webrtc wrote:
> > If dst_buffer is an output argument, you need to use a pointer to a
> > scoped_refptr. (Or a pointer to an I420Buffer if we will allow that).
> 
> I think the const is ok here; the scoped_refptr itself is not modified in any
> way, but the object it points to have it's pixel buffers overwritten.

Yes, using const here will compile, but according to the style guide you need to
use a pointer to a scoped_refptr if it's an output.

[nisse-webrtc, 2016-08-25 12:38:40]

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
File webrtc/common_video/libyuv/include/webrtc_libyuv.h (right):

https://codereview.webrtc.org/2278883002/diff/20001/webrtc/common_video/libyu...
webrtc/common_video/libyuv/include/webrtc_libyuv.h:105: const
rtc::scoped_refptr<I420Buffer>& dst_buffer);
On 2016/08/25 11:49:08, magjed_webrtc wrote:
> On 2016/08/25 11:31:35, nisse-webrtc wrote:
> > On 2016/08/25 11:12:56, magjed_webrtc wrote:
> > > If dst_buffer is an output argument, you need to use a pointer to a
> > > scoped_refptr. (Or a pointer to an I420Buffer if we will allow that).
> > 
> > I think the const is ok here; the scoped_refptr itself is not modified in
any
> > way, but the object it points to have it's pixel buffers overwritten.
> 
> Yes, using const here will compile, but according to the style guide you need
to
> use a pointer to a scoped_refptr if it's an output.

I'll do that then. That makes sense in some way. But it gets awkward to
dereference, like

  int dst_width = (*dst_buffer)->width();

I think I'll count that as one point for using a raw pointer argument instead.

[nisse-webrtc, 2016-08-25 13:20:35]

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

[commit-bot: I haz the power, 2016-08-25 13:20:41]

Dry run: 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, 2016-08-25 14:08:39]

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

[commit-bot: I haz the power, 2016-08-25 14:08:40]

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

[honghaiz3, 2016-09-06 21:47:06]

honghaiz@webrtc.org changed reviewers:
+ honghaiz@webrtc.org

[honghaiz3, 2016-09-06 21:47:07]

lgtm on the api/android directory.

[nisse-webrtc, 2016-09-13 14:31:25]

Ping?

If this is a bit too big, please suggest a piece you'd be willing to review, and
I can chop that off into a separate cl.

I can start with a cl for the two android files (reviewed by by honghaiz).

[perkj_webrtc, 2016-09-13 15:29:22]

puh, that was a big cl.
lgtm

the comments below are suggested improvements not directly related to this cl.

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/common_video/vide...
File webrtc/common_video/video_frame.cc (left):

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/common_video/vide...
webrtc/common_video/video_frame.cc:81: CreateEmptyFrame(width, height, stride_y,
stride_u, stride_v);
Remove CreateEmptyFrame?

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/modules/video_cap...
File webrtc/modules/video_capture/video_capture_impl.cc (left):

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:299:
_captureFrame.set_render_time_ms(rtc::TimeMillis());
render_time_ms set somewhere else?

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.cc (right):

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:28: ChromaGenerator(size_t width, size_t height)
: angle_(0.0) {
change size_t to int and skip the static_cast?

[nisse-webrtc, 2016-09-14 06:59:06]

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/common_video/vide...
File webrtc/common_video/video_frame.cc (left):

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/common_video/vide...
webrtc/common_video/video_frame.cc:81: CreateEmptyFrame(width, height, stride_y,
stride_u, stride_v);
On 2016/09/13 15:29:22, perkj_webrtc wrote:
> Remove CreateEmptyFrame?

I do want to remove CreateEmptyFrame. And the default constructor (they're often
used together) and IsZeroSize. But it's going to be a bit easier after these
changes are landed, it seems it reduces the number of CreateEmptyFrame calls
from 41 to 15.

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/modules/video_cap...
File webrtc/modules/video_capture/video_capture_impl.cc (left):

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/modules/video_cap...
webrtc/modules/video_capture/video_capture_impl.cc:299:
_captureFrame.set_render_time_ms(rtc::TimeMillis());
On 2016/09/13 15:29:22, perkj_webrtc wrote:
> render_time_ms set somewhere else? 

Besides test code, it seems it is called in webrtcvideoengine2.cc,
generic_decoder.cc, and vie_encoder.cc. Also note that since cl
https://codereview.webrtc.org/2282713002, set_render_time_ms is just a
compatibility wrapper for set_timestamp_us.

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/test/frame_genera...
File webrtc/test/frame_generator.cc (right):

https://codereview.webrtc.org/2278883002/diff/180001/webrtc/test/frame_genera...
webrtc/test/frame_generator.cc:28: ChromaGenerator(size_t width, size_t height)
: angle_(0.0) {
On 2016/09/13 15:29:22, perkj_webrtc wrote:
> change size_t to int and skip the static_cast?

The FrameGenerator interface could use a general cleanup. NextFrame is a bit
awkward, and maybe they should produce just VideoFrameBuffers rather than
VideoFrames.

[magjed_webrtc, 2016-09-14 08:45:37]

lgtm

[nisse-webrtc, 2016-09-20 07:01:09]

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

[nisse-webrtc, 2016-09-20 07:01:09]

Stefan, please have a look. Owner's approval needed for modules/video_coding and
modules/video_processing.

[stefan-webrtc, 2016-09-20 11:47:04]

rs lgtm

[nisse-webrtc, 2016-09-20 12:01:11]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-09-20 12:01:11]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
perkj@webrtc.org, honghaiz@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2278883002/#ps260001 (title:
"Delete unused variable.")

[commit-bot: I haz the power, 2016-09-20 12:01:26]

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, 2016-09-20 14:01:42]

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

[commit-bot: I haz the power, 2016-09-20 14:01:43]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-09-21 07:49:06]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2016-09-21 07:49:18]

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, 2016-09-21 08:27:40]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-09-21 08:27:42]

Description was changed from

==========
Move MutableDataY{,U,V} methods to I420Buffer only.

Deleted from the VideoFrameBuffer base class.

BUG=webrtc:5921
==========

to

==========
Move MutableDataY{,U,V} methods to I420Buffer only.

Deleted from the VideoFrameBuffer base class.

BUG=webrtc:5921

Committed: https://crrev.com/5539ef6c03c273f39fadae41ace47fdc11ac6d60
Cr-Commit-Position: refs/heads/master@{#14317}
==========

[commit-bot: I haz the power, 2016-09-21 08:27:43]

Patchset 14 (id:??) landed as
https://crrev.com/5539ef6c03c273f39fadae41ace47fdc11ac6d60
Cr-Commit-Position: refs/heads/master@{#14317}

[nisse-webrtc, 2016-09-21 10:51:36]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.webrtc.org/2354223002/ by nisse@webrtc.org.

The reason for reverting is: Broke downstream application..