Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

BUG=webrtc:5682
R=nisse@webrtc.org, pbos@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/cd89e86428119088cdfa4052418d58bd9179f5e4

[Sergey Ulanov, 2016-06-17 22:10:12]

Description was changed from

==========
Cleanup cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.
==========

to

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.
==========

[Sergey Ulanov, 2016-06-17 22:14:12]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-06-17 22:15:18]

sergeyu@chromium.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2016-06-17 22:25:36]

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

[pbos-webrtc, 2016-06-17 22:29:13]

+R nisse@, PTAL. Are these kept around for downstream projects or are they ready
to go?

[nisse-webrtc, 2016-06-20 07:52:23]

I'm not aware of any obstacles to deleting these methods, it's good to see them
go.

You can refer to bug https://bugs.chromium.org/p/webrtc/issues/detail?id=5682

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.cc (right):

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.cc:184: rotated_frame_.reset(new
WebRtcVideoFrame());
The plan is to make VideoFrameBuffer always immutable. So it's more in line with
this plan to arrange this to first create an I420Buffer for the rotated image
(I420Buffer will stay mutable), and then use the WebRtcVideoFrame constructor
taking buffer, timestamp and rotation as arguments.

See https://bugs.chromium.org/p/webrtc/issues/detail?id=5921

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:104: friend class WebRtcVideoFrameTest;
Why? At least deserves a comment.

[Sergey Ulanov, 2016-06-20 18:24:13]

Description was changed from

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.
==========

to

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

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

[Sergey Ulanov, 2016-06-20 18:42:46]

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.cc (right):

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.cc:184: rotated_frame_.reset(new
WebRtcVideoFrame());
On 2016/06/20 07:52:23, nisse-webrtc wrote:
> The plan is to make VideoFrameBuffer always immutable. So it's more in line
with
> this plan to arrange this to first create an I420Buffer for the rotated image
> (I420Buffer will stay mutable), and then use the WebRtcVideoFrame constructor
> taking buffer, timestamp and rotation as arguments.
> 
> See https://bugs.chromium.org/p/webrtc/issues/detail?id=5921

Done.

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:104: friend class WebRtcVideoFrameTest;
On 2016/06/20 07:52:23, nisse-webrtc wrote:
> Why? At least deserves a comment.

Tests need to mutate |rotation_|. Added comment. It would be easy to avoid by
removing GetCopyWithRotationApplied() from VideoFrame and replacing it with a
static RotateVideoFrame(frame, rotation).

[nisse-webrtc, 2016-06-21 07:52:35]

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.cc (right):

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.cc:184: rotated_frame_.reset(new
WebRtcVideoFrame());
On 2016/06/20 18:42:45, Sergey Ulanov wrote:
> On 2016/06/20 07:52:23, nisse-webrtc wrote:
> > The plan is to make VideoFrameBuffer always immutable. So it's more in line
> with
> > this plan to arrange this to first create an I420Buffer for the rotated
image
> > (I420Buffer will stay mutable), and then use the WebRtcVideoFrame
constructor
> > taking buffer, timestamp and rotation as arguments.
> > 
> > See https://bugs.chromium.org/p/webrtc/issues/detail?id=5921
> 
> Done.

It's not quite enough; rotated_frame->video_frame_buffer() returns a reference
to the base class VideoFrameBuffer, where we'd like to delete the MutableData
methods. So I suggest you write it as

rtc::scoped_refptr<webrtc::I420Buffer> rotated_buffer(
  new RefCountedObject<webrtc::I420Buffer>(rotated_width, rotated_height));

libyuv::I420Rotate(... rotated_buffer->MutableDataY(), ...);

