Support using OpenGL ES context on desktop

Support using OpenGL ES context on desktop

Support using OpenGL ES context on desktop for unix and Android platforms. This
is mainly useful in development.

Add --gpuAPI flag to gm, dm, bench, bench_pictures and render_pictures. The
possible parameters for the flag are "gl" and "gles".

Committed: https://skia.googlesource.com/skia/+/80549fcdd50269d7e069d6db02b395fca128056c

[Kimmo Kinnunen, 2014-06-09 05:29:31]

Do you think this is worth the complexity?

Initially I did it to develop NVPR, but turns out NVPR is not advertised on our
desktop ES, yet. It'll take a few months/years/whatever to get such a driver
out..

[bsalomon, 2014-06-09 13:53:47]

I definitely think it's worth it. Thank you for doing the interface assembler
for ES. I've been meaning to do that. Adding a few others to have a look.

https://codereview.chromium.org/319043005/diff/30001/src/gpu/gl/GrGLAssembleG...
File src/gpu/gl/GrGLAssembleGLESInterface.h (right):

https://codereview.chromium.org/319043005/diff/30001/src/gpu/gl/GrGLAssembleG...
src/gpu/gl/GrGLAssembleGLESInterface.h:9: const GrGLInterface*
GrGLAssembleGLESInterface(void* ctx, GrGLGetProc get) {
W000000000000000000000T!

[mtklein, 2014-06-09 14:10:46]

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp#newcode44
dm/DM.cpp:44: DEFINE_string(forceGpuApi, "", "Force use of specific gpu API. 
Using \"gl\" "
Personally I'd go with just gpuAPI?

Is there any way we can do both GL and GLES in the same process?  Is this a
global thing or can we get two Ganeshes going at once, one GL, one GLES?  We
don't necessarily have to do that today, but it'd help me understand how better
to structure this in DM.

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp#newcode220
dm/DM.cpp:220: static void get_forced_gpu_api_if_matches(GrGLStandard*
forcedGpuApi) {
Simplify?

static GrGLStandard get_gl_standard() {
  if (FLAGS_gpuAPI.contains("gl"))   { return kGL_GrGLStandard;   }
  if (FLAGS_gpuAPI.conaints("gles")) { return kGLES_GrGLStandard; }
  return kNone_GrGLStandard;
}

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp#newcode230
dm/DM.cpp:230: *forcedGpuApi = kNone_GrGLStandard;
What happens if I run DM in None mode?  Does that pick up the default?

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp#newcode297
dm/DM.cpp:297: kick_off_gms(gms, configs, forcedGpuApi, *expectations,
&reporter, &tasks);
e.g. could we logically just loop over GPU APIs here instead of picking one?

https://codereview.chromium.org/319043005/diff/30001/tests/BlurTest.cpp
File tests/BlurTest.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/tests/BlurTest.cpp#newco...
tests/BlurTest.cpp:281: kNone_GrGLStandard);
Passing kNone in all these tests reads weird.  Can we make this
kDefault_GrGLStandard or kNative_GrGLStandard?

[bsalomon, 2014-06-09 14:15:11]

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp#newcode44
dm/DM.cpp:44: DEFINE_string(forceGpuApi, "", "Force use of specific gpu API. 
Using \"gl\" "
On 2014/06/09 14:10:46, mtklein wrote:
> Personally I'd go with just gpuAPI?
> 
> Is there any way we can do both GL and GLES in the same process?  Is this a
> global thing or can we get two Ganeshes going at once, one GL, one GLES?  We
> don't necessarily have to do that today, but it'd help me understand how
better
> to structure this in DM.

It definitely *could* be implemented that way. One way of structuring this could
be to have the API as part of the config, though I'd want the configs to be
generated in that case as the x-product is getting larger (msaa sample count,
nvpr-enabled vs disabled, GLES/GL, debug/null/native/mesa/angle)

[mtklein, 2014-06-09 14:20:21]

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp#newcode44
dm/DM.cpp:44: DEFINE_string(forceGpuApi, "", "Force use of specific gpu API. 
Using \"gl\" "
On 2014/06/09 14:15:11, bsalomon wrote:
> On 2014/06/09 14:10:46, mtklein wrote:
> > Personally I'd go with just gpuAPI?
> > 
> > Is there any way we can do both GL and GLES in the same process?  Is this a
> > global thing or can we get two Ganeshes going at once, one GL, one GLES?  We
> > don't necessarily have to do that today, but it'd help me understand how
> better
> > to structure this in DM.
> 
> It definitely *could* be implemented that way. One way of structuring this
could
> be to have the API as part of the config, though I'd want the configs to be
> generated in that case as the x-product is getting larger (msaa sample count,
> nvpr-enabled vs disabled, GLES/GL, debug/null/native/mesa/angle)

Yeah, this is fine, we can cross them here inside DM.cpp.  What I'm wondering is
if we really have to choose here, or can we do everything at the same time?

[bsalomon, 2014-06-09 14:29:52]

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/dm/DM.cpp#newcode44
dm/DM.cpp:44: DEFINE_string(forceGpuApi, "", "Force use of specific gpu API. 
Using \"gl\" "
On 2014/06/09 14:20:21, mtklein wrote:
> On 2014/06/09 14:15:11, bsalomon wrote:
> > On 2014/06/09 14:10:46, mtklein wrote:
> > > Personally I'd go with just gpuAPI?
> > > 
> > > Is there any way we can do both GL and GLES in the same process?  Is this
a
> > > global thing or can we get two Ganeshes going at once, one GL, one GLES? 
We
> > > don't necessarily have to do that today, but it'd help me understand how
> > better
> > > to structure this in DM.
> > 
> > It definitely *could* be implemented that way. One way of structuring this
> could
> > be to have the API as part of the config, though I'd want the configs to be
> > generated in that case as the x-product is getting larger (msaa sample
count,
> > nvpr-enabled vs disabled, GLES/GL, debug/null/native/mesa/angle)
> 
> Yeah, this is fine, we can cross them here inside DM.cpp.  What I'm wondering
is
> if we really have to choose here, or can we do everything at the same time?

If we move towards supporting arbitrary members of the x-product of GPU config
variants, the number of configs is too large to run all at once and very
redundant. I think we'll want some idea of defaults that give good coverage and
then have a way for a caller to select an arbitrary subset at the cmd line or
perhaps with a config file.

[robertphillips, 2014-06-09 14:31:05]

https://codereview.chromium.org/319043005/diff/30001/src/gpu/gl/unix/SkNative...
File src/gpu/gl/unix/SkNativeGLContext_unix.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/src/gpu/gl/unix/SkNative...
src/gpu/gl/unix/SkNativeGLContext_unix.cpp:216: static const int
context_attribs_gl_fallback[] = {
Shouldn't this value be '1'?

https://codereview.chromium.org/319043005/diff/30001/tests/BlurTest.cpp
File tests/BlurTest.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/tests/BlurTest.cpp#newco...
tests/BlurTest.cpp:281: kNone_GrGLStandard);
I agree - it is kind of weird.

https://codereview.chromium.org/319043005/diff/30001/tests/DeferredCanvasTest...
File tests/DeferredCanvasTest.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/tests/DeferredCanvasTest...
tests/DeferredCanvasTest.cpp:762: if (useGpu) {
nit: overlength

https://codereview.chromium.org/319043005/diff/30001/tests/ImageFilterTest.cpp
File tests/ImageFilterTest.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/tests/ImageFilterTest.cp...
tests/ImageFilterTest.cpp:748: DEF_GPUTEST(HugeBlurImageFilterGPU, reporter,
factory) {
nits: this and the next are overlength

https://codereview.chromium.org/319043005/diff/30001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/319043005/diff/30001/tests/SurfaceTest.cpp#ne...
tests/SurfaceTest.cpp:427: if (NULL != factory) {
nit: overlength

[Kimmo Kinnunen, 2014-06-16 12:36:40]

How about this?

https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp
File dm/DM.cpp (right):

https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp#newcode44
dm/DM.cpp:44: DEFINE_string(forceGpuApi, "", "Force use of specific gpu API. 
Using \"gl\" "
On 2014/06/09 14:10:46, mtklein wrote:
> Personally I'd go with just gpuAPI?

Done.

https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp#newcode220
dm/DM.cpp:220: static void get_forced_gpu_api_if_matches(GrGLStandard*
forcedGpuApi) {
On 2014/06/09 14:10:45, mtklein wrote:
> Simplify?
> 
> static GrGLStandard get_gl_standard() {
>   if (FLAGS_gpuAPI.contains("gl"))   { return kGL_GrGLStandard;   }
>   if (FLAGS_gpuAPI.conaints("gles")) { return kGLES_GrGLStandard; }
>   return kNone_GrGLStandard;
> }

Done.

https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp#newcode230
dm/DM.cpp:230: *forcedGpuApi = kNone_GrGLStandard;
On 2014/06/09 14:10:45, mtklein wrote:
> What happens if I run DM in None mode?  Does that pick up the default?

Yeah. None picks up the default for the platform.

https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp#newcode297
dm/DM.cpp:297: kick_off_gms(gms, configs, forcedGpuApi, *expectations,
&reporter, &tasks);
On 2014/06/09 14:10:45, mtklein wrote:
> e.g. could we logically just loop over GPU APIs here instead of picking one?

IIUC from the discussion above, this was not needed.

https://chromiumcodereview.appspot.com/319043005/diff/30001/src/gpu/gl/unix/S...
File src/gpu/gl/unix/SkNativeGLContext_unix.cpp (right):

https://chromiumcodereview.appspot.com/319043005/diff/30001/src/gpu/gl/unix/S...
src/gpu/gl/unix/SkNativeGLContext_unix.cpp:216: static const int
context_attribs_gl_fallback[] = {
On 2014/06/09 14:31:05, robertphillips wrote:
> Shouldn't this value be '1'?

Done. (sorry..)

https://chromiumcodereview.appspot.com/319043005/diff/30001/tests/BlurTest.cpp
File tests/BlurTest.cpp (right):

https://chromiumcodereview.appspot.com/319043005/diff/30001/tests/BlurTest.cp...
tests/BlurTest.cpp:281: kNone_GrGLStandard);
On 2014/06/09 14:10:46, mtklein wrote:
> Passing kNone in all these tests reads weird.  Can we make this
> kDefault_GrGLStandard or kNative_GrGLStandard?

That would be double the weird for all the other funcs that accept GrGLStandard,
as it would give the impression that there's 4 possible distinct values, and the
other code handles only 2 or max 3 cases.

I can of course make the "get" function to have default value in its second
parameter, eg. GrContextFactor::get(GLContextType type, GrGLStandard forcedApi =
kNone_GrGLStandard). The idea would be that passing kNone would read "set the
forced api to none, in other words do not force the api". Does this suffice?

https://chromiumcodereview.appspot.com/319043005/diff/30001/tests/DeferredCan...
File tests/DeferredCanvasTest.cpp (right):

https://chromiumcodereview.appspot.com/319043005/diff/30001/tests/DeferredCan...
tests/DeferredCanvasTest.cpp:762: if (useGpu) {
On 2014/06/09 14:31:05, robertphillips wrote:
> nit: overlength

Done. (reverted)

https://chromiumcodereview.appspot.com/319043005/diff/30001/tests/ImageFilter...
File tests/ImageFilterTest.cpp (right):

https://chromiumcodereview.appspot.com/319043005/diff/30001/tests/ImageFilter...
tests/ImageFilterTest.cpp:748: DEF_GPUTEST(HugeBlurImageFilterGPU, reporter,
factory) {
On 2014/06/09 14:31:05, robertphillips wrote:
> nits: this and the next are overlength

Done.

https://chromiumcodereview.appspot.com/319043005/diff/30001/tests/SurfaceTest...
File tests/SurfaceTest.cpp (right):

https://chromiumcodereview.appspot.com/319043005/diff/30001/tests/SurfaceTest...
tests/SurfaceTest.cpp:427: if (NULL != factory) {
On 2014/06/09 14:31:05, robertphillips wrote:
> nit: overlength

Done.

[bsalomon, 2014-06-16 14:25:10]

lgtm if Mike's ok with the DM changes. If so, I'll launch some try bots on
different platforms to make sure that the GL interface setup code works.

https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp
File dm/DM.cpp (right):

https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp#newcode297
dm/DM.cpp:297: kick_off_gms(gms, configs, forcedGpuApi, *expectations,
&reporter, &tasks);
On 2014/06/16 12:36:39, kkinnunen wrote:
> On 2014/06/09 14:10:45, mtklein wrote:
> > e.g. could we logically just loop over GPU APIs here instead of picking one?
> 
> IIUC from the discussion above, this was not needed.

+1 for leaving any running of multiple APIs in a single invocation as future
work.

[mtklein, 2014-06-16 15:09:43]

On 2014/06/16 14:25:10, bsalomon wrote:
> lgtm if Mike's ok with the DM changes. If so, I'll launch some try bots on
> different platforms to make sure that the GL interface setup code works.
> 
> https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp
> File dm/DM.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/319043005/diff/30001/dm/DM.cpp#newcode297
> dm/DM.cpp:297: kick_off_gms(gms, configs, forcedGpuApi, *expectations,
> &reporter, &tasks);
> On 2014/06/16 12:36:39, kkinnunen wrote:
> > On 2014/06/09 14:10:45, mtklein wrote:
> > > e.g. could we logically just loop over GPU APIs here instead of picking
one?
> > 
> > IIUC from the discussion above, this was not needed.
> 
> +1 for leaving any running of multiple APIs in a single invocation as future
> work.

lgtm

[Kimmo Kinnunen, 2014-06-19 11:36:31]

On 2014/06/16 14:25:10, bsalomon wrote:
> lgtm if Mike's ok with the DM changes. If so, I'll launch some try bots on
> different platforms to make sure that the GL interface setup code works.

Mike did give LGTM for this. This would be now ready for some try bots.
Unfortunately I still can not submit any tries personally -- the diffs seem to
go to the repo, but the results never come back via email.

[bsalomon, 2014-06-19 13:08:32]

On 2014/06/19 11:36:31, kkinnunen wrote:
> On 2014/06/16 14:25:10, bsalomon wrote:
> > lgtm if Mike's ok with the DM changes. If so, I'll launch some try bots on
> > different platforms to make sure that the GL interface setup code works.
> 
> Mike did give LGTM for this. This would be now ready for some try bots.
> Unfortunately I still can not submit any tries personally -- the diffs seem to
> go to the repo, but the results never come back via email.

I ran the try bots but forgot to come back and check the CQ. Doing that now.

[bsalomon, 2014-06-19 13:08:36]

The CQ bit was checked by bsalomon@google.com

[commit-bot: I haz the power, 2014-06-19 13:09:38]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/319043005/50001

[commit-bot: I haz the power, 2014-06-19 13:09:52]

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

[commit-bot: I haz the power, 2014-06-19 13:09:55]

Failed to apply patch for dm/DM.cpp:
While running git apply --index -p1;
  error: patch failed: dm/DM.cpp:36
  error: dm/DM.cpp: patch does not apply

Patch:       dm/DM.cpp
Index: dm/DM.cpp
diff --git a/dm/DM.cpp b/dm/DM.cpp
index
87555984bc581d43caa74a95d7ead4065fc7d5d5..723d4e856d6e8ed0e93b4bd61cfabdcc9ada6ea2
100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -36,8 +36,15 @@ using skiagm::GMRegistry;
 using skiatest::Test;
 using skiatest::TestRegistry;
 
+static const char kGpuAPINameGL[] = "gl";
+static const char kGpuAPINameGLES[] = "gles";
+
 DEFINE_int32(threads, -1, "Threads for CPU work. Default NUM_CPUS.");
 DEFINE_int32(gpuThreads, 1, "Threads for GPU work.");
+DEFINE_string(gpuAPI, "", "Force use of specific gpu API.  Using \"gl\" "
+              "forces OpenGL API. Using \"gles\" forces OpenGL ES API. "
+              "Defaults to empty string, which selects the API native to the "
+              "system.");
 #ifdef SK_BUILD_JSON_WRITER
 DEFINE_string2(expectations, r, "",
                "If a directory, compare generated images against images under
this path. "
@@ -100,6 +107,7 @@ native;
 
 static void kick_off_gms(const SkTDArray<GMRegistry::Factory>& gms,
                          const SkTArray<SkString>& configs,
+                         GrGLStandard gpuAPI,
                          const DM::Expectations& expectations,
                          DM::Reporter* reporter,
                          DM::TaskRunner* tasks) {
@@ -109,17 +117,18 @@ static void kick_off_gms(const
SkTDArray<GMRegistry::Factory>& gms,
     }
     for (int i = 0; i < gms.count(); i++) {
         for (int j = 0; j < configs.count(); j++) {
+
             START("565",        CpuGMTask, expectations, kRGB_565_SkColorType);
             START("8888",       CpuGMTask, expectations, kN32_SkColorType);
-            START("gpu",        GpuGMTask, expectations, native, 0);
-            START("msaa4",      GpuGMTask, expectations, native, 4);
-            START("msaa16",     GpuGMTask, expectations, native, 16);
-            START("nvprmsaa4",  GpuGMTask, expectations, nvpr,   4);
-            START("nvprmsaa16", GpuGMTask, expectations, nvpr,   16);
-            START("gpunull",    GpuGMTask, expectations, null,   0);
-            START("gpudebug",   GpuGMTask, expectations, debug,  0);
-            START("angle",      GpuGMTask, expectations, angle,  0);
-            START("mesa",       GpuGMTask, expectations, mesa,   0);
+            START("gpu",        GpuGMTask, expectations, native, gpuAPI, 0);
+            START("msaa4",      GpuGMTask, expectations, native, gpuAPI, 4);
+            START("msaa16",     GpuGMTask, expectations, native, gpuAPI, 16);
+            START("nvprmsaa4",  GpuGMTask, expectations, nvpr,   gpuAPI, 4);
+            START("nvprmsaa16", GpuGMTask, expectations, nvpr,   gpuAPI, 16);
+            START("gpunull",    GpuGMTask, expectations, null,   gpuAPI, 0);
+            START("gpudebug",   GpuGMTask, expectations, debug,  gpuAPI, 0);
+            START("angle",      GpuGMTask, expectations, angle,  gpuAPI, 0);
+            START("mesa",       GpuGMTask, expectations, mesa,   gpuAPI, 0);
             START("pdf",        PDFTask,   RASTERIZE_PDF_PROC);
         }
     }
@@ -128,6 +137,7 @@ static void kick_off_gms(const
SkTDArray<GMRegistry::Factory>& gms,
 
 static void kick_off_benches(const SkTDArray<BenchRegistry::Factory>& benches,
                              const SkTArray<SkString>& configs,
+                             GrGLStandard gpuAPI,
                              DM::Reporter* reporter,
                              DM::TaskRunner* tasks) {
 #define START(name, type, ...)                                                 
               \
@@ -139,15 +149,15 @@ static void kick_off_benches(const
SkTDArray<BenchRegistry::Factory>& benches,
             START("nonrendering", NonRenderingBenchTask);
             START("565",          CpuBenchTask, kRGB_565_SkColorType);
             START("8888",         CpuBenchTask, kN32_SkColorType);
-            START("gpu",          GpuBenchTask, native, 0);
-            START("msaa4",        GpuBenchTask, native, 4);
-            START("msaa16",       GpuBenchTask, native, 16);
-            START("nvprmsaa4",    GpuBenchTask, nvpr,   4);
-            START("nvprmsaa16",   GpuBenchTask, nvpr,   16);
-            START("gpunull",      GpuBenchTask, null,   0);
-            START("gpudebug",     GpuBenchTask, debug,  0);
-            START("angle",        GpuBenchTask, angle,  0);
-            START("mesa",         GpuBenchTask, mesa,   0);
+            START("gpu",          GpuBenchTask, native, gpuAPI, 0);
+            START("msaa4",        GpuBenchTask, native, gpuAPI, 4);
+            START("msaa16",       GpuBenchTask, native, gpuAPI, 16);
+            START("nvprmsaa4",    GpuBenchTask, nvpr,   gpuAPI, 4);
+            START("nvprmsaa16",   GpuBenchTask, nvpr,   gpuAPI, 16);
+            START("gpunull",      GpuBenchTask, null,   gpuAPI, 0);
+            START("gpudebug",     GpuBenchTask, debug,  gpuAPI, 0);
+            START("angle",        GpuBenchTask, angle,  gpuAPI, 0);
+            START("mesa",         GpuBenchTask, mesa,   gpuAPI, 0);
         }
     }
 #undef START
@@ -209,6 +219,16 @@ static void report_failures(const SkTArray<SkString>&
failures) {
     SkDebugf("%d failures.\n", failures.count());
 }
 
+static GrGLStandard get_gl_standard() {
+  if (FLAGS_gpuAPI.contains(kGpuAPINameGL)) {
+      return kGL_GrGLStandard;
+  }
+  if (FLAGS_gpuAPI.contains(kGpuAPINameGLES)) {
+      return kGLES_GrGLStandard;
+  }
+  return kNone_GrGLStandard;
+}
+
 template <typename T, typename Registry>
 static void append_matching_factories(Registry* head, SkTDArray<typename
Registry::Factory>* out) {
     for (const Registry* reg = head; reg != NULL; reg = reg->next()) {
@@ -239,6 +259,8 @@ int tool_main(int argc, char** argv) {
         SkStrSplit(FLAGS_config[i], ", ", &configs);
     }
 
+    GrGLStandard gpuAPI = get_gl_standard();
+
     SkTDArray<GMRegistry::Factory> gms;
     SkAutoTDelete<DM::Expectations> expectations(SkNEW(DM::NoExpectations));
     if (FLAGS_gms) {
@@ -270,8 +292,8 @@ int tool_main(int argc, char** argv) {
              gms.count(), benches.count(), configs.count(), tests.count());
     DM::Reporter reporter;
     DM::TaskRunner tasks(FLAGS_threads, FLAGS_gpuThreads);
-    kick_off_gms(gms, configs, *expectations, &reporter, &tasks);
-    kick_off_benches(benches, configs, &reporter, &tasks);
+    kick_off_gms(gms, configs, gpuAPI, *expectations, &reporter, &tasks);
+    kick_off_benches(benches, configs, gpuAPI, &reporter, &tasks);
     kick_off_tests(tests, &reporter, &tasks);
     kick_off_skps(&reporter, &tasks);
     tasks.wait();

[Kimmo Kinnunen, 2014-06-23 05:41:35]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2014-06-23 05:41:42]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/319043005/70001

[commit-bot: I haz the power, 2014-06-23 05:57:00]

Change committed as 74fc727dc88ee24d89f88cb1709f963e9073aeb3

[rmistry, 2014-06-23 12:22:13]

On 2014/06/23 05:57:00, I haz the power (commit-bot) wrote:
> Change committed as 74fc727dc88ee24d89f88cb1709f963e9073aeb3

This CL seems to have caused segmentation faults in RunBench and BenchPictures:
http://108.170.220.21:10117/builders/Perf-Android-Xoom-Tegra2-Arm7-Release/bu...

[rmistry, 2014-06-23 13:04:21]

On 2014/06/23 12:22:13, rmistry wrote:
> On 2014/06/23 05:57:00, I haz the power (commit-bot) wrote:
> > Change committed as 74fc727dc88ee24d89f88cb1709f963e9073aeb3
> 
> This CL seems to have caused segmentation faults in RunBench and
BenchPictures:
>
http://108.170.220.21:10117/builders/Perf-Android-Xoom-Tegra2-Arm7-Release/bu...

The following is the comprehensive list of all broken builders from this CL:

Test-Android-Nexus4-Adreno320-Arm7-Debug: RunBench - 
http://108.170.220.21:10117/builders/Test-Android-Nexus4-Adreno320-Arm7-Debug...
Test-Android-Nexus4-Adreno320-Arm7-Release: CompareGMs -
http://108.170.220.21:10117/builders/Test-Android-Nexus4-Adreno320-Arm7-Relea...
Test-Android-NexusS-SGX540-Arm7-Debug: RunBench -
http://108.170.220.21:10117/builders/Test-Android-NexusS-SGX540-Arm7-Debug/bu...
Test-Android-NexusS-SGX540-Arm7-Release: CompareGMs -
http://108.170.220.21:10117/builders/Test-Android-NexusS-SGX540-Arm7-Release/...
Test-Android-Xoom-Tegra2-Arm7-Debug: RunBench -
http://108.170.220.21:10117/builders/Test-Android-Xoom-Tegra2-Arm7-Debug/buil...
Test-Android-Xoom-Tegra2-Arm7-Release: CompareGMs -
http://108.170.220.21:10117/builders/Test-Android-Xoom-Tegra2-Arm7-Release/bu...
Perf-Android-Nexus7-Tegra3-Arm7-Release: CheckForRegressions -
http://108.170.220.21:10117/builders/Perf-Android-Nexus7-Tegra3-Arm7-Release/...
Perf-Android-NexusS-SGX540-Arm7-Release: RunBench -
http://108.170.220.21:10117/builders/Perf-Android-NexusS-SGX540-Arm7-Release/...
Perf-Android-Xoom-Tegra2-Arm7-Release: BenchPictures -
http://108.170.220.21:10117/builders/Perf-Android-Xoom-Tegra2-Arm7-Release/bu...

[rmistry, 2014-06-23 13:08:30]

A revert of this CL has been created in
https://codereview.chromium.org/351583002/ by rmistry@google.com.

The reason for reverting is: Caused segmentation fault on many builders. Please
see reverted CL's msg #21 for details..

[Kimmo Kinnunen, 2014-06-24 06:29:07]

The crash fixed, PTAL, thanks.

[bsalomon, 2014-06-24 14:01:57]

https://codereview.chromium.org/319043005/diff/90001/src/gpu/gl/GrGLAssembleG...
File src/gpu/gl/GrGLAssembleGLESInterface.h (right):

https://codereview.chromium.org/319043005/diff/90001/src/gpu/gl/GrGLAssembleG...
src/gpu/gl/GrGLAssembleGLESInterface.h:10: const char* verStr =
reinterpret_cast<const char*>(glGetString(GR_GL_VERSION));
Is this assuming we always have a gl.h included here?

[Kimmo Kinnunen, 2014-06-30 07:44:48]

On 2014/06/24 14:01:57, bsalomon wrote:
>
https://codereview.chromium.org/319043005/diff/90001/src/gpu/gl/GrGLAssembleG...
> File src/gpu/gl/GrGLAssembleGLESInterface.h (right):
> 
>
https://codereview.chromium.org/319043005/diff/90001/src/gpu/gl/GrGLAssembleG...
> src/gpu/gl/GrGLAssembleGLESInterface.h:10: const char* verStr =
> reinterpret_cast<const char*>(glGetString(GR_GL_VERSION));
> Is this assuming we always have a gl.h included here?

Yeah.. The structure of the code is a bit unfortunate due to the use of
preprocessor things..

The glGetString symbol itself is already referenced by the includer before the
reference above. 

Updated the patch. I tried to make it more explicit by removing the forward
declaration from the separate header file, making the function itself static and
adding some documentation.

[bsalomon, 2014-06-30 13:21:00]

lgtm

[commit-bot: I haz the power, 2014-06-30 13:36:36]

Change committed as 80549fcdd50269d7e069d6db02b395fca128056c

[mtklein, 2014-06-30 13:57:55]

On 2014/06/30 13:36:36, I haz the power (commit-bot) wrote:
> Change committed as 80549fcdd50269d7e069d6db02b395fca128056c

You guys aware of failures?

http://108.170.220.120:10117/builders/Test-Win7-ShuttleA-HD2000-x86-Debug-ANG...

[bsalomon, 2014-06-30 13:59:20]

On 2014/06/30 13:57:55, mtklein wrote:
> On 2014/06/30 13:36:36, I haz the power (commit-bot) wrote:
> > Change committed as 80549fcdd50269d7e069d6db02b395fca128056c
> 
> You guys aware of failures?
> 
>
http://108.170.220.120:10117/builders/Test-Win7-ShuttleA-HD2000-x86-Debug-ANG...

Taking a look...

[bsalomon, 2014-06-30 15:36:12]

A revert of this CL has been created in
https://codereview.chromium.org/341433007/ by bsalomon@google.com.

The reason for reverting is: Breaks unit tests on ANGLE bot..

[tomhudson, 2014-09-10 14:46:51]

tomhudson@google.com changed reviewers:
+ tomhudson@google.com

[tomhudson, 2014-09-10 14:46:52]

https://codereview.chromium.org/319043005/diff/110001/src/gpu/gl/android/SkNa...
File src/gpu/gl/android/SkNativeGLContext_android.cpp (right):

https://codereview.chromium.org/319043005/diff/110001/src/gpu/gl/android/SkNa...
src/gpu/gl/android/SkNativeGLContext_android.cpp:95: for (size_t i = 0; NULL ==
interface && i < apiLimit; ++api) {
Note that this is a bug that happened to get past code review and not kill us on
most platforms, but surfaces on some platforms. Fix incoming.

[tomhudson, 2014-09-10 14:47:18]

tomhudson@google.com changed reviewers:
+ joshualitt@google.com