Make cricket::VideoFrame inherit webrtc::VideoFrame.

Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors. And similarly for the
subclass cricket::WebRtcVideoFrame.

TBR=tkchin@webrtc.org  # Added an include line
BUG=webrtc:5682
Committed: https://crrev.com/dda6ec008a0fc8d52e118814fb779032e8931968
Cr-Commit-Position: refs/heads/master@{#14576}

[nisse-webrtc, 2016-09-06 09:47:21]

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

[nisse-webrtc, 2016-09-06 09:47:22]

This is almost possible now.

Not sure in which order we should do things. To me it makes some sense to use
aliases. One drawback is that it breaks forward declares of cricket::VideoFrame.
An alternative might be to let cricket::VideoFrame inherit webrtc::VideoFrame
and do nothing.

We should also purge all use of the cricket VideoFrame classes from webrtc, but
that doesn't have to be done in the same cl.

[pthatcher1, 2016-09-07 18:11:39]

https://codereview.webrtc.org/2315663002/diff/1/webrtc/media/base/fakevideoca...
File webrtc/media/base/fakevideocapturer.h (left):

https://codereview.webrtc.org/2315663002/diff/1/webrtc/media/base/fakevideoca...
webrtc/media/base/fakevideocapturer.h:25: #endif
Why would fakevideocapturer.h be used if HAVE_WEBRTC_VIDEO is false?

https://codereview.webrtc.org/2315663002/diff/1/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/2315663002/diff/1/webrtc/video_frame.h#newcode193
webrtc/video_frame.h:193: friend class cricket::WebRtcVideoFrameTest;
A reference to a unit test class?  That seems terrible.  Why don't we just add
some kind of set_rotation method?

[nisse-webrtc, 2016-09-08 11:07:13]

https://codereview.webrtc.org/2315663002/diff/1/webrtc/media/base/fakevideoca...
File webrtc/media/base/fakevideocapturer.h (left):

https://codereview.webrtc.org/2315663002/diff/1/webrtc/media/base/fakevideoca...
webrtc/media/base/fakevideocapturer.h:25: #endif
On 2016/09/07 18:11:39, pthatcher1 wrote:
> Why would fakevideocapturer.h be used if HAVE_WEBRTC_VIDEO is false?

I'm afraid I have no idea about that.

https://codereview.webrtc.org/2315663002/diff/1/webrtc/video_frame.h
File webrtc/video_frame.h (right):

https://codereview.webrtc.org/2315663002/diff/1/webrtc/video_frame.h#newcode193
webrtc/video_frame.h:193: friend class cricket::WebRtcVideoFrameTest;
On 2016/09/07 18:11:39, pthatcher1 wrote:
> A reference to a unit test class?  That seems terrible.  Why don't we just add
> some kind of set_rotation method?

No longer needed, since this cl also deletes those tests. Deleted.

For background, this friend declaration can be found in  in webrtcvideoframe.h
on master. WebRtcVideoFrame used to have a set_rotation method, used only by
tests. It was removed by sergeyu and replaced by this friend declaration, in cl
https://codereview.webrtc.org/2075983003/

[pthatcher1, 2016-09-09 02:22:32]

This seems great.  Hopefully you'll be able to land it soon.  Please let me know
once it's ready for submission (once the dependencies are submitted).

[nisse-webrtc, 2016-09-09 13:44:02]

Description was changed from

==========
Make cricket::VideoFrame and cricket::WebRtcVideoFrame aliases for
webrtc::VideoFrame.

WIP cl, on top of https://codereview.webrtc.org/2262443003/ and
https://codereview.webrtc.org/2305623002/.

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

to

==========
Make cricket::VideoFrame and cricket::WebRtcVideoFrame aliases for
webrtc::VideoFrame.

WIP cl, on top of https://codereview.webrtc.org/2262443003/ .

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

[nisse-webrtc, 2016-09-09 14:10:54]

Description was changed from

==========
Make cricket::VideoFrame and cricket::WebRtcVideoFrame aliases for
webrtc::VideoFrame.

WIP cl, on top of https://codereview.webrtc.org/2262443003/ .

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

to

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors.

WIP cl, on top of https://codereview.webrtc.org/2262443003/ .

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

[nisse-webrtc, 2016-09-30 10:55:29]

Description was changed from

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors.

WIP cl, on top of https://codereview.webrtc.org/2262443003/ .

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

to

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors. And similarly for the
subclass cricket::WebRtcVideoFrame.

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

[nisse-webrtc, 2016-09-30 11:01:31]

Cl https://codereview.webrtc.org/2370993003/ was landed earlier this morning,
with no immediate breakages. This is the next step.

In a followup, we should then be able to delete all references to the old
classes within webrtc. But I expect there might get some complications, when
outside code inherits classes and overrides methods currently expecting
cricket::VideoFrame.

[nisse-webrtc, 2016-10-04 14:49:14]

Ping?

[perkj_webrtc, 2016-10-04 15:36:24]

Sorry, I was not aware this was ready and waiting. 

Looks great.

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (left):

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/BUILD.gn#old...
webrtc/media/BUILD.gn:311: "base/videoframe_unittest.h",
does these tests exist somewhere else?

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:11: // Deprecated, replace by cricket::VideoFrame
by webrtc::VideoFrame
replace ....

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:26: webrtc::VideoRotation rotation,
git cl format ?

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

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:11: // Deprecated, replace by
cricket::WebRtcVideoFrame by
/s replace....

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe_unittest.cc (left):

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe_unittest.cc:128: WebRtcVideoFrame applied0;
Is this test worth keeping or is there another similar test?

[pthatcher1, 2016-10-04 15:41:54]

Nice to see this getting so close.

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:11: // Deprecated, replace by cricket::VideoFrame
by webrtc::VideoFrame
On 2016/10/04 15:36:23, perkj_webrtc wrote:
> replace ....

"replace by X by Y" => "replace X with Y"

Probably also needs a "TODO".

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

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:11: // Deprecated, replace by
cricket::WebRtcVideoFrame by
On 2016/10/04 15:36:24, perkj_webrtc wrote:
> /s replace....

Same here.  (one "with" instead of two "by"s)

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:42: set_timestamp(transport_frame_id);
That seems like kind of a crazy assumption.  Would it be better to have a
separate field in the class?

[nisse-webrtc, 2016-10-06 07:49:45]

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/BUILD.gn
File webrtc/media/BUILD.gn (left):

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/BUILD.gn#old...
webrtc/media/BUILD.gn:311: "base/videoframe_unittest.h",
On 2016/10/04 15:36:23, perkj_webrtc wrote:
> does these tests exist somewhere else?

No, they test features not present in webrtc::VideoFrame.

Unit tests of webrtc::VideoFrame seem to be located in
https://cs.chromium.org/chromium/src/third_party/webrtc/common_video/i420_vid...

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/base/videofr...
File webrtc/media/base/videoframe.h (right):

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:11: // Deprecated, replace by cricket::VideoFrame
by webrtc::VideoFrame
On 2016/10/04 15:41:54, pthatcher1 wrote:
> On 2016/10/04 15:36:23, perkj_webrtc wrote:
> > replace ....
> 
> "replace by X by Y" => "replace X with Y"
> 
> Probably also needs a "TODO".

Done.

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/base/videofr...
webrtc/media/base/videoframe.h:26: webrtc::VideoRotation rotation,
On 2016/10/04 15:36:23, perkj_webrtc wrote:
> git cl format ?

Done.

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

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:11: // Deprecated, replace by
cricket::WebRtcVideoFrame by
On 2016/10/04 15:41:54, pthatcher1 wrote:
> On 2016/10/04 15:36:24, perkj_webrtc wrote:
> > /s replace....
> 
> Same here.  (one "with" instead of two "by"s)

Done.

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:42: set_timestamp(transport_frame_id);
On 2016/10/04 15:41:54, pthatcher1 wrote:
> That seems like kind of a crazy assumption.  Would it be better to have a
> separate field in the class?

That's how it currently works. I discussed it with sergeyu (who wrote the code
using transport_frame_id), and we agreed it's good enough for the time being. I
could add a new field, but I really dislike adding even more timestamps to
webrtc::VideoFrame.

