Clean up RTCVideoFrame

Clean up RTCVideoFrame

RTCVideoFrame is an ObjectiveC version of webrtc::VideoFrame, but it
currently contains some extra logic beyond that. We want RTCVideoFrame
to be as simple as possible, i.e. just a container with no extra state,
so we can use it as input to RTCVideoSource without complicating the
interface for consumers.

BUG=webrtc:7177
NOTRY=True

Review-Url: https://codereview.webrtc.org/2695203004
Cr-Commit-Position: refs/heads/master@{#16740}
Committed: https://chromium.googlesource.com/external/webrtc/+/7ee512581c589b40302e67140a2c426edcd40249

[magjed_webrtc, 2017-02-16 21:35:08]

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

[commit-bot: I haz the power, 2017-02-16 21:35:18]

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, 2017-02-16 22:41:56]

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

[commit-bot: I haz the power, 2017-02-16 22:41:57]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-02-17 10:31:20]

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

[commit-bot: I haz the power, 2017-02-17 10:31:23]

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-02-17 11:57:52]

Description was changed from

==========
Simplify RTCVideoFrame

BUG=None
==========

to

==========
Clean up RTCVideoFrame

RTCVideoFrame is an ObjectiveC version of webrtc::VideoFrame, but it currently
contains some extra logic beyond that. We want RTCVideoFrame to be as simple as
possible, i.e. just a container with no extra state, so we can use it as input
to RTCVideoSource without complicating the interface for consumers.

BUG=webrtc:7177
==========

[magjed_webrtc, 2017-02-17 12:03:00]

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

[magjed_webrtc, 2017-02-17 12:03:05]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-17 12:03:09]

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-02-17 12:03:13]

Patchset #3 (id:60001) has been deleted

[magjed_webrtc, 2017-02-17 12:03:21]

Patchset #2 (id:40001) has been deleted

[magjed_webrtc, 2017-02-17 12:03:33]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2017-02-17 12:04:18]

Description was changed from

==========
Clean up RTCVideoFrame

RTCVideoFrame is an ObjectiveC version of webrtc::VideoFrame, but it currently
contains some extra logic beyond that. We want RTCVideoFrame to be as simple as
possible, i.e. just a container with no extra state, so we can use it as input
to RTCVideoSource without complicating the interface for consumers.

BUG=webrtc:7177
==========

to

==========
Clean up RTCVideoFrame

RTCVideoFrame is an ObjectiveC version of webrtc::VideoFrame, but it
currently contains some extra logic beyond that. We want RTCVideoFrame
to be as simple as possible, i.e. just a container with no extra state,
so we can use it as input to RTCVideoSource without complicating the
interface for consumers.

BUG=webrtc:7177
==========

[magjed_webrtc, 2017-02-17 12:15:33]

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

[commit-bot: I haz the power, 2017-02-17 12:15:39]

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-02-17 12:15:39]

Patchset #1 (id:80001) has been deleted

[magjed_webrtc, 2017-02-17 12:16:27]

magjed@webrtc.org changed reviewers:
+ denicija@webrtc.org

[magjed_webrtc, 2017-02-17 12:16:38]

Daniela - please take a look.

[commit-bot: I haz the power, 2017-02-17 14:00:56]

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

[commit-bot: I haz the power, 2017-02-17 14:00:57]

Dry run: This issue passed the CQ dry run.

