[Remoting Android] Make EglThreadContext create OpenGL ES 3 context

remoting/client/jni/egl_thread_context.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef REMOTING_CLIENT_JNI_EGL_THREAD_CONTEXT_H_
 #define REMOTING_CLIENT_JNI_EGL_THREAD_CONTEXT_H_
 
 #include <EGL/egl.h>
 
 #include "base/macros.h"
 #include "base/threading/thread_checker.h"
 
 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:
   EglThreadContext();
   ~EglThreadContext();
 
   // Creates a surface on the given window and binds the context to the surface.
   // Unbinds |window| last bound if |window| is NULL.
   // EGLNativeWindowType is platform specific. E.g. ANativeWindow* on Android.
   void BindToWindow(EGLNativeWindowType window);
 
   // Returns true IFF the context is bound to a window (i.e. current surface is

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

s/IFF/if/

Not everybody is familiar with this term, so it's better to avoid it.

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

Done.

   // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(EglThreadContext);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_CLIENT_JNI_EGL_THREAD_CONTEXT_H_