Unclear when we're get to cleaning this up (since I'm going to focus on quic
coming months), but I'd like to delete both the timestamp field (90 kHz rtp
clock) and the ntp timestamp from the webrtc::VideoFrame class. Left would only
be timestamp_us (rtc::TimeMicros time) and (if needed) transport_frame_id, which
happens to be the same thing as the rtp timestamp.

If Per agrees, I can add a separate webrtc::VideoFrame::transport_frame_id_
member.

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvideoframe_unittest.cc (left):

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe_unittest.cc:128: WebRtcVideoFrame applied0;
On 2016/10/04 15:36:23, perkj_webrtc wrote:
> Is this test worth keeping or is there another similar test?

It used to be a unit test for GetCopyWithRotationApplied, then hacked a bit to
still work when that method was deleted.

We need tests for apply rotation logic, but I don't think this is the way to do
it. You added an end-to-end test a while back, right? 

It seems we lack unittests for I420Buffer::Rotate, though.

[pthatcher2, 2016-10-06 16:43:50]

pthatcher@chromium.org changed reviewers:
+ pthatcher@chromium.org

[pthatcher2, 2016-10-06 16:43:51]

lgtm

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

https://codereview.webrtc.org/2315663002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvideoframe.h:42: set_timestamp(transport_frame_id);
On 2016/10/06 07:49:44, nisse-webrtc wrote:
> On 2016/10/04 15:41:54, pthatcher1 wrote:
> > That seems like kind of a crazy assumption.  Would it be better to have a
> > separate field in the class?
> 
> That's how it currently works. I discussed it with sergeyu (who wrote the code
> using transport_frame_id), and we agreed it's good enough for the time being.
I
> could add a new field, but I really dislike adding even more timestamps to
> webrtc::VideoFrame.
> 
> Unclear when we're get to cleaning this up (since I'm going to focus on quic
> coming months), but I'd like to delete both the timestamp field (90 kHz rtp
> clock) and the ntp timestamp from the webrtc::VideoFrame class. Left would
only
> be timestamp_us (rtc::TimeMicros time) and (if needed) transport_frame_id,
which
> happens to be the same thing as the rtp timestamp.
> 
> If Per agrees, I can add a separate webrtc::VideoFrame::transport_frame_id_
> member.

