Re-land: gpu: Add CHROMIUM_sync_query extension.

Re-land: gpu: Add CHROMIUM_sync_query extension.

This adds a GL_COMMANDS_COMPLETED_CHROMIUM query target that provide
a finer granularity of synchronizing GL command completion than offered
by glFinish().

BUG=269808, 356871, 273274
TEST=gpu_unittests --gtest_filter=GLES2DecoderManualInitTest.BeginEndQueryEXTCommandsCompletedCHROMIUM

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264675

[reveman, 2014-04-15 15:27:22]

Please take a look. I'll make the compositor use this as a follow up.

[no sievers, 2014-04-15 18:41:15]

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/f...
File gpu/command_buffer/service/feature_info.cc (right):

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/f...
gpu/command_buffer/service/feature_info.cc:783: feature_flags_.arb_sync = true;
I missed that es3 has this.
We should update gl_fence.cc to use this on es3 (also |ui_gl_fence_works|
above).

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7308: GLsync gl_sync =
reinterpret_cast<GLsync>(0xdeadbeef);
nit: const GLsync kGLSync = ...

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7311:
.RetiresOnSaturation();
no glFlush() expected?

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
gpu/command_buffer/service/query_manager.cc:433: sync_ =
glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
You have to glFlush() somewhere or the commands might never complete.

Can you just use the GLFence class here?

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
gpu/command_buffer/service/query_manager.cc:437: glFinish();  // Poor man's GPU
fence.
Do we need this fallback, or can we not support the query?
I think GLFence should give us pretty good coverage across platforms (esp. if we
add es3).

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
gpu/command_buffer/service/query_manager.cc:445: glGetSynciv(sync_,
GL_SYNC_STATUS, 1, NULL, &value);
See comment in gl_fence.cc that this does not work on OSX 10.7. It uses
glClientWaitSync() != GL_TIMEOUT_EXPIRED instead.

[no sievers, 2014-04-15 18:48:43]

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
gpu/command_buffer/service/query_manager.cc:433: sync_ =
glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
On 2014/04/15 18:41:15, sievers wrote:
> You have to glFlush() somewhere or the commands might never complete.
> 
> Can you just use the GLFence class here?

"Querying the GL_QUERY_RESULT implicitly flushes the GL pipeline". So we could
also defer it, if that's not already happening somewhere on the service-side.
That would make it more efficient for clients that insert a lot of these queries
per frame.

[reveman, 2014-04-15 22:29:27]

PTAL

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/g...
File gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc (right):

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7308: GLsync gl_sync =
reinterpret_cast<GLsync>(0xdeadbeef);
On 2014/04/15 18:41:15, sievers wrote:
> nit: const GLsync kGLSync = ...

Done.

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/g...
gpu/command_buffer/service/gles2_cmd_decoder_unittest.cc:7311:
.RetiresOnSaturation();
On 2014/04/15 18:41:15, sievers wrote:
> no glFlush() expected?

I was hoping we wouldn't need an implicit flush. The gpu client currently issues
an implicit flush in QueryTracker::Query::CheckResultsAvailable and maybe that's
required by the occlusion query extension but I'm not sure we should be doing
that for chromium based queries unless pname=GL_QUERY_RESULT_EXT and we're about
to block. I think it's generally a good idea to minimize implicit flushes and
instead leave it up to the compositor to handle this whenever possible.

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
gpu/command_buffer/service/query_manager.cc:433: sync_ =
glFenceSync(GL_SYNC_GPU_COMMANDS_COMPLETE, 0);
On 2014/04/15 18:41:15, sievers wrote:
> You have to glFlush() somewhere or the commands might never complete.

As I mentioned above, the idea is to make the client responsible for this. I
think it would be a bit weird if the client didn't have to call glFlush.

> 
> Can you just use the GLFence class here?

Ah, wasn't aware of this class. Done.

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
gpu/command_buffer/service/query_manager.cc:437: glFinish();  // Poor man's GPU
fence.
On 2014/04/15 18:41:15, sievers wrote:
> Do we need this fallback, or can we not support the query?
> I think GLFence should give us pretty good coverage across platforms (esp. if
we
> add es3).

