SurfaceViewRenderer: Add resource name to log outputs and exceptions

SurfaceViewRenderer: Add resource name to log outputs and exceptions

Add resource name to log outputs to distinguish local renderer from remote renderer.

This Cl also adds some thread checks and factors out a small helper function makeBlack().

Committed: https://crrev.com/b2d1c5026dc3486670d2ffc7f663be3265bf18b9
Cr-Commit-Position: refs/heads/master@{#10596}

[magjed_webrtc, 2015-10-29 17:07:29]

Description was changed from

==========
SurfaceViewRenderer: Add resource name to log outputs and exceptions

Add resource name to log outputs to distinguish local renderer from remote
renderer.

This Cl also adds some thread checks and factors out a small helper function
makeBlack().
==========

to

==========
SurfaceViewRenderer: Add resource name to log outputs and exceptions

Add resource name to log outputs to distinguish local renderer from remote
renderer.

This Cl also adds some thread checks and factors out a small helper function
makeBlack().
==========

[magjed_webrtc, 2015-10-29 17:07:29]

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

[magjed_webrtc, 2015-11-09 18:04:03]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2015-11-09 18:04:16]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-11-09 18:05:40]

hbos - Please take a look.

[hbos, 2015-11-10 14:53:17]

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:385: return
(getId() == NO_ID) ? "" : getResources().getResourceEntryName(getId()) + ". ";
nit: End with ": " instead of ". "?
(How does a resource name look like? Is it unique?)

Doesn't everything have an ID and a resource entry name?
- Will getId() ever be NO_ID? When/why? If there are other/related reasons why
it might now have a resource entry name even if getId returns something, can
this ever throw Resources.NotFoundException in practice?

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:393:
eglBase.swapBuffers();
This only makes it black under the assumption that glClearColor is set to black,
I believe? Either rename to clearColors or something or set glClearColor to
black.

[magjed_webrtc, 2015-11-10 15:18:46]

hbos - Please take another look.

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:385: return
(getId() == NO_ID) ? "" : getResources().getResourceEntryName(getId()) + ". ";
On 2015/11/10 14:53:16, hbos wrote:
> nit: End with ": " instead of ". "?
> (How does a resource name look like? Is it unique?)
> 
> Doesn't everything have an ID and a resource entry name?
> - Will getId() ever be NO_ID? When/why? If there are other/related reasons why
> it might now have a resource entry name even if getId returns something, can
> this ever throw Resources.NotFoundException in practice?

Done, changed from '.' to ':'.

A resource name is the string you set in the layout xml file, e.g. in AppRTCDemo
activity_call.xml:
<org.webrtc.SurfaceViewRenderer
    android:id="@+id/local_video_view"
and getResourceEntryName() will return "local_video_view". If you don't set an
id in the xml file, then it will be NO_ID. I think NO_ID and
Resources.NotFoundException catch the same problem, but I can try/catch instead
to be sure.

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:393:
eglBase.swapBuffers();
On 2015/11/10 14:53:16, hbos wrote:
> This only makes it black under the assumption that glClearColor is set to
black,
> I believe? Either rename to clearColors or something or set glClearColor to
> black.

This class has a private EGLContext, and glClearColor() has this official
documentation:
"Specify the red, green, blue, and alpha values used when the color buffers are
cleared. The initial values are all 0."
But maybe better to make it explicit.

[hbos, 2015-11-10 15:39:18]

lgtm

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java (right):

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:385: return
(getId() == NO_ID) ? "" : getResources().getResourceEntryName(getId()) + ". ";
On 2015/11/10 15:18:46, magjed_webrtc wrote:
> On 2015/11/10 14:53:16, hbos wrote:
> > nit: End with ": " instead of ". "?
> > (How does a resource name look like? Is it unique?)
> > 
> > Doesn't everything have an ID and a resource entry name?
> > - Will getId() ever be NO_ID? When/why? If there are other/related reasons
why
> > it might now have a resource entry name even if getId returns something, can
> > this ever throw Resources.NotFoundException in practice?
> 
> Done, changed from '.' to ':'.
> 
> A resource name is the string you set in the layout xml file, e.g. in
AppRTCDemo
> activity_call.xml:
> <org.webrtc.SurfaceViewRenderer
>     android:id="@+id/local_video_view"
> and getResourceEntryName() will return "local_video_view". If you don't set an
> id in the xml file, then it will be NO_ID. I think NO_ID and
> Resources.NotFoundException catch the same problem, but I can try/catch
instead
> to be sure.

Acknowledged.

https://codereview.webrtc.org/1420203003/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/SurfaceViewRenderer.java:393:
eglBase.swapBuffers();
On 2015/11/10 15:18:46, magjed_webrtc wrote:
> On 2015/11/10 14:53:16, hbos wrote:
> > This only makes it black under the assumption that glClearColor is set to
> black,
> > I believe? Either rename to clearColors or something or set glClearColor to
> > black.
> 
> This class has a private EGLContext, and glClearColor() has this official
> documentation:
> "Specify the red, green, blue, and alpha values used when the color buffers
are
> cleared. The initial values are all 0."
> But maybe better to make it explicit.

Acknowledged.

[magjed_webrtc, 2015-11-10 16:28:00]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2015-11-10 16:28:06]

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

[commit-bot: I haz the power, 2015-11-10 16:32:40]

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

[commit-bot: I haz the power, 2015-11-10 16:32:41]

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

[magjed_webrtc, 2015-11-11 11:00:38]

The CQ bit was checked by magjed@webrtc.org

[commit-bot: I haz the power, 2015-11-11 11:00:46]

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

[commit-bot: I haz the power, 2015-11-11 11:06:45]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-11-11 11:06:57]

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