ObjC: Remove RTCOpenGLVideoRenderer

ObjC: Remove RTCOpenGLVideoRenderer

RTCOpenGLVideoRenderer is currently shared between RTCEAGLVideoView and
RTCNSGLVideoView, and contains some iOS specific code and some Mac
specific code. We plan on adding an interface for injectable OpenGL ES
shaders to RTCEAGLVideoView, and it's easier if RTCEAGLVideoView and
RTCNSGLVideoView have separate code paths. This CL removes
RTCOpenGLVideoRenderer and inlines the necessary code in
RTCEAGLVideoView and RTCNSGLVideoView.

BUG=webrtc:7473
Review-Url: https://codereview.webrtc.org/2812613003
Cr-Commit-Position: refs/heads/master@{#17693}
Committed: https://chromium.googlesource.com/external/webrtc/+/2f7f9b8dad20d98cc2f17d707abb7cab72ab291d

[magjed_webrtc, 2017-04-11 10:34:56]

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

[commit-bot: I haz the power, 2017-04-11 10:34:59]

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-04-11 10:38:54]

Description was changed from

==========
ObjC: Remove RTCOpenGLVideoRenderer

RTCOpenGLVideoRenderer is currently shared between RTCEAGLVideoView and
RTCNSGLVideoView, and contains some iOS specific code and some Mac
specific code. We plan on adding an interface for injectable OpenGL ES
shaders to RTCEAGLVideoView, and it's easier if RTCEAGLVideoView and
RTCNSGLVideoView have separate code paths. This CL removes
RTCOpenGLVideoRenderer and inlines the necessary code in
RTCEAGLVideoView and RTCNSGLVideoView.

BUG=webrtc:7473
==========

to

==========
ObjC: Remove RTCOpenGLVideoRenderer

RTCOpenGLVideoRenderer is currently shared between RTCEAGLVideoView and
RTCNSGLVideoView, and contains some iOS specific code and some Mac
specific code. We plan on adding an interface for injectable OpenGL ES
shaders to RTCEAGLVideoView, and it's easier if RTCEAGLVideoView and
RTCNSGLVideoView have separate code paths. This CL removes
RTCOpenGLVideoRenderer and inlines the necessary code in
RTCEAGLVideoView and RTCNSGLVideoView.

BUG=webrtc:7473
==========

[magjed_webrtc, 2017-04-11 10:38:54]

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

[magjed_webrtc, 2017-04-11 10:39:05]

Please take a look.

[commit-bot: I haz the power, 2017-04-11 10:57:49]

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

[commit-bot: I haz the power, 2017-04-11 10:57:49]

Dry run: This issue passed the CQ dry run.

[sakal, 2017-04-11 11:00:21]

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m (right):

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:100: BOOL _isInitialized;
Why is this only needed on iOS and not on Mac?

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:206: if (!_isInitialized ||
!frame || frame == _lastDrawnFrame) {
Is this something we expect to happen or should we log here?

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:223: if (shader && [shader
drawFrame:frame]) {
Log on failure?

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:267: if (!_isInitialized) {
I don't like the way isInitialized is used. It seems to only refer to parts of
the class. Can it be changed to affect the whole class? i.e. check if
if(_isInitialized) and return at the start. Same for teardown.

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCOpenGLVideoRenderer.mm (left):

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCOpenGLVideoRenderer.mm:27:
glDisable(GL_DITHER);
This is not in the new code?

[magjed_webrtc, 2017-04-11 12:05:43]

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m (right):

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:100: BOOL _isInitialized;
On 2017/04/11 11:00:21, sakal wrote:
> Why is this only needed on iOS and not on Mac?

It was obvious from the code that it wasn't necessary on Mac so I removed it. I
want to be careful and not make any functional changes, so I kept it for iOS. I
looked into it more now, and it looks like it's possible to remove for iOS as
well.

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:206: if (!_isInitialized ||
!frame || frame == _lastDrawnFrame) {
On 2017/04/11 11:00:21, sakal wrote:
> Is this something we expect to happen or should we log here?

Yes, it can happen that we try to render the same frame multiple times.

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:223: if (shader && [shader
drawFrame:frame]) {
On 2017/04/11 11:00:21, sakal wrote:
> Log on failure?

We didn't do it previously, but I guess we can add a log. We will log closer to
the actual failure as well when drawFrame fails.

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:267: if (!_isInitialized) {
On 2017/04/11 11:00:21, sakal wrote:
> I don't like the way isInitialized is used. It seems to only refer to parts of
> the class. Can it be changed to affect the whole class? i.e. check if
> if(_isInitialized) and return at the start. Same for teardown.

It looks like we don't need it at all.

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
File webrtc/sdk/objc/Framework/Classes/RTCOpenGLVideoRenderer.mm (left):

https://codereview.webrtc.org/2812613003/diff/1/webrtc/sdk/objc/Framework/Cla...
webrtc/sdk/objc/Framework/Classes/RTCOpenGLVideoRenderer.mm:27:
glDisable(GL_DITHER);
On 2017/04/11 11:00:21, sakal wrote:
> This is not in the new code?

Hmm, I thought it wasn't used, but apparently +initialize is a class method, run
before any instances of the class are created and before other class methods are
run. I don't think it worked properly previously because the correct GL context
must be set in order for this to work. I moved it to where we setup/prepare
OpenGL now.

[sakal, 2017-04-11 12:21:52]

lgtm

[daniela-webrtc, 2017-04-12 08:46:27]

https://codereview.webrtc.org/2812613003/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m (right):

https://codereview.webrtc.org/2812613003/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:215: NSAssert(_nv12Shader,
@"couldn't create nv12 shader");
Do you think assertion is a good choice here? Usually we assert to indicate that
a design contract has been violated by the developer or the api has been used in
wrong manner etc. Do we want to assert when our stack has failed to properly
initialize?

https://codereview.webrtc.org/2812613003/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCNSGLVideoView.m (right):

https://codereview.webrtc.org/2812613003/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCNSGLVideoView.m:45: id<RTCShader>
_i420Shader;
Should the shader be atomic property?

[magjed_webrtc, 2017-04-12 13:29:06]

https://codereview.webrtc.org/2812613003/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m (right):

https://codereview.webrtc.org/2812613003/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCEAGLVideoView.m:215: NSAssert(_nv12Shader,
@"couldn't create nv12 shader");
On 2017/04/12 08:46:27, daniela-webrtc wrote:
> Do you think assertion is a good choice here? Usually we assert to indicate
that
> a design contract has been violated by the developer or the api has been used
in
> wrong manner etc. Do we want to assert when our stack has failed to properly
> initialize?

My thinking was that not being able to render video is essentially a fatal error
for a video call, and this assert makes it easier for us to debug if it happens.
But I changed it back to the previous behavior of just dropping frames, and
added logs to the i420 and nv12 shader init functions if they fail.

https://codereview.webrtc.org/2812613003/diff/20001/webrtc/sdk/objc/Framework...
File webrtc/sdk/objc/Framework/Classes/RTCNSGLVideoView.m (right):

https://codereview.webrtc.org/2812613003/diff/20001/webrtc/sdk/objc/Framework...
webrtc/sdk/objc/Framework/Classes/RTCNSGLVideoView.m:45: id<RTCShader>
_i420Shader;
On 2017/04/12 08:46:27, daniela-webrtc wrote:
> Should the shader be atomic property?

Good catch. Yes, it looks necessary.

[daniela-webrtc, 2017-04-12 14:23:18]

lgtm

[magjed_webrtc, 2017-04-12 17:03:49]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2017-04-12 17:03:50]

The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2812613003/#ps40001 (title:
"Remove NSAsserts and log instead. Also make i420Shader on Mac atomic.")

[commit-bot: I haz the power, 2017-04-12 17:04:00]

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-04-12 17:06:49]

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

[commit-bot: I haz the power, 2017-04-12 17:06:50]

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/23619)

