Move VideoFrame and related declarations to webrtc/api/video.

Move VideoFrame and related declarations to webrtc/api/video.

Moves webrtc/common_video/rotation.h and parts of
webrtc/common_video/include/video_frame_buffer.h and
webrtc/video_frame.h, and adds to a new GN target api:video_frame_api.

BUG=webrtc:5880
Review-Url: https://codereview.webrtc.org/2517173004
Cr-Commit-Position: refs/heads/master@{#15993}
Committed: https://chromium.googlesource.com/external/webrtc/+/af916899cc8ec685ebeaf414d5aa86ca83e1fca6

[nisse-webrtc, 2016-11-23 14:28:17]

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

[nisse-webrtc, 2016-11-23 14:28:18]

This moves VideoFrame and VideoFrameBuffer into webrtc/api/video/

VideoFrameBuffer subclasses are left under common_video. Which is a bit of a
problem:

examples/peerconnection/client/linux/main_wnd.cc calls the static method
webrtc::I420Buffer::Rotate (and similarly for other platforms).

Some options:

1. Move I420Buffer to webrtc/api/video/ as well. Makes some sense, since this is
the main way to create frames with pixel data, which is essential when
implementing a custom capturer. But I haven't been thinking about it as a public
api, so maybe it isn't ready.

2. White-list inclusion of common_video/include/video_frame_buffer, to silence
presubmit warnings while we figure out what the right thing is.

3. Move the Rotate method to the VideoFrameBuffer namespace (that it happens to
construct an I420Buffer can be viewed as an implementation detail). But I'm
afraid that would introduce a circular GN dependency api:video_frame_api <-->
common_video).

What would you prefer?

[the sun, 2016-11-24 10:34:26]

On 2016/11/23 14:28:18, nisse-webrtc wrote:
> This moves VideoFrame and VideoFrameBuffer into webrtc/api/video/
> 
> VideoFrameBuffer subclasses are left under common_video. Which is a bit of a
> problem:
> 
> examples/peerconnection/client/linux/main_wnd.cc calls the static method
> webrtc::I420Buffer::Rotate (and similarly for other platforms).
> 
> Some options:
> 
> 1. Move I420Buffer to webrtc/api/video/ as well. Makes some sense, since this
is
> the main way to create frames with pixel data, which is essential when
> implementing a custom capturer. But I haven't been thinking about it as a
public
> api, so maybe it isn't ready.
> 
> 2. White-list inclusion of common_video/include/video_frame_buffer, to silence
> presubmit warnings while we figure out what the right thing is.
> 
> 3. Move the Rotate method to the VideoFrameBuffer namespace (that it happens
to
> construct an I420Buffer can be viewed as an implementation detail). But I'm
> afraid that would introduce a circular GN dependency api:video_frame_api <-->
> common_video).
> 
> What would you prefer?

Option #1 seems to make the most sense, but if you're not sure, let's discuss,
as I'm not fully aware of how these classes fit together.

[the sun, 2016-11-24 10:36:59]