[daniela-webrtc, 2017-02-17 16:26:34]

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/RTCVideoFrame+Private.h (left):

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/RTCVideoFrame+Private.h:25:
rotation:(webrtc::VideoRotation)rotation
I think in general using Enums is better approach than int. Was there any
particular reason why you decided to remove this?

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/RTCVideoFrame.mm (right):

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/RTCVideoFrame.mm:63: -
(RTCVideoFrame*)convertToI420 {
This method might not be safe from memory management perspective since it's
allocating new memory and might cause memory leaks (case in, point this patch
increases the memory usage in Mac AppRTCMobile for several GBs).
I'd suggest thinking of:
1. Renaming this method to something like newI420Frame or similar 
Source:
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Me...

2. Go trough this document
http://clang.llvm.org/docs/AutomaticReferenceCounting.html
and see if there is a reasonable ownership qualifier that can be applied to the
return pointer to ease memory management.

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoFrame.h (right):

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoFrame.h:49: -
(RTCVideoFrame*)convertToI420;
Nit: pointer spacing, here and in other places as well.
RTCVideoFrame* -> RTCVideoFrame *

[daniela-webrtc, 2017-02-20 12:17:27]

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/RTCVideoFrame.mm (right):

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/RTCVideoFrame.mm:63: -
(RTCVideoFrame*)convertToI420 {
On 2017/02/17 16:26:34, denicija-webrtc wrote:
> This method might not be safe from memory management perspective since it's
> allocating new memory and might cause memory leaks (case in, point this patch
> increases the memory usage in Mac AppRTCMobile for several GBs).
> I'd suggest thinking of:
> 1. Renaming this method to something like newI420Frame or similar 
> Source:
>
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Me...
> 
> 2. Go trough this document
> http://clang.llvm.org/docs/AutomaticReferenceCounting.html
> and see if there is a reasonable ownership qualifier that can be applied to
the
> return pointer to ease memory management.

Ok, I've done bit more thinking about this. I think the best way forward to be
to have this method always return new instance of RTCVideoFrame and name the
method as newI420VideoFrame.
We can keep the check for the native_handle() and use the NativeToI420
conversion only when needed. 
The creation of the new object *should* not take away too much from the
optimization

[magjed_webrtc, 2017-02-20 14:45:43]

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

[commit-bot: I haz the power, 2017-02-20 14:45:53]

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, 2017-02-20 14:49:44]

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

[commit-bot: I haz the power, 2017-02-20 14:49:45]

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

[magjed_webrtc, 2017-02-20 16:02:50]

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

[commit-bot: I haz the power, 2017-02-20 16:02:53]

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-02-20 16:03:14]

Patchset #2 (id:120001) has been deleted

[magjed_webrtc, 2017-02-20 16:10:31]

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

[commit-bot: I haz the power, 2017-02-20 16:10:43]

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-02-20 16:11:42]

Patchset #2 (id:140001) has been deleted

[magjed_webrtc, 2017-02-20 16:16:44]

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/RTCVideoFrame+Private.h (left):

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/RTCVideoFrame+Private.h:25:
rotation:(webrtc::VideoRotation)rotation
On 2017/02/17 16:26:34, denicija-webrtc wrote:
> I think in general using Enums is better approach than int. Was there any
> particular reason why you decided to remove this?

It caused so many static casts in the implementation file. The reason we can't
use webrtc::VideoRotation in the public header is because it's C++ (because of
the namespace). It leads to this error:
../../webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m -o
obj/webrtc/sdk/objc/Framework/Classes/rtc_sdk_peerconnection_objc.RTCEAGLVideoView.o
In file included from
../../webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:16:
In file included from
../../webrtc/sdk/objc/Framework/Headers/WebRTC//RTCVideoFrame.h:16:
../../webrtc/common_video/rotation.h:14:1: error: unknown type name 'namespace'
namespace webrtc {

I added a corresponding pure ObjC enum instead now.

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/RTCVideoFrame.mm (right):

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/RTCVideoFrame.mm:63: -
(RTCVideoFrame*)convertToI420 {
On 2017/02/20 12:17:27, denicija-webrtc wrote:
> On 2017/02/17 16:26:34, denicija-webrtc wrote:
> > This method might not be safe from memory management perspective since it's
> > allocating new memory and might cause memory leaks (case in, point this
patch
> > increases the memory usage in Mac AppRTCMobile for several GBs).
> > I'd suggest thinking of:
> > 1. Renaming this method to something like newI420Frame or similar 
> > Source:
> >
>
https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Me...
> > 
> > 2. Go trough this document
> > http://clang.llvm.org/docs/AutomaticReferenceCounting.html
> > and see if there is a reasonable ownership qualifier that can be applied to
> the
> > return pointer to ease memory management.
> 
> Ok, I've done bit more thinking about this. I think the best way forward to be
> to have this method always return new instance of RTCVideoFrame and name the
> method as newI420VideoFrame.
> We can keep the check for the native_handle() and use the NativeToI420
> conversion only when needed. 
> The creation of the new object *should* not take away too much from the
> optimization

Done.

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoFrame.h (right):

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Headers/WebRTC/RTCVideoFrame.h:49: -
(RTCVideoFrame*)convertToI420;
On 2017/02/17 16:26:34, denicija-webrtc wrote:
> Nit: pointer spacing, here and in other places as well.
> RTCVideoFrame* -> RTCVideoFrame *

Done.

[commit-bot: I haz the power, 2017-02-20 18:11:21]

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

[commit-bot: I haz the power, 2017-02-20 18:11:22]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_compile_mips_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_compile_x86_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build
URL)
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_more_configs on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[daniela-webrtc, 2017-02-21 08:30:18]

LGTM

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
File webrtc/sdk/objc/Framework/Classes/RTCVideoFrame+Private.h (left):

https://codereview.webrtc.org/2695203004/diff/100001/webrtc/sdk/objc/Framewor...
webrtc/sdk/objc/Framework/Classes/RTCVideoFrame+Private.h:25:
rotation:(webrtc::VideoRotation)rotation
On 2017/02/20 16:16:44, magjed_webrtc wrote:
> On 2017/02/17 16:26:34, denicija-webrtc wrote:
> > I think in general using Enums is better approach than int. Was there any
> > particular reason why you decided to remove this?
> 
> It caused so many static casts in the implementation file. The reason we can't
> use webrtc::VideoRotation in the public header is because it's C++ (because of
> the namespace). It leads to this error:
> ../../webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m -o
>
obj/webrtc/sdk/objc/Framework/Classes/rtc_sdk_peerconnection_objc.RTCEAGLVideoView.o
> In file included from
> ../../webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:16:
> In file included from
> ../../webrtc/sdk/objc/Framework/Headers/WebRTC//RTCVideoFrame.h:16:
> ../../webrtc/common_video/rotation.h:14:1: error: unknown type name
'namespace'
> namespace webrtc {
> 
> I added a corresponding pure ObjC enum instead now.

Acknowledged.

[magjed_webrtc, 2017-02-21 11:52:49]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-02-21 11:52:56]

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

