Move InitToBlack and Reset methods from cricket::VideoFrame to its subclass cricket::WebRtcVideoFra…

Move InitToBlack and Reset methods from cricket::VideoFrame to its subclass cricket::WebRtcVideoFrame.

BUG=webrtc:5682
R=perkj@webrtc.org

Committed: https://crrev.com/03bd4008b61b7ead41528610de75adafa22d3faf
Cr-Commit-Position: refs/heads/master@{#12260}

[nisse-webrtc, 2016-03-24 09:42:21]

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

[nisse-webrtc, 2016-03-24 09:42:22]

Eliminates two public methods of cricket::VideoFrame.

About the tests, not sure if there are other tests which exercise the the
apply_rotation logic in Reset? It is used when constructing a WebRtcVideoFrame
from a CapturedFrame.

[perkj_webrtc, 2016-03-24 09:53:00]

Change looks good but 
what is the purpose of moving them if the end goal is to stop using
cricket::VIdeoFrame and instead use webrtc::VideoFrame?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

https://codereview.webrtc.org/1831523004/diff/1/webrtc/media/base/videoframe_...
File webrtc/media/base/videoframe_unittest.h (right):

https://codereview.webrtc.org/1831523004/diff/1/webrtc/media/base/videoframe_...
webrtc/media/base/videoframe_unittest.h:1375: // TODO(nisse): Reset is no longer
a public method, and not
Take a look and advice please. 
You can probably make an init test out of this.

[nisse-webrtc, 2016-03-24 10:03:14]

I like to cut down on the size of the cricket::VideoFrame interface, to get it
closer to webrtc::VideoFrame.

But then I'm not sure where cricket::WebRtcVideoFrame fits in the picture, I
guess it should also ultimately be deleted, and all needed features moved over
to webrtc::VideoFrame?

https://codereview.webrtc.org/1831523004/diff/1/webrtc/media/base/videoframe_...
File webrtc/media/base/videoframe_unittest.h (right):

https://codereview.webrtc.org/1831523004/diff/1/webrtc/media/base/videoframe_...
webrtc/media/base/videoframe_unittest.h:1375: // TODO(nisse): Reset is no longer
a public method, and not
On 2016/03/24 09:53:00, perkj_webrtc wrote:
> Take a look and advice please. 
> You can probably make an init test out of this.

Only problem is that the Init-method to test takes a CapturedFrame as input. I
can do that, question is if it is needed, or if we have other tests calling
Init(CapturedFrame, ... ) with various values of rotation and apply_rotation.

[perkj_webrtc, 2016-03-24 10:26:19]

On 2016/03/24 10:03:14, nisse-webrtc wrote:
> I like to cut down on the size of the cricket::VideoFrame interface, to get it
> closer to webrtc::VideoFrame.
> 
> But then I'm not sure where cricket::WebRtcVideoFrame fits in the picture, I
> guess it should also ultimately be deleted, and all needed features moved over
> to webrtc::VideoFrame?
cricket::WebRtcVideoFrame is a cricket::VideoFrame and should go away yes.
We can also put things in the VideoFrameFactory that is only used when creating
new frames. 
> 
>
https://codereview.webrtc.org/1831523004/diff/1/webrtc/media/base/videoframe_...
> File webrtc/media/base/videoframe_unittest.h (right):
> 
>
https://codereview.webrtc.org/1831523004/diff/1/webrtc/media/base/videoframe_...
> webrtc/media/base/videoframe_unittest.h:1375: // TODO(nisse): Reset is no
longer
> a public method, and not
> On 2016/03/24 09:53:00, perkj_webrtc wrote:
> > Take a look and advice please. 
> > You can probably make an init test out of this.
> 
> Only problem is that the Init-method to test takes a CapturedFrame as input. I
> can do that, question is if it is needed, or if we have other tests calling
> Init(CapturedFrame, ... ) with various values of rotation and apply_rotation.
If it it is not used in any other way then yes.  I guess it is not since you
could move Reset to be protected.

[nisse-webrtc, 2016-03-24 13:18:31]

On 2016/03/24 10:26:19, perkj_webrtc wrote:
> On 2016/03/24 10:03:14, nisse-webrtc wrote:
> > I like to cut down on the size of the cricket::VideoFrame interface, to get
it
> > closer to webrtc::VideoFrame.
> > 
> > But then I'm not sure where cricket::WebRtcVideoFrame fits in the picture, I
> > guess it should also ultimately be deleted, and all needed features moved
over
> > to webrtc::VideoFrame?
> cricket::WebRtcVideoFrame is a cricket::VideoFrame and should go away yes.
> We can also put things in the VideoFrameFactory that is only used when
creating
> new frames. 
> > 
> >
>
https://codereview.webrtc.org/1831523004/diff/1/webrtc/media/base/videoframe_...
> > File webrtc/media/base/videoframe_unittest.h (right):
> > 
> >
>
https://codereview.webrtc.org/1831523004/diff/1/webrtc/media/base/videoframe_...
> > webrtc/media/base/videoframe_unittest.h:1375: // TODO(nisse): Reset is no
> longer
> > a public method, and not
> > On 2016/03/24 09:53:00, perkj_webrtc wrote:
> > > Take a look and advice please. 
> > > You can probably make an init test out of this.
> > 
> > Only problem is that the Init-method to test takes a CapturedFrame as input.
I
> > can do that, question is if it is needed, or if we have other tests calling
> > Init(CapturedFrame, ... ) with various values of rotation and
apply_rotation.
> If it it is not used in any other way then yes.  I guess it is not since you
> could move Reset to be protected.

The Reset method *is* protected in this cl... That's why the test can't be used
as is.

[nisse-webrtc, 2016-03-30 11:33:44]

I've investigated the testing. Broken apply_rotation handling in the Reset
method triggers other unit tests, including
WebRtcVideoFrameTest.ConstructI420Mirror and
WebRtcVideoFrameTest.InitRotated90DontApplyRotation. Hence the Reset test can be
deleted.

[perkj_webrtc, 2016-03-31 05:01:22]

lgtm

[nisse-webrtc, 2016-04-06 09:04:13]

The CQ bit was checked by nisse@webrtc.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1831523004/#ps40001 (title:
"Rebase.")