We'll need a fall-back somewhere but maybe it should just be software
compositing. Changed latest patch so it's up to the client to handle this
instead.

https://codereview.chromium.org/238933003/diff/1/gpu/command_buffer/service/q...
gpu/command_buffer/service/query_manager.cc:445: glGetSynciv(sync_,
GL_SYNC_STATUS, 1, NULL, &value);
On 2014/04/15 18:41:15, sievers wrote:
> See comment in gl_fence.cc that this does not work on OSX 10.7. It uses
> glClientWaitSync() != GL_TIMEOUT_EXPIRED instead.

Ok, makes sense to use GLFence and handle these type of workarounds in one
place.

[piman, 2014-04-15 22:41:12]

https://codereview.chromium.org/238933003/diff/20001/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/GLES2/gl2extchromium...
gpu/GLES2/gl2extchromium.h:695: /* GL_CHROMIUM_sync_query */
Can you write an extension for this? They go in gpu/GLES2/extensions/CHROMIUM/

[no sievers, 2014-04-15 23:53:37]

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/query_manager.cc:424:
fence_.reset(gfx::GLFence::CreateWithoutFlush());
Make sure piman@ is ok with this being used here :)

Also, can we maybe find a place where we can imply a flush?
Maybe at least from the client when the result is queried?

[piman, 2014-04-16 00:32:03]

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/build...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:844:
'GL_COMMANDS_COMPLETED_CHROMIUM',
We should only add this to the set of valid enums if the necessary underlying
extensions are available (i.e. in feature_info.cc)

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9515: case
GL_COMMANDS_COMPLETED_CHROMIUM:
We need to refuse this if the extension is not supported.

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/query_manager.cc:424:
fence_.reset(gfx::GLFence::CreateWithoutFlush());
On 2014/04/15 23:53:37, sievers wrote:
> Make sure piman@ is ok with this being used here :)
> 
> Also, can we maybe find a place where we can imply a flush?
> Maybe at least from the client when the result is queried?

The client only forces a shallow flush, because the (service-side)
glGetQueryObjectuivEXT is expected to imply a glFlush.

Note the wording in
http://www.khronos.org/registry/gles/extensions/EXT/EXT_occlusion_query_boole...:
    Querying the state for any given query object forces that occlusion query 
    to complete within a finite amount of time. Repeatedly querying the 
    QUERY_RESULT_AVAILABLE_EXT state for any given query object is guaranteed
    to return true eventually.


I'd prefer we explicitly flush here (GLFence::Create), unless we can actually
measure performance issues. If so, we can relax flushing here, but then we need
to implement the above guarantee somehow (and add tests for it).

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/query_manager.cc:425: DCHECK(fence_);
gfx::GLFence::Create (with or without flush) will fail if the underlying
extension is not available. See comment in build_gles2_cmd_buffer.py
That's why we need to refuse the enum.

[reveman, 2014-04-16 01:28:46]

https://codereview.chromium.org/238933003/diff/20001/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/GLES2/gl2extchromium...
gpu/GLES2/gl2extchromium.h:695: /* GL_CHROMIUM_sync_query */
On 2014/04/15 22:41:12, piman wrote:
> Can you write an extension for this? They go in gpu/GLES2/extensions/CHROMIUM/