[magjed_webrtc, 2017-02-21 12:16:42]

The CQ bit was unchecked by magjed@webrtc.org

[magjed_webrtc, 2017-02-21 12:16:49]

Description was changed from

==========
Clean up RTCVideoFrame

RTCVideoFrame is an ObjectiveC version of webrtc::VideoFrame, but it
currently contains some extra logic beyond that. We want RTCVideoFrame
to be as simple as possible, i.e. just a container with no extra state,
so we can use it as input to RTCVideoSource without complicating the
interface for consumers.

BUG=webrtc:7177
==========

to

==========
Clean up RTCVideoFrame

RTCVideoFrame is an ObjectiveC version of webrtc::VideoFrame, but it
currently contains some extra logic beyond that. We want RTCVideoFrame
to be as simple as possible, i.e. just a container with no extra state,
so we can use it as input to RTCVideoSource without complicating the
interface for consumers.

BUG=webrtc:7177
NOTRY=True
==========

[magjed_webrtc, 2017-02-21 12:16:58]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2017-02-21 12:17:03]

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-02-21 12:19:47]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487679418006940,
"parent_rev": "a518a39963d34616d8f0e94991c7f5fbb5affb38", "commit_rev":
"7ee512581c589b40302e67140a2c426edcd40249"}

[commit-bot: I haz the power, 2017-02-21 12:19:49]

Description was changed from

==========
Clean up RTCVideoFrame

RTCVideoFrame is an ObjectiveC version of webrtc::VideoFrame, but it
currently contains some extra logic beyond that. We want RTCVideoFrame
to be as simple as possible, i.e. just a container with no extra state,
so we can use it as input to RTCVideoSource without complicating the
interface for consumers.

BUG=webrtc:7177
NOTRY=True
==========

to

==========
Clean up RTCVideoFrame

RTCVideoFrame is an ObjectiveC version of webrtc::VideoFrame, but it
currently contains some extra logic beyond that. We want RTCVideoFrame
to be as simple as possible, i.e. just a container with no extra state,
so we can use it as input to RTCVideoSource without complicating the
interface for consumers.

BUG=webrtc:7177
NOTRY=True

Review-Url: https://codereview.webrtc.org/2695203004
Cr-Commit-Position: refs/heads/master@{#16740}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7ee512581c589b40302e67140...
==========

[commit-bot: I haz the power, 2017-02-21 12:19:50]

Committed patchset #2 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/7ee512581c589b40302e67140...