[Remoting Android] Make EglThreadContext create OpenGL ES 3 context

remoting/client/jni/egl_thread_context.h

Index: remoting/client/jni/egl_thread_context.h
diff --git a/remoting/client/jni/egl_thread_context.h b/remoting/client/jni/egl_thread_context.h
index f8f2b8e89cd3b862f49b8fc26024d13fe442f81d..e9788cfe935c0973d63914fbc01b4f6ddaae109e 100644
--- a/remoting/client/jni/egl_thread_context.h
+++ b/remoting/client/jni/egl_thread_context.h
@@ -12,19 +12,26 @@
 
 namespace remoting {
 
-// Establishes an EGL-OpenGL|ES2 context on current thread. Must be constructed,
-// used, and deleted on single thread (i.e. the display thread). Each thread can
-// have no more than one EglThreadContext.
+// Establishes an EGL-OpenGL|ES 2 (if 3 is not supported) or 3 (backward
+// compatible with 2) context on current thread. Must be constructed, used, and
+// deleted on single thread (i.e. the display thread). Each thread can have no
+// more than one EglThreadContext.
 //
 // An example use case:

[Lambros, 2016/07/22 20:36:07]

Does this example offer any value above the real implementation?

[Yuwei, 2016/07/22 21:22:39]

Remove this?

 // class DisplayHandler {
 //   void OnSurfaceCreated(ANativeWindow* surface) {
+//     if (context_.GetClientVersion() > 2) {
+//       renderer_.reset(new BetterRenderer());
+//     } else {
+//       renderer_.reset(new SoftwareRenderer());
+//     }
 //     context_.BindToWindow(surface);
 //   }
 //   void OnSurfaceDestroyed() {
 //     context_.BindToWindow(nullptr);
 //   }
 //   EglThreadContext context_;
+//   unique_ptr<VideoRenderer> renderer_;
 // };
 class EglThreadContext {
  public:
@@ -40,15 +47,26 @@ class EglThreadContext {
   // not NULL).
   bool IsWindowBound() const;
 
-  // Posts EGL surface buffer to the window being bound. Window must be bound
-  // before calling this function.
-  void SwapBuffers();
+  // Posts EGL surface buffer to the window being bound.
+  // Returns true IFF the buffer is successfully swapped.

[Sergey Ulanov, 2016/07/22 20:12:34]

s/IFF/if/
(also it's not abbreviation, so it should be in upper-case)

[Yuwei, 2016/07/22 21:22:39]

Done.

+  bool SwapBuffers();
+
+  // Returns the current OpenGL ES client version of the EGL context. It will be
+  // either 2 or 3.
+  int GetClientVersion() const;

[Lambros, 2016/07/22 20:36:07]

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.

[Yuwei, 2016/07/22 21:22:39]

inlined.

It will be used in JniGlDisplayHandler as what the example does...

 
  private:
+  // renderable_type:

[Sergey Ulanov, 2016/07/22 20:12:34]

document this parameter? or maybe replace it with client_version

[Lambros, 2016/07/22 20:36:07]

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"?

[Yuwei, 2016/07/22 21:22:40]

Changed the comment a little bit... EGLContext and renderable type are EGL
terms...

+  // client_version: Either 2 or 3. Specifying whether OpenGL ES 2 or OpenGL ES
+  // 3 context should be created.
+  bool CreateContextWithClientVersion(unsigned int renderable_type,

[Sergey Ulanov, 2016/07/22 20:12:34]

Please use int instead of unsigned int. See
https://google.github.io/styleguide/cppguide.html#Integer_Types

[Yuwei, 2016/07/22 21:22:40]

Done.

+                                      int client_version);
+
   EGLDisplay display_ = EGL_NO_DISPLAY;
   EGLConfig config_ = nullptr;
   EGLSurface surface_ = EGL_NO_SURFACE;
   EGLContext context_ = EGL_NO_CONTEXT;
+  int client_version_ = 0;

[Sergey Ulanov, 2016/07/22 20:12:34]

nit: maybe useful to declare enum for GL version

[Lambros, 2016/07/22 20:36:07]

Do you need to store version here? Couldn't you call context_.GetClientVersion()
as you do in your example code comment?

[Yuwei, 2016/07/22 21:22:39]

GetClientVersion() just returns client_version_...

[Yuwei, 2016/07/22 21:22:39]

Done.

 
   base::ThreadChecker thread_checker_;