[commit-bot: I haz the power, 2016-04-06 09:04:25]

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

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

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

[commit-bot: I haz the power, 2016-04-06 11:05:12]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[nisse-webrtc, 2016-04-06 11:07:24]

Description was changed from

==========
Move InitToBlack and Reset methods from cricket::VideoFrame to its subclass
cricket::WebRtcVideoFrame.

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

to

==========
Move InitToBlack and Reset methods from cricket::VideoFrame to its subclass
cricket::WebRtcVideoFrame.

BUG=webrtc:5682
R=perkj@webrtc.org

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

[nisse-webrtc, 2016-04-06 11:07:26]

Committed patchset #3 (id:40001) manually as
03bd4008b61b7ead41528610de75adafa22d3faf (presubmit successful).

[commit-bot: I haz the power, 2016-04-06 11:07:26]

Description was changed from

==========
Move InitToBlack and Reset methods from cricket::VideoFrame to its subclass
cricket::WebRtcVideoFrame.

BUG=webrtc:5682
R=perkj@webrtc.org

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

to

==========
Move InitToBlack and Reset methods from cricket::VideoFrame to its subclass
cricket::WebRtcVideoFrame.

BUG=webrtc:5682
R=perkj@webrtc.org

Committed: https://crrev.com/03bd4008b61b7ead41528610de75adafa22d3faf
Cr-Commit-Position: refs/heads/master@{#12260}
==========

[commit-bot: I haz the power, 2016-04-06 11:07:27]

Patchset 3 (id:??) landed as
https://crrev.com/03bd4008b61b7ead41528610de75adafa22d3faf
Cr-Commit-Position: refs/heads/master@{#12260}