Great stuff!

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/android/jni/native...
File webrtc/api/android/jni/native_handle_impl.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/android/jni/native...
webrtc/api/android/jni/native_handle_impl.h:17: #include
"webrtc/api/video/rotation.h"
nit: include order

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/android/jni/surfac...
File webrtc/api/android/jni/surfacetexturehelper_jni.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/android/jni/surfac...
webrtc/api/android/jni/surfacetexturehelper_jni.h:16: #include
"webrtc/api/video/video_frame_buffer.h"
nit: include order here as well (not going to comment again if there are more of
these - take a look across the changes and git cl format)

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/video/video_frame.h
File webrtc/api/video/video_frame.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/video/video_frame....
webrtc/api/video/video_frame.h:13: 
nit: add stdint.h

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/video/video_frame....
webrtc/api/video/video_frame.h:17: // than forward declaring it. Right?
Agree.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/include/v...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/include/v...
webrtc/common_video/include/video_frame_buffer.h:11: #ifndef
WEBRTC_COMMON_VIDEO_INCLUDE_VIDEO_FRAME_BUFFER_H_
Change name of this file to i420_buffer.h ?

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/libyuv/we...
File webrtc/common_video/libyuv/webrtc_libyuv.cc (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/libyuv/we...
webrtc/common_video/libyuv/webrtc_libyuv.cc:16: // NOTE(ajm): Path provided by
gyp.
This comment is wrong

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/rotation.h
File webrtc/common_video/rotation.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/rotation....
webrtc/common_video/rotation.h:14: #include "webrtc/api/video/rotation.h"
Add a TODO(nisse) to drop this file once downstream code is updated.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/video_fra...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/video_fra...
webrtc/common_video/video_frame.cc:11: #include "webrtc/video_frame.h"
Seems like a bunch of things here need to be cleaned up.

For instance, the file should be renamed to encoded_image.cc.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/video_fra...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/video_fra...
webrtc/common_video/video_frame_buffer.cc:1: /*
Rename file to i420_buffer.cc

https://codereview.webrtc.org/2517173004/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:32: #include
"webrtc/common_video/include/corevideo_frame_buffer.h"
Should corevideo_frame_buffer move here? The "sdk/"s should serve as tests for
the C++ API - that it provides the necessary "pins" on the chip - so no poking
around with internals should be required/allowed.

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

https://codereview.webrtc.org/2517173004/diff/1/webrtc/video_frame.h#newcode22
webrtc/video_frame.h:22: class EncodedImage {
This also need to be in api/? Did you intend to move+rename in another CL?

[nisse-webrtc, 2016-11-25 08:58:03]

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/android/jni/native...
File webrtc/api/android/jni/native_handle_impl.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/android/jni/native...
webrtc/api/android/jni/native_handle_impl.h:17: #include
"webrtc/api/video/rotation.h"
On 2016/11/24 10:36:59, the sun wrote:
> nit: include order

Done.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/android/jni/surfac...
File webrtc/api/android/jni/surfacetexturehelper_jni.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/android/jni/surfac...
webrtc/api/android/jni/surfacetexturehelper_jni.h:16: #include
"webrtc/api/video/video_frame_buffer.h"
On 2016/11/24 10:36:59, the sun wrote:
> nit: include order here as well (not going to comment again if there are more
of
> these - take a look across the changes and git cl format)

Done a git cl format, and manual review of include ordering.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/video/video_frame.h
File webrtc/api/video/video_frame.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/video/video_frame....
webrtc/api/video/video_frame.h:13: 
On 2016/11/24 10:36:59, the sun wrote:
> nit: add stdint.h

Done. (It was indirectly included, via video_frame_buffer.h).

https://codereview.webrtc.org/2517173004/diff/1/webrtc/api/video/video_frame....
webrtc/api/video/video_frame.h:17: // than forward declaring it. Right?
On 2016/11/24 10:36:59, the sun wrote:
> Agree.

Good. TODO comment deleted.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/include/v...
File webrtc/common_video/include/video_frame_buffer.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/include/v...
webrtc/common_video/include/video_frame_buffer.h:11: #ifndef
WEBRTC_COMMON_VIDEO_INCLUDE_VIDEO_FRAME_BUFFER_H_
On 2016/11/24 10:36:59, the sun wrote:
> Change name of this file to i420_buffer.h ? 

Since this cl is already pretty large, I'd prefer to leave cleanup of the code
which isn't moved around for later cls.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/libyuv/we...
File webrtc/common_video/libyuv/webrtc_libyuv.cc (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/libyuv/we...
webrtc/common_video/libyuv/webrtc_libyuv.cc:16: // NOTE(ajm): Path provided by
gyp.
On 2016/11/24 10:36:59, the sun wrote:
> This comment is wrong

Edited s/gyp/gn/.

BTW, I don't quite like the special handling of libyuv include paths in the
build files. It's needed for two reasons:

1. Internal includes of libyuv require it (which I find wrong, but we couldn't
reach agreement to land the fix in https://codereview.chromium.org/1977813002/).

2. We might want to support building webrtc with an out-of-tree libyuv, e.g.,
with headers installed in /usr/include.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/rotation.h
File webrtc/common_video/rotation.h (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/rotation....
webrtc/common_video/rotation.h:14: #include "webrtc/api/video/rotation.h"
On 2016/11/24 10:36:59, the sun wrote:
> Add a TODO(nisse) to drop this file once downstream code is updated.

Done.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/video_fra...
File webrtc/common_video/video_frame.cc (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/video_fra...
webrtc/common_video/video_frame.cc:11: #include "webrtc/video_frame.h"
On 2016/11/24 10:36:59, the sun wrote:
> Seems like a bunch of things here need to be cleaned up.
> 
> For instance, the file should be renamed to encoded_image.cc.

I'd prefer leaving that for later cls.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/video_fra...
File webrtc/common_video/video_frame_buffer.cc (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/common_video/video_fra...
webrtc/common_video/video_frame_buffer.cc:1: /*
On 2016/11/24 10:36:59, the sun wrote:
> Rename file to i420_buffer.cc

I think that should be done in this cl if and only if we decide to move the
I420Buffer class to api/video.

https://codereview.webrtc.org/2517173004/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2517173004/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:32: #include
"webrtc/common_video/include/corevideo_frame_buffer.h"
On 2016/11/24 10:36:59, the sun wrote:
> Should corevideo_frame_buffer move here? The "sdk/"s should serve as tests for
> the C++ API - that it provides the necessary "pins" on the chip - so no poking
> around with internals should be required/allowed.

I agree that makes some sense. But it's for someone more familiar with the objc
sdk to decide. (And doesn't have to be in this cl).

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

https://codereview.webrtc.org/2517173004/diff/1/webrtc/video_frame.h#newcode22
webrtc/video_frame.h:22: class EncodedImage {
On 2016/11/24 10:36:59, the sun wrote:
> This also need to be in api/? Did you intend to move+rename in another CL?

For now, I intend to move only interfaces related to unencoded video frames. 

The interfaces dealing with encoded frames is a mess, there's this class,
EncodedImage, which is quite ugly by itself. And then there's subclasses
VCMEncodedFrame, VCMFrameBuffer, and philipel's new FrameObject class used by
the new jitterbuffer.

And when it's passed around, it's accompanied by auxillary data represented by
CodecSpecificInfo and RtpFragmentationHeader.

This area desperately needs a refactoring effort, and I don't think it makes
sense to move anything to api/ in the current state.

[kjellander_webrtc, 2016-11-29 08:11:49]

Description was changed from

==========
Move VideoFrame and related declarations to webrtc/api/video.

Moves webrtc/common_video/rotation.h and parts of
webrtc/common_video/include/video_frame_buffer.h and
webrtc/video_frame.h, and adds to a new GN tarbet api:video_frame_api.

BUG=webrtc:5880
==========

to

==========
Move VideoFrame and related declarations to webrtc/api/video.

Moves webrtc/common_video/rotation.h and parts of
webrtc/common_video/include/video_frame_buffer.h and
webrtc/video_frame.h, and adds to a new GN target api:video_frame_api.

BUG=webrtc:5880
==========

[kjellander_webrtc, 2016-11-29 08:14:47]

lgtm. 
Do you plan to move EncodedImage (or rename webrtc/video_frame.h) in a future
CL?

[nisse-webrtc, 2016-11-29 08:26:22]

On 2016/11/29 08:14:47, kjellander_webrtc wrote:
> lgtm. 
> Do you plan to move EncodedImage (or rename webrtc/video_frame.h) in a future
> CL?

As I wrote in #5, I think EncodedImage and related classes really need a bit of
refactoring.
Don't know if/when that can be prioritized, but I'd prefer to not move to api/
in the 
current state. At least, we should have a plan for what want it to be,
documenting 
what we'd like to keep and what's deprecated, before moving.

[magjed_webrtc, 2016-11-29 15:43:49]

I also think we should move I420Buffer to webrtc/api/video/.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.cc (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:2: *  Copyright (c) 2014 The WebRTC project
authors. All Rights Reserved.
2016?

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:46: VideoFrame::~VideoFrame() {}
maybe '= default' is better?

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:71: ;
nit: remove lonely semicolon.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.h (left):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.h:106: return timestamp_us() /
rtc::kNumMicrosecsPerMillisec;
Why are some implementations (but not all) moved to the .cc file?

[nisse-webrtc, 2016-11-29 15:56:15]

Replying to some comments, no new patchset yes.

I'll get to moving I420Buffer, but I'd like to land
https://codereview.webrtc.org/2528143002/ first.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.cc (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:2: *  Copyright (c) 2014 The WebRTC project
authors. All Rights Reserved.
On 2016/11/29 15:43:49, magjed_webrtc wrote:
> 2016?

I just copied the copyright header along with the contents.

I think I've read somewhere that we don't try to keep copyright years accurate.
(Projects that do try, e.g., the GNU project, list all years when there have
been changes to the file).

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:46: VideoFrame::~VideoFrame() {}
On 2016/11/29 15:43:49, magjed_webrtc wrote:
> maybe '= default' is better?

Makes sense.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.h (left):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.h:106: return timestamp_us() /
rtc::kNumMicrosecsPerMillisec;
On 2016/11/29 15:43:49, magjed_webrtc wrote:
> Why are some implementations (but not all) moved to the .cc file?

I moved this one, because I wanted to avoid having to depend on timeutils.h in
the public header. Do you want me to move all implementations, for consistency?

[magjed_webrtc, 2016-11-29 16:40:26]

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.cc (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:2: *  Copyright (c) 2014 The WebRTC project
authors. All Rights Reserved.
On 2016/11/29 15:56:14, nisse-webrtc wrote:
> On 2016/11/29 15:43:49, magjed_webrtc wrote:
> > 2016?
> 
> I just copied the copyright header along with the contents.
> 
> I think I've read somewhere that we don't try to keep copyright years
accurate.
> (Projects that do try, e.g., the GNU project, list all years when there have
> been changes to the file).

The year has been changed from 2012 to 2014. This is technically a new file, so
I don't know if you should update the year to 2016 or leave it untouched.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.h (left):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.h:106: return timestamp_us() /
rtc::kNumMicrosecsPerMillisec;
On 2016/11/29 15:56:15, nisse-webrtc wrote:
> On 2016/11/29 15:43:49, magjed_webrtc wrote:
> > Why are some implementations (but not all) moved to the .cc file?
> 
> I moved this one, because I wanted to avoid having to depend on timeutils.h in
> the public header. Do you want me to move all implementations, for
consistency?

Ok, then I understand. No you can leave the other implementations untouched.

[pthatcher1, 2016-11-30 00:48:28]

lgtm, modulo some things that could be addressed in follow-up CLs, if we choose.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:346: "video/rotation.h",
We have video/video_x.h for everything else.  Wouldn't video/video_rotation.h be
more consistent?

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.cc (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:14: #include "webrtc/base/timeutils.h"
Does this need to eventually be moved to webrtc/api to avoid webrtc/api =>
webrtc/base dependencies?

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame_buffer.h:23: class VideoFrameBuffer : public
rtc::RefCountInterface {
For classes that are pure interfaces, can we make the naming convention of
XInterface?   For example, VideoFrameBufferInterface?

[kjellander_webrtc, 2016-11-30 07:19:12]

Replied only on the year discussion.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.cc (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:2: *  Copyright (c) 2014 The WebRTC project
authors. All Rights Reserved.
On 2016/11/29 16:40:26, magjed_webrtc wrote:
> On 2016/11/29 15:56:14, nisse-webrtc wrote:
> > On 2016/11/29 15:43:49, magjed_webrtc wrote:
> > > 2016?
> > 
> > I just copied the copyright header along with the contents.
> > 
> > I think I've read somewhere that we don't try to keep copyright years
> accurate.
> > (Projects that do try, e.g., the GNU project, list all years when there have
> > been changes to the file).
> 
> The year has been changed from 2012 to 2014. This is technically a new file,
so
> I don't know if you should update the year to 2016 or leave it untouched.

Year should be current year if it's a new file, then never changed:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
If it's a moved file, don't change it.

[nisse-webrtc, 2016-11-30 11:26:03]

Addressed nits. Will look into moving I420Buffer next.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/BUILD.gn
File webrtc/api/BUILD.gn (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/BUILD.gn#newco...
webrtc/api/BUILD.gn:346: "video/rotation.h",
On 2016/11/30 00:48:28, pthatcher1 wrote:
> We have video/video_x.h for everything else.  Wouldn't video/video_rotation.h
be
> more consistent?

I'm renaming it video/video_rotation.h, for consistency. It defines the (enum)
type VideoRotation.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame.cc (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:2: *  Copyright (c) 2014 The WebRTC project
authors. All Rights Reserved.
On 2016/11/30 07:19:12, kjellander_webrtc wrote:
> On 2016/11/29 16:40:26, magjed_webrtc wrote:
> > On 2016/11/29 15:56:14, nisse-webrtc wrote:
> > > On 2016/11/29 15:43:49, magjed_webrtc wrote:
> > > > 2016?
> > > 
> > > I just copied the copyright header along with the contents.
> > > 
> > > I think I've read somewhere that we don't try to keep copyright years
> > accurate.
> > > (Projects that do try, e.g., the GNU project, list all years when there
have
> > > been changes to the file).
> > 
> > The year has been changed from 2012 to 2014. This is technically a new file,
> so
> > I don't know if you should update the year to 2016 or leave it untouched.
> 
> Year should be current year if it's a new file, then never changed:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> If it's a moved file, don't change it.

It's moving parts of webrtc/video_frame.h, which has a 2012 copyright header
(while the header file is dated 2014). Does that count as a "new" file?

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:14: #include "webrtc/base/timeutils.h"
On 2016/11/30 00:48:28, pthatcher1 wrote:
> Does this need to eventually be moved to webrtc/api to avoid webrtc/api =>
> webrtc/base dependencies?  

This is the implementation file. As far as I understand dependencis on internal
things should be fine here.

That said, we've been discussing moving a stripped down timeutils.h to api/.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:46: VideoFrame::~VideoFrame() {}
On 2016/11/29 15:43:49, magjed_webrtc wrote:
> maybe '= default' is better?

Done.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame.cc:71: ;
On 2016/11/29 15:43:49, magjed_webrtc wrote:
> nit: remove lonely semicolon.

Done.

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
File webrtc/api/video/video_frame_buffer.h (right):

https://codereview.webrtc.org/2517173004/diff/20001/webrtc/api/video/video_fr...
webrtc/api/video/video_frame_buffer.h:23: class VideoFrameBuffer : public
rtc::RefCountInterface {
On 2016/11/30 00:48:28, pthatcher1 wrote:
> For classes that are pure interfaces, can we make the naming convention of
> XInterface?   For example, VideoFrameBufferInterface?

I see your point. I'd prefer to not change this name now. Maybe if we go a long
with Magnus' suggestion to remove the Data and Stride methods from the interface
(keeping only a (renamed) NativeToI420Buffer to provide access to the pixels),
that could be a reasonable point to also change the name.

[nisse-webrtc, 2016-11-30 12:56:54]

New version, moving I420Buffer, and tweaking the signature of the Rotate method.

What about SetToBlack, should we deprecate that too and define it as a static
method instead?

[nisse-webrtc, 2016-12-01 07:51:58]

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

[commit-bot: I haz the power, 2016-12-01 07:52:09]

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-12-01 07:53:36]

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

[commit-bot: I haz the power, 2016-12-01 07:53:37]

Dry run: Try jobs failed on following builders:
  android_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
  ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/12997)
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/13015)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15360)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15220)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20576)
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10905)

[the sun, 2016-12-05 22:36:29]

lgtm % comments

In particular, consider moving utilities out of I420Buffer to avoid pushing a
dep on libyuv all users of the video_frame_api target.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
File webrtc/api/video/i420_buffer.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
webrtc/api/video/i420_buffer.h:42: static rtc::scoped_refptr<I420Buffer>
Rotate(const VideoFrameBuffer& src,
IIUC most of these utility functions don't even need to be static members, they
can be implemented outside of the class because all fields they access are
exposed through member functions.

 suggest you either remove from the public API, or, if we really want to support
them, put in a separate namespace (e.g. i420buffer_utils). That namespace should
be in a separate build target, because then only users of those particular
functions need to depend on libyuv - a 'casual' user of I420Buffer won't need
it, and the class remains a 'stupid' POD-like buffer.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
webrtc/api/video/i420_buffer.h:63: uint8_t* MutableDataY();
Put overrides from base class together in one section, and these in another.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:161: // Applying
rotation is only supported for legacy reasons and performance is
Use I420Buffer::Rotate?

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
File webrtc/test/fake_texture_frame.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
webrtc/test/fake_texture_frame.h:42: buffer->InitializeData();
SetToBlack()? Do we really need both SetToBlack and InitializeData?

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/frame_utils.cc
File webrtc/test/frame_utils.cc (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/frame_utils....
webrtc/test/frame_utils.cc:11: #include <cstdio>
we generally use stdio.h etc

[nisse-webrtc, 2016-12-06 12:02:15]

Comments only, no updated patchset yet.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
File webrtc/api/video/i420_buffer.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
webrtc/api/video/i420_buffer.h:42: static rtc::scoped_refptr<I420Buffer>
Rotate(const VideoFrameBuffer& src,
On 2016/12/05 22:36:29, the sun wrote:
> IIUC most of these utility functions don't even need to be static members,
they
> can be implemented outside of the class because all fields they access are
> exposed through member functions.
> 
>  suggest you either remove from the public API, or, if we really want to
support
> them, put in a separate namespace (e.g. i420buffer_utils). 

I found webrtc::I420Buffer::Rotate to be a reasonable name. Precisely what do
you suggest instead? webrtc::i420buffer_utils::Rotate looks a bit clumsy to me.

> That namespace should
> be in a separate build target, because then only users of those particular
> functions need to depend on libyuv - a 'casual' user of I420Buffer won't need
> it, and the class remains a 'stupid' POD-like buffer.

Will it make any practical difference? I would expect almost any executable
which depends on video_frame_api to include at least one object (at the creation
or consumption end of the pipeline) which uses either some of these helper
functions, or uses libyuv directly.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
webrtc/api/video/i420_buffer.h:63: uint8_t* MutableDataY();
On 2016/12/05 22:36:29, the sun wrote:
> Put overrides from base class together in one section, and these in another.

Ok, will reorder.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:161: // Applying
rotation is only supported for legacy reasons and performance is
On 2016/12/05 22:36:29, the sun wrote:
> Use I420Buffer::Rotate?

Good catch, will do.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
File webrtc/test/fake_texture_frame.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
webrtc/test/fake_texture_frame.h:42: buffer->InitializeData();
On 2016/12/05 22:36:29, the sun wrote:
> SetToBlack()? Do we really need both SetToBlack and InitializeData?

Probably not. Do you want InitializeData deleted in this cl?

We also need to decide what to do about SetToBlack, if it should be converted
from a regular method to a static method or plain function.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/frame_utils.cc
File webrtc/test/frame_utils.cc (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/frame_utils....
webrtc/test/frame_utils.cc:11: #include <cstdio>
On 2016/12/05 22:36:29, the sun wrote:
> we generally use stdio.h etc

I see. There are only 8 other uses of cstdio.

[magjed_webrtc, 2016-12-06 16:52:14]

lgtm

[the sun, 2016-12-07 15:43:51]

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
File webrtc/api/video/i420_buffer.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
webrtc/api/video/i420_buffer.h:42: static rtc::scoped_refptr<I420Buffer>
Rotate(const VideoFrameBuffer& src,
On 2016/12/06 12:02:15, nisse-webrtc wrote:
> On 2016/12/05 22:36:29, the sun wrote:
> > IIUC most of these utility functions don't even need to be static members,
> they
> > can be implemented outside of the class because all fields they access are
> > exposed through member functions.
> > 
> >  suggest you either remove from the public API, or, if we really want to
> support
> > them, put in a separate namespace (e.g. i420buffer_utils). 
> 
> I found webrtc::I420Buffer::Rotate to be a reasonable name. Precisely what do
> you suggest instead? webrtc::i420buffer_utils::Rotate looks a bit clumsy to
me.
> 
> > That namespace should
> > be in a separate build target, because then only users of those particular
> > functions need to depend on libyuv - a 'casual' user of I420Buffer won't
need
> > it, and the class remains a 'stupid' POD-like buffer.
> 
> Will it make any practical difference? I would expect almost any executable
> which depends on video_frame_api to include at least one object (at the
creation
> or consumption end of the pipeline) which uses either some of these helper
> functions, or uses libyuv directly. 
> 

If it makes no practical difference, then leave it.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
File webrtc/test/fake_texture_frame.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
webrtc/test/fake_texture_frame.h:42: buffer->InitializeData();
On 2016/12/06 12:02:15, nisse-webrtc wrote:
> On 2016/12/05 22:36:29, the sun wrote:
> > SetToBlack()? Do we really need both SetToBlack and InitializeData?
> 
> Probably not. Do you want InitializeData deleted in this cl?

Not necessarily, but if you intend to delete it, don't use it here.

> We also need to decide what to do about SetToBlack, if it should be converted
> from a regular method to a static method or plain function.

If you keep Rotate() etc as static functions in the class, then make it one too.

[nisse-webrtc, 2016-12-13 15:08:02]

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
File webrtc/api/video/i420_buffer.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/api/video/i420_bu...
webrtc/api/video/i420_buffer.h:63: uint8_t* MutableDataY();
On 2016/12/05 22:36:29, the sun wrote:
> Put overrides from base class together in one section, and these in another.

Done.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:161: // Applying
rotation is only supported for legacy reasons and performance is
On 2016/12/06 12:02:15, nisse-webrtc wrote:
> On 2016/12/05 22:36:29, the sun wrote:
> > Use I420Buffer::Rotate?
> 
> Good catch, will do.

Done.

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
File webrtc/test/fake_texture_frame.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
webrtc/test/fake_texture_frame.h:42: buffer->InitializeData();
On 2016/12/07 15:43:51, the sun wrote:
> On 2016/12/06 12:02:15, nisse-webrtc wrote:
> > On 2016/12/05 22:36:29, the sun wrote:
> > > SetToBlack()? Do we really need both SetToBlack and InitializeData?
> > 
> > Probably not. Do you want InitializeData deleted in this cl?
> 
> Not necessarily, but if you intend to delete it, don't use it here.
> 
> > We also need to decide what to do about SetToBlack, if it should be
converted
> > from a regular method to a static method or plain function.
> 
> If you keep Rotate() etc as static functions in the class, then make it one
too.

Done. Have to keep the old method for compatibility with Chrome, so I named the
new static version SetBlack. Ok?

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/frame_utils.cc
File webrtc/test/frame_utils.cc (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/frame_utils....
webrtc/test/frame_utils.cc:11: #include <cstdio>
On 2016/12/06 12:02:15, nisse-webrtc wrote:
> On 2016/12/05 22:36:29, the sun wrote:
> > we generally use stdio.h etc
> 
> I see. There are only 8 other uses of cstdio.

Done.

[the sun, 2016-12-13 15:29:29]

still lgtm

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
File webrtc/test/fake_texture_frame.h (right):

https://codereview.webrtc.org/2517173004/diff/180001/webrtc/test/fake_texture...
webrtc/test/fake_texture_frame.h:42: buffer->InitializeData();
On 2016/12/13 15:08:02, nisse-webrtc wrote:
> On 2016/12/07 15:43:51, the sun wrote:
> > On 2016/12/06 12:02:15, nisse-webrtc wrote:
> > > On 2016/12/05 22:36:29, the sun wrote:
> > > > SetToBlack()? Do we really need both SetToBlack and InitializeData?
> > > 
> > > Probably not. Do you want InitializeData deleted in this cl?
> > 
> > Not necessarily, but if you intend to delete it, don't use it here.
> > 
> > > We also need to decide what to do about SetToBlack, if it should be
> converted
> > > from a regular method to a static method or plain function.
> > 
> > If you keep Rotate() etc as static functions in the class, then make it one
> too.
> 
> Done. Have to keep the old method for compatibility with Chrome, so I named
the
> new static version SetBlack. Ok?

Sure.

[nisse-webrtc, 2016-12-14 08:36:55]

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

[commit-bot: I haz the power, 2016-12-14 08:36:59]

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-12-14 08:38:49]

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

[commit-bot: I haz the power, 2016-12-14 08:38:50]

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

[nisse-webrtc, 2016-12-16 08:42:54]

https://codereview.webrtc.org/2517173004/diff/260001/webrtc/common_video/BUIL...
File webrtc/common_video/BUILD.gn (right):

https://codereview.webrtc.org/2517173004/diff/260001/webrtc/common_video/BUIL...
webrtc/common_video/BUILD.gn:68: deps += [ "$rtc_libyuv_dir" ]
Adding the same value to both deps and public_deps looks redundant?

[kjellander_webrtc, 2016-12-16 08:46:48]

https://codereview.webrtc.org/2517173004/diff/260001/webrtc/common_video/BUIL...
File webrtc/common_video/BUILD.gn (right):

https://codereview.webrtc.org/2517173004/diff/260001/webrtc/common_video/BUIL...
webrtc/common_video/BUILD.gn:68: deps += [ "$rtc_libyuv_dir" ]
On 2016/12/16 08:42:53, nisse-webrtc wrote:
> Adding the same value to both deps and public_deps looks redundant?

Yeah you're right. But it's like that all over the codebase so better clean that
up in a separate CL.

[nisse-webrtc, 2016-12-16 14:01:24]

Seems to work now. I had to add a couple of extra includes to not break
applications relying on indirect includes.

Otherwise, this is intended to be backwards compatible and not break things. So
I think I'll try to get it landed on Monday.

[nisse-webrtc, 2016-12-19 09:00:12]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-12-19 09:00:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, pthatcher@webrtc.org, magjed@webrtc.org,
solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2517173004/#ps330001 (title:
"Include base/timeutils.h from api/video_frame.h.")

[commit-bot: I haz the power, 2016-12-19 09:00:23]

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-12-19 09:03:58]

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

[commit-bot: I haz the power, 2016-12-19 09:03:59]

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

[nisse-webrtc, 2016-12-19 09:35:53]

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

[nisse-webrtc, 2016-12-19 09:35:54]

Magnus: This pretty large cl needs root owner's approval, and since it's about
video, I think you're the most appropriate one.

No intended change in behavior. Some breakage may happen in downstream code
which relies on indirect includes, but I've worked around all issues I've been
able to find.

[mflodman, 2016-12-22 06:10:37]

One minor comment, then LGTM.

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

https://codereview.webrtc.org/2517173004/diff/330001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame_buffer.h:18: // expect it to declare
I420Buffer. Delete after callers are updated.
Is it the same for video_rotation.h? If so, add that to the TODO. (I didn't see
rotation used here any longer).

[nisse-webrtc, 2016-12-22 09:47:08]

On 2016/12/22 06:10:37, mflodman wrote:
> One minor comment, then LGTM.
> 
>
https://codereview.webrtc.org/2517173004/diff/330001/webrtc/common_video/incl...
> File webrtc/common_video/include/video_frame_buffer.h (right):
> 
>
https://codereview.webrtc.org/2517173004/diff/330001/webrtc/common_video/incl...
> webrtc/common_video/include/video_frame_buffer.h:18: // expect it to declare
> I420Buffer. Delete after callers are updated.
> Is it the same for video_rotation.h? If so, add that to the TODO. (I didn't
see
> rotation used here any longer).

Thanks. I'm OOO today. Will have a look after the holidays.

[nisse-webrtc, 2017-01-09 11:41:18]

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

https://codereview.webrtc.org/2517173004/diff/330001/webrtc/common_video/incl...
webrtc/common_video/include/video_frame_buffer.h:18: // expect it to declare
I420Buffer. Delete after callers are updated.
On 2016/12/22 06:10:37, mflodman wrote:
> Is it the same for video_rotation.h? If so, add that to the TODO. (I didn't
see
> rotation used here any longer).

I'm trying to delete the video_rotation.h include, and see if that breaks the
trybots.

[nisse-webrtc, 2017-01-09 14:45:50]

I've tried applying the cl to my chromium tree. It builds fine, after the
final(?) transition hack added in patchset #21.

So I'm going to cross my fingers and try to get this landed.

[nisse-webrtc, 2017-01-09 14:46:16]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-01-09 14:46:17]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
solenberg@webrtc.org, kjellander@webrtc.org, pthatcher@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2517173004/#ps390001 (title:
"Another transition hack, to not break Chrome build.")

[commit-bot: I haz the power, 2017-01-09 14:46:23]

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, 2017-01-09 15:12:59]

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

[commit-bot: I haz the power, 2017-01-09 15:13:01]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL)

[nisse-webrtc, 2017-01-09 15:31:06]

https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:163: buffer =
I420Buffer::Rotate(buffer->NativeToI420Buffer(),
Magnus: Does this rebase with your rotation bugfix look right?

I'm not sure if the below swap of captured_width and captured_height is needed.
These are used for stats only, and it's unclear to me if they should be affected
by rotation.

[magjed_webrtc, 2017-01-10 13:48:37]

https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:163: buffer =
I420Buffer::Rotate(buffer->NativeToI420Buffer(),
On 2017/01/09 15:31:06, nisse-webrtc wrote:
> Magnus: Does this rebase with your rotation bugfix look right?
> 
> I'm not sure if the below swap of captured_width and captured_height is
needed.
> These are used for stats only, and it's unclear to me if they should be
affected
> by rotation.

It looks correct.

It's unclear for me as well how the captured_width and captured_height stats
should be reported when apply_rotation() is on, but I don't think it matters, so
don't change anything unrelated in this CL.

https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:165: if (rotation
!= kVideoRotation_180) {
Makes more sense to do:
if (rotation == kVideoRotation_90 || rotation == kVideoRotation_270) {
  std::swap(captured_width, captured_height);
}
now.

[nisse-webrtc, 2017-01-10 14:09:00]

https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):

https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:165: if (rotation
!= kVideoRotation_180) {
On 2017/01/10 13:48:37, magjed_webrtc wrote:
> Makes more sense to do:
> if (rotation == kVideoRotation_90 || rotation == kVideoRotation_270) {
>   std::swap(captured_width, captured_height);
> }
> now.

Should be equivalent (since rotation 0 is excluded earlier). I liked the more
concise version, but I'll change it if you think your version is more readable.

[nisse-webrtc, 2017-01-10 14:09:23]

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

[commit-bot: I haz the power, 2017-01-10 14:09:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[magjed_webrtc, 2017-01-10 14:24:05]

On 2017/01/10 14:09:00, nisse-webrtc wrote:
>
https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
> File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right):
> 
>
https://codereview.webrtc.org/2517173004/diff/410001/webrtc/sdk/objc/Framewor...
> webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:165: if
(rotation
> != kVideoRotation_180) {
> On 2017/01/10 13:48:37, magjed_webrtc wrote:
> > Makes more sense to do:
> > if (rotation == kVideoRotation_90 || rotation == kVideoRotation_270) {
> >   std::swap(captured_width, captured_height);
> > }
> > now.
> 
> Should be equivalent (since rotation 0 is excluded earlier). I liked the more
> concise version, but I'll change it if you think your version is more
readable.

Yes, I think it's more readable and less brittle to not rely on the context and
do a full comparison on all the enum values.

[nisse-webrtc, 2017-01-10 15:15:10]

The CQ bit was unchecked by nisse@webrtc.org

[nisse-webrtc, 2017-01-10 15:15:10]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-01-10 15:15:11]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
solenberg@webrtc.org, kjellander@webrtc.org, pthatcher@webrtc.org,
mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2517173004/#ps430001 (title:
"Make rotation check clearer.")

[commit-bot: I haz the power, 2017-01-10 15:15:22]

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, 2017-01-10 15:44:28]

CQ is committing da patch.

Bot data: {"patchset_id": 430001, "attempt_start_ts": 1484061310919640,
"parent_rev": "658024ee92b27cad66af90b366842ae8cc6cd624", "commit_rev":
"af916899cc8ec685ebeaf414d5aa86ca83e1fca6"}

[commit-bot: I haz the power, 2017-01-10 15:44:32]

Description was changed from

==========
Move VideoFrame and related declarations to webrtc/api/video.

Moves webrtc/common_video/rotation.h and parts of
webrtc/common_video/include/video_frame_buffer.h and
webrtc/video_frame.h, and adds to a new GN target api:video_frame_api.

BUG=webrtc:5880
==========

to

==========
Move VideoFrame and related declarations to webrtc/api/video.

Moves webrtc/common_video/rotation.h and parts of
webrtc/common_video/include/video_frame_buffer.h and
webrtc/video_frame.h, and adds to a new GN target api:video_frame_api.

BUG=webrtc:5880

Review-Url: https://codereview.webrtc.org/2517173004
Cr-Commit-Position: refs/heads/master@{#15993}
Committed:
https://chromium.googlesource.com/external/webrtc/+/af916899cc8ec685ebeaf414d...
==========

[commit-bot: I haz the power, 2017-01-10 15:44:34]

Committed patchset #23 (id:430001) as
https://chromium.googlesource.com/external/webrtc/+/af916899cc8ec685ebeaf414d...