Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175913002:
[Remoting Android] Make EglThreadContext create OpenGL ES 3 context (Closed)
Description[Remoting Android] Make EglThreadContext create OpenGL ES 3 context
Currently EglThreadContext only creates OpenGL ES 2 context. OpenGL ES 3 is
backward compatible with ES 2 and has new interfaces (e.g. GL_PACK_ROW_LENGTH)
that can improve the performance of the client OpenGL renderer.
This CL makes EglThreadContext create OpenGL ES context of the highest
version supported by the software or hardward of the device.
BUG=385924
Committed: https://crrev.com/23299813b94afbf46396188c11491be00c90a819
Cr-Commit-Position: refs/heads/master@{#407589}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Reviewer's Feedback #
Total comments: 4
Patch Set 3 : Reviewer's Feedback #
Total comments: 6
Patch Set 4 : Reviewer's Feedback #Messages
Total messages: 17 (6 generated)
yuweih@chromium.org changed reviewers: + lambroslambrou@chromium.org
ptal
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... File remoting/client/jni/egl_thread_context.cc (right): https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.cc:28: } else { nit: for consistency return above and remove else. Or alternatively you can format this code as follows: if (init_opengl_3) { version = 3; } else (init_opengl_2) { LOG(WARNING) << "using OGL2"; version = 2; } else { LOG(FATAL). } https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.cc:93: EGLint numConfigs; num_configs https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... File remoting/client/jni/egl_thread_context.h (right): https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:46: // Returns true IFF the context is bound to a window (i.e. current surface is s/IFF/if/ Not everybody is familiar with this term, so it's better to avoid it. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:51: // Returns true IFF the buffer is successfully swapped. s/IFF/if/ (also it's not abbreviation, so it should be in upper-case) https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:59: // renderable_type: document this parameter? or maybe replace it with client_version https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:62: bool CreateContextWithClientVersion(unsigned int renderable_type, Please use int instead of unsigned int. See https://google.github.io/styleguide/cppguide.html#Integer_Types https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:69: int client_version_ = 0; nit: maybe useful to declare enum for GL version
https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... File remoting/client/jni/egl_thread_context.cc (right): https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.cc:106: LOG(WARNING) << "Failed to create context: " << eglGetError(); You can maybe remove this log, as you are already logging failure in the ctor at line 24. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... File remoting/client/jni/egl_thread_context.h (right): https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:20: // An example use case: Does this example offer any value above the real implementation? https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:56: int GetClientVersion() const; This is a simple getter so call it client_version() and inline it here. Also, do you need a public getter? This CL doesn't currently use it. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:59: // renderable_type: This comment is confusing. Can you write it as English sentences (beginning with capital, ending with period)? What is |renderable_type|? What is a "Context"? https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:69: int client_version_ = 0; Do you need to store version here? Couldn't you call context_.GetClientVersion() as you do in your example code comment?
Patchset #2 (id:20001) has been deleted
ptal https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... File remoting/client/jni/egl_thread_context.cc (right): https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.cc:28: } else { On 2016/07/22 20:12:34, Sergey Ulanov wrote: > nit: for consistency return above and remove else. > Or alternatively you can format this code as follows: > > if (init_opengl_3) { > version = 3; > } else (init_opengl_2) { > LOG(WARNING) << "using OGL2"; > version = 2; > } else { > LOG(FATAL). > } Done. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.cc:93: EGLint numConfigs; On 2016/07/22 20:12:34, Sergey Ulanov wrote: > num_configs Done. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.cc:106: LOG(WARNING) << "Failed to create context: " << eglGetError(); On 2016/07/22 20:36:07, Lambros wrote: > You can maybe remove this log, as you are already logging failure in the ctor at > line 24. Done. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... File remoting/client/jni/egl_thread_context.h (right): https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:20: // An example use case: On 2016/07/22 20:36:07, Lambros wrote: > Does this example offer any value above the real implementation? Remove this? https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:46: // Returns true IFF the context is bound to a window (i.e. current surface is On 2016/07/22 20:12:34, Sergey Ulanov wrote: > s/IFF/if/ > > Not everybody is familiar with this term, so it's better to avoid it. Done. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:51: // Returns true IFF the buffer is successfully swapped. On 2016/07/22 20:12:34, Sergey Ulanov wrote: > s/IFF/if/ > (also it's not abbreviation, so it should be in upper-case) Done. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:56: int GetClientVersion() const; On 2016/07/22 20:36:07, Lambros wrote: > This is a simple getter so call it client_version() and inline it here. > > Also, do you need a public getter? This CL doesn't currently use it. inlined. It will be used in JniGlDisplayHandler as what the example does... https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:59: // renderable_type: On 2016/07/22 20:36:07, Lambros wrote: > This comment is confusing. Can you write it as English sentences (beginning with > capital, ending with period)? > What is |renderable_type|? > What is a "Context"? Changed the comment a little bit... EGLContext and renderable type are EGL terms... https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:62: bool CreateContextWithClientVersion(unsigned int renderable_type, On 2016/07/22 20:12:34, Sergey Ulanov wrote: > Please use int instead of unsigned int. See > https://google.github.io/styleguide/cppguide.html#Integer_Types Done. https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:69: int client_version_ = 0; On 2016/07/22 20:36:07, Lambros wrote: > Do you need to store version here? Couldn't you call context_.GetClientVersion() > as you do in your example code comment? GetClientVersion() just returns client_version_... https://codereview.chromium.org/2175913002/diff/1/remoting/client/jni/egl_thr... remoting/client/jni/egl_thread_context.h:69: int client_version_ = 0; On 2016/07/22 20:12:34, Sergey Ulanov wrote: > nit: maybe useful to declare enum for GL version Done.
lgtm https://codereview.chromium.org/2175913002/diff/40001/remoting/client/jni/egl... File remoting/client/jni/egl_thread_context.h (right): https://codereview.chromium.org/2175913002/diff/40001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.h:36: // not NULL), false otherwise. "false otherwise" is not needed; see https://google.github.io/styleguide/cppguide.html#Function_Comments (search for "false otherwise"). https://codereview.chromium.org/2175913002/diff/40001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.h:40: // Returns true if the buffer is successfully swapped, false otherwise. Same here.
Hi Sergey, do you have any other comments for this CL? https://codereview.chromium.org/2175913002/diff/40001/remoting/client/jni/egl... File remoting/client/jni/egl_thread_context.h (right): https://codereview.chromium.org/2175913002/diff/40001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.h:36: // not NULL), false otherwise. On 2016/07/23 01:50:13, Lambros wrote: > "false otherwise" is not needed; see > https://google.github.io/styleguide/cppguide.html#Function_Comments > (search for "false otherwise"). > Done. https://codereview.chromium.org/2175913002/diff/40001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.h:40: // Returns true if the buffer is successfully swapped, false otherwise. On 2016/07/23 01:50:13, Lambros wrote: > Same here. Done.
lgtm when my comments are addressed. https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... File remoting/client/jni/egl_thread_context.cc (right): https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.cc:83: ClientVersion client_version) { DCHECK here that client_version != UNKNOWN https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... File remoting/client/jni/egl_thread_context.h (right): https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.h:21: enum ClientVersion { nit: use enum class and remove CLIENT_VERSION_ prefix from the names (then each value can be referenced as ClientVersion::ES_2). https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.h:21: enum ClientVersion { Call this GlVersion?
https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... File remoting/client/jni/egl_thread_context.cc (right): https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.cc:83: ClientVersion client_version) { On 2016/07/25 19:47:11, Sergey Ulanov wrote: > DCHECK here that client_version != UNKNOWN Done. https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... File remoting/client/jni/egl_thread_context.h (right): https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.h:21: enum ClientVersion { On 2016/07/25 19:47:11, Sergey Ulanov wrote: > nit: use enum class and remove CLIENT_VERSION_ prefix from the names (then each > value can be referenced as ClientVersion::ES_2). Done. https://codereview.chromium.org/2175913002/diff/60001/remoting/client/jni/egl... remoting/client/jni/egl_thread_context.h:21: enum ClientVersion { On 2016/07/25 19:47:11, Sergey Ulanov wrote: > Call this GlVersion? Done.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2175913002/#ps80001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Make EglThreadContext create OpenGL ES 3 context Currently EglThreadContext only creates OpenGL ES 2 context. OpenGL ES 3 is backward compatible with ES 2 and has new interfaces (e.g. GL_PACK_ROW_LENGTH) that can improve the performance of the client OpenGL renderer. This CL makes EglThreadContext create OpenGL ES context of the highest version supported by the software or hardward of the device. BUG=385924 ========== to ========== [Remoting Android] Make EglThreadContext create OpenGL ES 3 context Currently EglThreadContext only creates OpenGL ES 2 context. OpenGL ES 3 is backward compatible with ES 2 and has new interfaces (e.g. GL_PACK_ROW_LENGTH) that can improve the performance of the client OpenGL renderer. This CL makes EglThreadContext create OpenGL ES context of the highest version supported by the software or hardward of the device. BUG=385924 Committed: https://crrev.com/23299813b94afbf46396188c11491be00c90a819 Cr-Commit-Position: refs/heads/master@{#407589} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/23299813b94afbf46396188c11491be00c90a819 Cr-Commit-Position: refs/heads/master@{#407589} |