rotated_frame.reset(new WebRtcVideoFrame(rotated_buffer, ...);

(I find constructing the WebRtcVideoFrame last is logical, but order isn't that
important. The important detail is that mutation is done via a pointer to the
subclass I420Buffer, not the base class VideoFrameBuffer).

https://codereview.webrtc.org/2075983003/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2075983003/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:104: // Tests mutate |rotataion_|, so the
base test class is a friend.
Spelling |rotation_|, otherwise good. 

Ultimately I'd like to move or delete GetCopyWithRotationApplied, and then I'd
expect this friend declaration can go away too.

[Sergey Ulanov, 2016-06-21 17:25:56]

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.cc (right):

https://codereview.webrtc.org/2075983003/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.cc:184: rotated_frame_.reset(new
WebRtcVideoFrame());
On 2016/06/21 07:52:35, nisse-webrtc wrote:
> On 2016/06/20 18:42:45, Sergey Ulanov wrote:
> > On 2016/06/20 07:52:23, nisse-webrtc wrote:
> > > The plan is to make VideoFrameBuffer always immutable. So it's more in
line
> > with
> > > this plan to arrange this to first create an I420Buffer for the rotated
> image
> > > (I420Buffer will stay mutable), and then use the WebRtcVideoFrame
> constructor
> > > taking buffer, timestamp and rotation as arguments.
> > > 
> > > See https://bugs.chromium.org/p/webrtc/issues/detail?id=5921
> > 
> > Done.
> 
> It's not quite enough; rotated_frame->video_frame_buffer() returns a reference
> to the base class VideoFrameBuffer, where we'd like to delete the MutableData
> methods. So I suggest you write it as
> 
> rtc::scoped_refptr<webrtc::I420Buffer> rotated_buffer(
>   new RefCountedObject<webrtc::I420Buffer>(rotated_width, rotated_height));
> 
> libyuv::I420Rotate(... rotated_buffer->MutableDataY(), ...);
> 
> rotated_frame.reset(new WebRtcVideoFrame(rotated_buffer, ...);
> 
> (I find constructing the WebRtcVideoFrame last is logical, but order isn't
that
> important. The important detail is that mutation is done via a pointer to the
> subclass I420Buffer, not the base class VideoFrameBuffer).

Done, though it's not really related to this CL, and potentially could be done
in a separate CL.
Also, we both agree that GetCopyWithRotationApplied() should be removed from
this class, so it doesn't makes sense to spend much time on it here.

https://codereview.webrtc.org/2075983003/diff/40001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2075983003/diff/40001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:104: // Tests mutate |rotataion_|, so the
base test class is a friend.
On 2016/06/21 07:52:35, nisse-webrtc wrote:
> Spelling |rotation_|, otherwise good. 
> 
> Ultimately I'd like to move or delete GetCopyWithRotationApplied, and then I'd
> expect this friend declaration can go away too.

Done.

[Sergey Ulanov, 2016-06-21 17:26:07]

Patchset #3 (id:60001) has been deleted

[nisse-webrtc, 2016-06-22 07:18:40]

lgtm

Thanks for updating the code, each place where mutation of VideoFrameBuffer is
eliminated makes refactoring easier.

https://codereview.webrtc.org/2075983003/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2075983003/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:114: mutable
std::unique_ptr<WebRtcVideoFrame> rotated_frame_;
I think this change of type is unnecessary, but I don't have a strong opinion on
which type to use.

[Sergey Ulanov, 2016-06-22 20:58:30]

pbos@: PTAL please

https://codereview.webrtc.org/2075983003/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2075983003/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:114: mutable
std::unique_ptr<WebRtcVideoFrame> rotated_frame_;
On 2016/06/22 07:18:40, nisse-webrtc (ooo August 15) wrote:
> I think this change of type is unnecessary, but I don't have a strong opinion
on
> which type to use.

reverted

[pbos-webrtc, 2016-06-23 14:27:46]

lgtm, consider my nit

https://codereview.webrtc.org/2075983003/diff/90001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe_unittest.cc (right):

https://codereview.webrtc.org/2075983003/diff/90001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe_unittest.cc:71: void
SetFrameRotation(WebRtcVideoFrame* frame,
Preferring just inlining this, not sure if this function makes sense.

frame->rotation_ = rotation; is just as readable.

[Sergey Ulanov, 2016-06-23 18:12:10]

https://codereview.webrtc.org/2075983003/diff/90001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe_unittest.cc (right):

https://codereview.webrtc.org/2075983003/diff/90001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe_unittest.cc:71: void
SetFrameRotation(WebRtcVideoFrame* frame,
On 2016/06/23 14:27:45, pbos-webrtc wrote:
> Preferring just inlining this, not sure if this function makes sense.
> 
> frame->rotation_ = rotation; is just as readable.

Done. That requires that the test itself is a friend with WebRtcVideoFrame
instead of WebRtcVideoFrameTest, but that's acceptable given that rotation_
needs to be mutated in only one test.

[Sergey Ulanov, 2016-06-23 18:12:15]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-23 18:12:16]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2075983003/#ps110001 (title:
".")

[commit-bot: I haz the power, 2016-06-23 18:12:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2075983003/110001

[commit-bot: I haz the power, 2016-06-23 19:56:08]

Description was changed from

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

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

to

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

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

[commit-bot: I haz the power, 2016-06-23 19:56:09]

Committed patchset #5 (id:110001)

[commit-bot: I haz the power, 2016-06-23 19:56:14]

Description was changed from

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

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

to

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

BUG=webrtc:5682

Committed: https://crrev.com/742d7b10b9720ec43de26e0faef52e5cb9c0daa8
Cr-Commit-Position: refs/heads/master@{#13275}
==========

[commit-bot: I haz the power, 2016-06-23 19:56:15]

Patchset 5 (id:??) landed as
https://crrev.com/742d7b10b9720ec43de26e0faef52e5cb9c0daa8
Cr-Commit-Position: refs/heads/master@{#13275}

[Taylor Brandstetter, 2016-06-23 23:39:50]

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

[Taylor Brandstetter, 2016-06-23 23:39:51]

https://codereview.webrtc.org/2075983003/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2075983003/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:22: #include
"webrtc/base/gtest_prod_util.h"
It looks like this broke the FYI bot, because Chromoting includes both this
gtest_prod_util and Chrome's gtest_prod_util. The macros in ours should probably
have an "RTC_" prefix added.

[honghaiz3, 2016-06-23 23:42:37]

On 2016/06/23 23:39:51, Taylor Brandstetter wrote:
>
https://codereview.webrtc.org/2075983003/diff/110001/webrtc/media/engine/webr...
> File webrtc/media/engine/webrtcvideoframe.h (right):
> 
>
https://codereview.webrtc.org/2075983003/diff/110001/webrtc/media/engine/webr...
> webrtc/media/engine/webrtcvideoframe.h:22: #include
> "webrtc/base/gtest_prod_util.h"
> It looks like this broke the FYI bot, because Chromoting includes both this
> gtest_prod_util and Chrome's gtest_prod_util. The macros in ours should
probably
> have an "RTC_" prefix added.

I will be starting to revert this CL.

[honghaiz3, 2016-06-23 23:43:29]

A revert of this CL (patchset #5 id:110001) has been created in
https://codereview.webrtc.org/2091983002/ by honghaiz@webrtc.org.

The reason for reverting is: Breaking Chrome FYI bots. .

[Sergey Ulanov, 2016-06-24 00:29:53]

Description was changed from

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

BUG=webrtc:5682

Committed: https://crrev.com/742d7b10b9720ec43de26e0faef52e5cb9c0daa8
Cr-Commit-Position: refs/heads/master@{#13275}
==========

to

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

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

[Sergey Ulanov, 2016-06-24 00:30:03]

https://codereview.webrtc.org/2075983003/diff/110001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoframe.h (right):

https://codereview.webrtc.org/2075983003/diff/110001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:22: #include
"webrtc/base/gtest_prod_util.h"
On 2016/06/23 23:39:51, Taylor Brandstetter wrote:
> It looks like this broke the FYI bot, because Chromoting includes both this
> gtest_prod_util and Chrome's gtest_prod_util. The macros in ours should
probably
> have an "RTC_" prefix added.

yep. I'm going to revert to the previous version of this CL that didn't need 
FRIEND_TEST_ALL_PREFIXES.

[Sergey Ulanov, 2016-06-24 00:31:42]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-06-24 00:31:42]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2075983003/#ps130001 (title:
"revert to patchset 4")

[commit-bot: I haz the power, 2016-06-24 00:32:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2075983003/130001

[commit-bot: I haz the power, 2016-06-24 00:39:03]

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

[commit-bot: I haz the power, 2016-06-24 00:39:05]

Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14436)

[Sergey Ulanov, 2016-06-24 00:54:11]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-06-24 00:54:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2075983003/130001

[commit-bot: I haz the power, 2016-06-24 01:00:43]

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

[commit-bot: I haz the power, 2016-06-24 01:00:45]

Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14437)

[Sergey Ulanov, 2016-06-24 16:00:49]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-06-24 16:00:54]

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-06-24 16:15:50]

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

[commit-bot: I haz the power, 2016-06-24 16:15:51]

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...)