Done.

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/build...
File gpu/command_buffer/build_gles2_cmd_buffer.py (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/build...
gpu/command_buffer/build_gles2_cmd_buffer.py:844:
'GL_COMMANDS_COMPLETED_CHROMIUM',
On 2014/04/16 00:32:03, piman wrote:
> We should only add this to the set of valid enums if the necessary underlying
> extensions are available (i.e. in feature_info.cc)

I'm assuming it's OK to handle this in HandleBeginQueryEXT where
ANY_SAMPLES_PASSED_* enums are validated.

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/gles2_cmd_decoder.cc (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/gles2_cmd_decoder.cc:9515: case
GL_COMMANDS_COMPLETED_CHROMIUM:
On 2014/04/16 00:32:03, piman wrote:
> We need to refuse this if the extension is not supported.

Done.

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/query_manager.cc (right):

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/query_manager.cc:424:
fence_.reset(gfx::GLFence::CreateWithoutFlush());
On 2014/04/16 00:32:03, piman wrote:
> On 2014/04/15 23:53:37, sievers wrote:
> > Make sure piman@ is ok with this being used here :)
> > 
> > Also, can we maybe find a place where we can imply a flush?
> > Maybe at least from the client when the result is queried?
> 
> The client only forces a shallow flush, because the (service-side)
> glGetQueryObjectuivEXT is expected to imply a glFlush.

Ah, looked like QueryTracker::Query::CheckResultsAvailable was issuing a real
flush but I see now that it doesn't. CommandBufferHelper and GLES2CmdHelper both
implementing a function named Flush and GLES2CmdHelper inheriting from
CommandBufferHelper is a bit confusing..

> 
> Note the wording in
>
http://www.khronos.org/registry/gles/extensions/EXT/EXT_occlusion_query_boole...:
>     Querying the state for any given query object forces that occlusion query 
>     to complete within a finite amount of time. Repeatedly querying the 
>     QUERY_RESULT_AVAILABLE_EXT state for any given query object is guaranteed
>     to return true eventually.
> 
> 
> I'd prefer we explicitly flush here (GLFence::Create), unless we can actually
> measure performance issues. If so, we can relax flushing here, but then we
need
> to implement the above guarantee somehow (and add tests for it).

I changed this to gfx::GLFence::Create() in latest patch. Not sure what the
performance impact of a flush after each CopyTexture and before each SwapBuffers
will have but I can look at that as a follow up.

https://codereview.chromium.org/238933003/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/query_manager.cc:425: DCHECK(fence_);
On 2014/04/16 00:32:03, piman wrote:
> gfx::GLFence::Create (with or without flush) will fail if the underlying
> extension is not available. See comment in build_gles2_cmd_buffer.py
> That's why we need to refuse the enum.

Got it.

[piman, 2014-04-16 01:33:12]

lgtm

[reveman, 2014-04-16 01:57:56]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-16 01:58:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/238933003/60001

[commit-bot: I haz the power, 2014-04-16 03:21:07]

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

[commit-bot: I haz the power, 2014-04-16 03:21:08]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[reveman, 2014-04-16 04:29:31]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-16 04:30:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/238933003/80001

[commit-bot: I haz the power, 2014-04-16 11:37:12]

Change committed as 264173

[hayato, 2014-04-17 03:52:57]

A revert of this CL has been created in
https://codereview.chromium.org/240273006/ by hayato@chromium.org.

The reason for reverting is: [gardening]

Seems that QueryTest.SyncQueryBasic started to fail on Android Tests (dbug).

http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg%29
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg...

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from QueryTest
[ RUN      ] QueryTest.SyncQueryBasic
../../gpu/command_buffer/tests/gl_query_unittest.cc:154: Failure
Value of: GLTestHelper::HasExtension("GL_CHROMIUM_sync_query")
  Actual: false
Expected: true
[0417/014324:ERROR:gles2_cmd_decoder.cc(9542)]
[GroupMarkerNotSet(crbug.com/242999)!:04B0A600]GL ERROR :GL_INVALID_OPERATION :
glBeginQueryEXT: not enabled for commands completed queries
[0417/014324:ERROR:gles2_cmd_decoder.cc(9617)]
[GroupMarkerNotSet(crbug.com/242999)!:04B0A600]GL ERROR :GL_INVALID_OPERATION :
glEndQueryEXT: No active query
../../gpu/command_buffer/tests/gl_query_unittest.cc:166: Failure
Value of: available
  Actual: false
Expected: true
../../gpu/command_buffer/tests/gl_manager.cc:241: Failure
Value of: glGetError() == GL_NONE
  Actual: false
Expected: true
[  FAILED  ] QueryTest.SyncQueryBasic (70 ms)
[----------] 1 test from QueryTest (71 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (71 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] QueryTest.SyncQueryBasic



In range 264168:264191, this patch might be the culprit.

Let me revert this patch and watch whether Android Tests will cycle to green or
not.
.

[reveman, 2014-04-17 20:25:35]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-04-17 20:26:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/238933003/100001

[commit-bot: I haz the power, 2014-04-17 23:48:29]

Change committed as 264675