I'll leave it up to you guys to decide.

[perkj_webrtc, 2016-10-07 07:06:59]

Great to see this finally happening. 
I am fine with just a todo and a comment about the frame_id vs rtp timestamp id.


lgtm provided you write a test for I420Buffer::Rotate separately.

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

https://codereview.webrtc.org/2315663002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:41: // For now, transport_frame_id and
rtp timestamp are the same.
TODO... - reconsider for quick?

[nisse-webrtc, 2016-10-07 12:47:31]

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

https://codereview.webrtc.org/2315663002/diff/120001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoframe.h:41: // For now, transport_frame_id and
rtp timestamp are the same.
On 2016/10/07 07:06:59, perkj_webrtc wrote:
> TODO... - reconsider for quick? 

Done.

[nisse-webrtc, 2016-10-07 12:51:38]

On 2016/10/07 07:06:59, perkj_webrtc wrote:
> Great to see this finally happening. 

Crossing my fingers that nothing breaks when landing...

> lgtm provided you write a test for I420Buffer::Rotate separately.

Should be doable using something similar to the tests of CropAndScaleFrom.

[nisse-webrtc, 2016-10-07 12:51:51]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-10-07 12:51:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@chromium.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2315663002/#ps140001 (title:
"Added TODO comment.")

[commit-bot: I haz the power, 2016-10-07 12:51:56]

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-10-07 12:55:50]

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

[commit-bot: I haz the power, 2016-10-07 12:55:52]

Try jobs failed on following builders:
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11336)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/11351)

[nisse-webrtc, 2016-10-07 13:30:10]

Description was changed from

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors. And similarly for the
subclass cricket::WebRtcVideoFrame.

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

to

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors. And similarly for the
subclass cricket::WebRtcVideoFrame.

TBR=tkchin@webrtc.org  # Added an include line
BUG=webrtc:5682
==========

[nisse-webrtc, 2016-10-07 13:30:10]

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

[nisse-webrtc, 2016-10-07 13:30:25]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-10-07 13:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@chromium.org, perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2315663002/#ps160001 (title:
"Add missing include of logging.h.")

[commit-bot: I haz the power, 2016-10-07 13:30:28]

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-10-07 15:16:57]

Description was changed from

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors. And similarly for the
subclass cricket::WebRtcVideoFrame.

TBR=tkchin@webrtc.org  # Added an include line
BUG=webrtc:5682
==========

to

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors. And similarly for the
subclass cricket::WebRtcVideoFrame.

TBR=tkchin@webrtc.org  # Added an include line
BUG=webrtc:5682
==========

[commit-bot: I haz the power, 2016-10-07 15:16:58]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-07 15:17:04]

Description was changed from

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors. And similarly for the
subclass cricket::WebRtcVideoFrame.

TBR=tkchin@webrtc.org  # Added an include line
BUG=webrtc:5682
==========

to

==========
Make cricket::VideoFrame inherit webrtc::VideoFrame. Delete
all methods but a few constructors. And similarly for the
subclass cricket::WebRtcVideoFrame.

TBR=tkchin@webrtc.org  # Added an include line
BUG=webrtc:5682

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

[commit-bot: I haz the power, 2016-10-07 15:17:05]

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

[kjellander_webrtc, 2016-10-09 04:40:26]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.webrtc.org/2402853002/ by kjellander@webrtc.org.

The reason for reverting is: Breaks compile for Chromium builds:
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Linux%20Builder/bui...
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Mac%20Builder/build...

FAILED: obj/remoting/protocol/protocol/webrtc_video_renderer_adapter.o 
../../remoting/protocol/webrtc_video_renderer_adapter.cc:110:52: error: no
member named 'transport_frame_id' in 'cricket::VideoFrame'
                 weak_factory_.GetWeakPtr(), frame.transport_frame_id(),
                                             ~~~~~ ^
1 error generated.

Please run chromium trybots as described at
https://webrtc.org/contributing/#tryjobs-on-chromium-trybots before relanding..