[Taylor Brandstetter, 2016-06-24 17:42:20]

deadbeef@webrtc.org changed reviewers:
- deadbeef@webrtc.org

[Sergey Ulanov, 2016-06-24 21:47:32]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-06-24 21:47:41]

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-06-24 21:54:50]

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

[commit-bot: I haz the power, 2016-06-24 21:54:51]

Try jobs failed on following builders:
  android_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/14453)

[commit-bot: I haz the power, 2016-06-24 23:28:32]

Description was changed from

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

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

to

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

BUG=webrtc:5682
R=nisse@webrtc.org, pbos@webrtc.org

Committed: https://crrev.com/cd89e86428119088cdfa4052418d58bd9179f5e4
Cr-Commit-Position: refs/heads/master@{#13288}
==========

[Sergey Ulanov, 2016-06-24 23:28:32]

Description was changed from

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

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

to

==========
Cleanups in cricket::VideoFrame and cricket::WebRtcVideoFrame.

Removed some protected virtual methods from VideoFrame that no longer
need to exist. Some minor cleanups in the tests.

BUG=webrtc:5682
R=nisse@webrtc.org, pbos@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/cd89e86428119088cdfa40524...
==========

[Sergey Ulanov, 2016-06-24 23:28:34]

Committed patchset #6 (id:130001) manually as
cd89e86428119088cdfa4052418d58bd9179f5e4 (presubmit successful).

[commit-bot: I haz the power, 2016-06-24 23:28:34]

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