[magjed_webrtc, 2017-04-13 08:56:25]

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

[commit-bot: I haz the power, 2017-04-13 08:56:31]

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-04-13 09:02:22]

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

[commit-bot: I haz the power, 2017-04-13 09:02:23]

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

[magjed_webrtc, 2017-04-13 09:31:37]

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

[commit-bot: I haz the power, 2017-04-13 09:31:46]

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-04-13 09:32:04]

Patchset #5 (id:80001) has been deleted

[magjed_webrtc, 2017-04-13 09:32:16]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-04-13 09:50:44]

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

[commit-bot: I haz the power, 2017-04-13 09:50:44]

Dry run: This issue passed the CQ dry run.

[magjed_webrtc, 2017-04-13 11:13:18]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2017-04-13 11:13:19]

The patchset sent to the CQ was uploaded after l-g-t-m from sakal@webrtc.org,
denicija@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2812613003/#ps100001 (title:
"Fix pointer typo and add include.")

[commit-bot: I haz the power, 2017-04-13 11:13: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-04-13 11:15:54]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492081998536920,
"parent_rev": "57742b2d14a15c4b4aa5a4bb47851a5fae758309", "commit_rev":
"2f7f9b8dad20d98cc2f17d707abb7cab72ab291d"}

[commit-bot: I haz the power, 2017-04-13 11:15:58]

Description was changed from

==========
ObjC: Remove RTCOpenGLVideoRenderer

RTCOpenGLVideoRenderer is currently shared between RTCEAGLVideoView and
RTCNSGLVideoView, and contains some iOS specific code and some Mac
specific code. We plan on adding an interface for injectable OpenGL ES
shaders to RTCEAGLVideoView, and it's easier if RTCEAGLVideoView and
RTCNSGLVideoView have separate code paths. This CL removes
RTCOpenGLVideoRenderer and inlines the necessary code in
RTCEAGLVideoView and RTCNSGLVideoView.

BUG=webrtc:7473
==========

to

==========
ObjC: Remove RTCOpenGLVideoRenderer

RTCOpenGLVideoRenderer is currently shared between RTCEAGLVideoView and
RTCNSGLVideoView, and contains some iOS specific code and some Mac
specific code. We plan on adding an interface for injectable OpenGL ES
shaders to RTCEAGLVideoView, and it's easier if RTCEAGLVideoView and
RTCNSGLVideoView have separate code paths. This CL removes
RTCOpenGLVideoRenderer and inlines the necessary code in
RTCEAGLVideoView and RTCNSGLVideoView.

BUG=webrtc:7473

Review-Url: https://codereview.webrtc.org/2812613003
Cr-Commit-Position: refs/heads/master@{#17693}
Committed:
https://chromium.googlesource.com/external/webrtc/+/2f7f9b8dad20d98cc2f17d707...
==========

[commit-bot: I haz the power, 2017-04-13 11:15:59]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/2f7f9b8dad20d98cc2f17d707...