Converted video frame and image callbacks to use new sync tokens.

Converted video frame and image callbacks to use new sync tokens.

As an incremental step towards utilizing the new sync tokens, this
CL converts existing video frame and image sync points to use
sync tokens instead.

In order to accomplish this, the GpuCommandBufferMsg_CreateImage
IPC message has been modified to accept a fence_release parameter
so that it can act as a sync token IPC.

A new SyncPointClientWaiter concept has also added which can wait
on other sync point clients without an associated order number.
This only works because the SyncPointClientWaiter cannot be waited
on so no deadlocks can occur.

BUG=514815
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/c7aff68a0c11820cc2b8d488851c682c0d32cd2b
Cr-Commit-Position: refs/heads/master@{#357997}

[David Yen, 2015-11-04 18:38:12]

Description was changed from

==========
Converted video frame and image callbacks to use new sync tokens.

BUG=514815
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Converted video frame and image callbacks to use new sync tokens.

As an incremental step towards utilizing the new sync tokens, this
CL converts existing video frame and image sync points to use
sync tokens instead.

In order to accomplish this, the GpuCommandBufferMsg_CreateImage
IPC message has been modified to accept a fence_release parameter
so that it can act as a sync token IPC.

A new SyncPointClientWaiter concept has also added which can wait
on other sync point clients without an associated order number.
This only works because the SyncPointClientWaiter cannot be waited
on so no deadlocks can occur.

BUG=514815
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[David Yen, 2015-11-04 18:58:36]

dyen@chromium.org changed reviewers:
+ dalecurtis@chromium.org, dcheng@chromium.org, piman@chromium.org

[David Yen, 2015-11-04 18:58:36]

I've modified the image/frame construction/destruction sync points to use sync
tokens instead now. Most of it is simply converting InsertSyncPoint to
InsertFenceSync, Flush, GenSyncToken. There are 2 slightly more complex changes
though:

1. The CreateImage IPC sends a fence sync to release sync tokens.

2. To make destruction be able to wait on fence syncs (they are not part of the
command buffer so are completely separate), I've added the concept of a
SyncPointCilentWaiter which is like a SyncPointClient except it has no
associated order number and it can only wait. Technically this functionality
could have been added to the SyncPointManager (so that anyone who has access to
the SyncPointManager can just wait even if they don't have an order number), but
that would potentially be dangerous because if a thread which owns a
SyncPointClient accidentally calls that, it could cause a deadlock. Because of
that I made this functionality a totally separate class with a big warning
message as a comment.

+piman for review

+dcheng for IPC review

+dalecurtis for media review

[dcheng, 2015-11-04 19:22:43]

https://codereview.chromium.org/1429213002/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1429213002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:429:
DCHECK((image_fence_sync - 1) <= flushed_fence_sync_release_);
DCHECK_LE

https://codereview.chromium.org/1429213002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1429213002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:192: IPC_STRUCT_MEMBER(uint32, internalformat)
Google style says this should be internal_format.

https://codereview.chromium.org/1429213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1429213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:697: DCHECK((fence_sync
- 1) == flushed_fence_sync_release_);
Nit: DCHECK_EQ

https://codereview.chromium.org/1429213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:700:
QueueTask(base::Bind(&InProcessCommandBuffer::CreateImageOnGpuThread,
Can you throw in a comment about why base::Unretained() is safe here?

[David Yen, 2015-11-04 19:47:40]

https://codereview.chromium.org/1429213002/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1429213002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/command_buffer_proxy_impl.cc:429:
DCHECK((image_fence_sync - 1) <= flushed_fence_sync_release_);
On 2015/11/04 19:22:43, dcheng wrote:
> DCHECK_LE

Done.

https://codereview.chromium.org/1429213002/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/1429213002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_messages.h:192: IPC_STRUCT_MEMBER(uint32, internalformat)
On 2015/11/04 19:22:43, dcheng wrote:
> Google style says this should be internal_format.

Done.

https://codereview.chromium.org/1429213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1429213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:697: DCHECK((fence_sync
- 1) == flushed_fence_sync_release_);
On 2015/11/04 19:22:43, dcheng wrote:
> Nit: DCHECK_EQ

Done.

https://codereview.chromium.org/1429213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:700:
QueueTask(base::Bind(&InProcessCommandBuffer::CreateImageOnGpuThread,
On 2015/11/04 19:22:43, dcheng wrote:
> Can you throw in a comment about why base::Unretained() is safe here?

This whole class is structured this way to pass messages to the gpu thread:

"function -> QueueTask(functionOnGpuThread, base::Unratained(this)...)"

Seems out of place to add a comment here when it's the pattern of the whole
file. Perhaps it can be added somewhere else? maybe above QueueTask()?

[David Yen, 2015-11-04 21:16:21]

dyen@chromium.org changed reviewers:
+ sky@chromium.org

[David Yen, 2015-11-04 21:16:21]

+sky@ for mojo changes

[sky, 2015-11-04 21:50:08]

LGTM

[piman, 2015-11-05 00:04:36]

https://codereview.chromium.org/1429213002/diff/120001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1429213002/diff/120001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:83: gl_->Flush();
ShallowFlushCHROMIUM instead off Flush:
- it doesn't imply a glFlush command, just kicking the work to the GPU process,
which is enough here
- InsertSyncPointCHROMIUM did not include a glFlush command.

https://codereview.chromium.org/1429213002/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1429213002/diff/120001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:455:
channel_->ValidateFlushIDReachedServer(stream_id_, true);
A potential optimization: we could make GpuCommandBufferMsg_CreateImage a sync
IPC that replies from the GPU process IO thread. That way we would avoid the
extra IPC.
Maybe leave it as a TODO?

https://codereview.chromium.org/1429213002/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/1429213002/diff/120001/content/common/gpu/cli...
content/common/gpu/client/gl_helper.cc:959: gl_->Flush();
ShallowFlushCHROMIUM (same reason as above).

https://codereview.chromium.org/1429213002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1429213002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:242: // deadlock could occur.
That isn't consistent with the usage in GpuChannelManager (same thread as
GpuCommandBufferStub which has a SyncPointClient).

https://codereview.chromium.org/1429213002/diff/120001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1429213002/diff/120001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:68: gl_->Flush();
ShallowFlushCHROMIUM

[David Yen, 2015-11-05 00:34:27]

https://codereview.chromium.org/1429213002/diff/120001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1429213002/diff/120001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:83: gl_->Flush();
On 2015/11/05 00:04:36, piman (slow to review) wrote:
> ShallowFlushCHROMIUM instead off Flush:
> - it doesn't imply a glFlush command, just kicking the work to the GPU
process,
> which is enough here
> - InsertSyncPointCHROMIUM did not include a glFlush command.

Done.

https://codereview.chromium.org/1429213002/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/1429213002/diff/120001/content/common/gpu/cli...
content/common/gpu/client/command_buffer_proxy_impl.cc:455:
channel_->ValidateFlushIDReachedServer(stream_id_, true);
On 2015/11/05 00:04:36, piman (slow to review) wrote:
> A potential optimization: we could make GpuCommandBufferMsg_CreateImage a sync
> IPC that replies from the GPU process IO thread. That way we would avoid the
> extra IPC.
> Maybe leave it as a TODO?

One advantage to having GpuChannelHost doing it is that it will validate any
other fences as well. That might be reason enough to just let GpuChannelHost to
do all the validating.

https://codereview.chromium.org/1429213002/diff/120001/content/common/gpu/cli...
File content/common/gpu/client/gl_helper.cc (right):

https://codereview.chromium.org/1429213002/diff/120001/content/common/gpu/cli...
content/common/gpu/client/gl_helper.cc:959: gl_->Flush();
On 2015/11/05 00:04:36, piman (slow to review) wrote:
> ShallowFlushCHROMIUM (same reason as above).

Done.

https://codereview.chromium.org/1429213002/diff/120001/gpu/command_buffer/ser...
File gpu/command_buffer/service/sync_point_manager.h (right):

https://codereview.chromium.org/1429213002/diff/120001/gpu/command_buffer/ser...
gpu/command_buffer/service/sync_point_manager.h:242: // deadlock could occur.
On 2015/11/05 00:04:36, piman (slow to review) wrote:
> That isn't consistent with the usage in GpuChannelManager (same thread as
> GpuCommandBufferStub which has a SyncPointClient).

For some reason I thought control messages were handled on the IO thread. I
think the rule is more like it can't be used in the same execution context as a
sync point client (between BeginProcessingOrderNumber and
FinishProcessingOrderNumber for a SyncPointOrderData object). I've updated the
comment to try to convey this...

https://codereview.chromium.org/1429213002/diff/120001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/1429213002/diff/120001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:68: gl_->Flush();
On 2015/11/05 00:04:36, piman (slow to review) wrote:
> ShallowFlushCHROMIUM

Done.

[piman, 2015-11-05 00:44:32]

lgtm

[dcheng, 2015-11-05 00:59:22]

IPC changes lgtm

https://codereview.chromium.org/1429213002/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/in_process_command_buffer.cc (right):

https://codereview.chromium.org/1429213002/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/in_process_command_buffer.cc:700:
QueueTask(base::Bind(&InProcessCommandBuffer::CreateImageOnGpuThread,
On 2015/11/04 at 19:47:40, David Yen wrote:
> On 2015/11/04 19:22:43, dcheng wrote:
> > Can you throw in a comment about why base::Unretained() is safe here?
> 
> This whole class is structured this way to pass messages to the gpu thread:
> 
> "function -> QueueTask(functionOnGpuThread, base::Unratained(this)...)"
> 
> Seems out of place to add a comment here when it's the pattern of the whole
file. Perhaps it can be added somewhere else? maybe above QueueTask()?

It doesn't have to be this patch (as this appears to be a pre-existing pattern).
But anytime base::Unretained is used, it's hard for a reviewer like me (who
isn't an expert in GPU stuff) to figure out how/if this sort of thing is safe.

If it's something prevalent in a file, maybe document it in a comment block at
the top that details how the callbacks of this class work and why Unretained is
used? etc.

[DaleCurtis, 2015-11-05 01:09:30]

media/ lgtm

[David Yen, 2015-11-05 02:05:48]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-11-05 02:05:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1429213002/#ps160001
(title: "fixed typo")

[commit-bot: I haz the power, 2015-11-05 02:06:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1429213002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1429213002/160001

[commit-bot: I haz the power, 2015-11-05 03:49:33]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-11-05 03:50:21]

Patchset 9 (id:??) landed as
https://crrev.com/c7aff68a0c11820cc2b8d488851c682c0d32cd2b
Cr-Commit-Position: refs/heads/master@{#357997}

[reveman, 2015-11-05 05:37:09]

On 2015/11/05 at 03:50:21, commit-bot wrote:
> Patchset 9 (id:??) landed as
https://crrev.com/c7aff68a0c11820cc2b8d488851c682c0d32cd2b
> Cr-Commit-Position: refs/heads/master@{#357997}

FYI, all exosphere (https://codereview.chromium.org/1412093006) unittests
started failing with something like the following as a result of this change:

124868:124868:1105/003439:453320867172:FATAL:gles2_implementation.cc(66)] Check
failed: 0 == gles2_implementation_->use_count_ (0 vs. 1)
#0 0x7f830a5ecd4e base::debug::StackTrace::StackTrace()
#1 0x7f830a502f8f logging::LogMessage::~LogMessage()
#2 0x7f8307fee7ca
gpu::gles2::GLES2Implementation::SingleThreadChecker::SingleThreadChecker()
#3 0x7f8307ff63ff gpu::gles2::GLES2Implementation::ShallowFlushCHROMIUM()
#4 0x7f83080159cb gpu::gles2::GLES2Implementation::CreateImageCHROMIUMHelper()
#5 0x7f8308015d92 gpu::gles2::GLES2Implementation::CreateImageCHROMIUM()
#6 0x0000010f5f4c exo::Buffer::Buffer()
#7 0x0000010f8963 exo::SharedMemory::CreateBuffer()
#8 0x000000526f62 exo::(anonymous
namespace)::SharedMemoryTest_CreateBuffer_Test::TestBody()
#9 0x0000009ed66a
_ZN7testing8internal12InvokeHelperIjNSt3tr15tupleIJEEEE12InvokeMethodIN3gfx13GPUTimingFakeEMS8_FjvEEEjPT_T0_RKS4_
#10 0x0000011274fe testing::internal::HandleExceptionsInMethodIfSupported<>()
#11 0x00000111c155 testing::Test::Run()
#12 0x00000111c8e8 testing::TestInfo::Run()
#13 0x00000111ce8a testing::TestCase::Run()
#14 0x00000112221c testing::internal::UnitTestImpl::RunAllTests()
#15 0x00000113b1fa testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#16 0x000001128cde testing::internal::HandleExceptionsInMethodIfSupported<>()
#17 0x000001121ebf testing::UnitTest::Run()
#18 0x000000762741 RUN_ALL_TESTS()
#19 0x0000007616f5 base::TestSuite::Run()
#20 0x0000004fafc9 base::internal::RunnableAdapter<>::Run()
#21 0x0000004faf0f
_ZN4base8internal12InvokeHelperILb0EiNS0_15RunnableAdapterIMNS_9TestSuiteEFivEEENS0_8TypeListIJPN3ash4test18AuraShellTestSuiteEEEEE8MakeItSoES6_SB_
#22 0x0000004faed8
_ZN4base8internal7InvokerINS_13IndexSequenceIJLm0EEEENS0_9BindStateINS0_15RunnableAdapterIMNS_9TestSuiteEFivEEEFiPS6_ENS0_8TypeListIJNS0_17UnretainedWrapperIN3ash4test18AuraShellTestSuiteEEEEEEEENSC_IJNS0_12UnwrapTraitsISH_EEEEENS0_12InvokeHelperILb0EiS9_NSC_IJPSG_EEEEEFivEE3RunEPNS0_13BindStateBaseE
#23 0x00000050e32e base::Callback<>::Run()
#24 0x000000799bb6 base::(anonymous namespace)::LaunchUnitTestsInternal()
#25 0x000000799ef8 base::LaunchUnitTestsSerially()
#26 0x0000004faa7e main
#27 0x7f83022ccec5 __libc_start_main
#28 0x0000004fa8f4 <unknown>

[kochi, 2015-11-05 05:48:08]

On 2015/11/05 05:37:09, reveman wrote:
> On 2015/11/05 at 03:50:21, commit-bot wrote:
> > Patchset 9 (id:??) landed as
> https://crrev.com/c7aff68a0c11820cc2b8d488851c682c0d32cd2b
> > Cr-Commit-Position: refs/heads/master@{#357997}
> 
> FYI, all exosphere (https://codereview.chromium.org/1412093006) unittests
> started failing with something like the following as a result of this change:
> 
> 124868:124868:1105/003439:453320867172:FATAL:gles2_implementation.cc(66)]
Check
> failed: 0 == gles2_implementation_->use_count_ (0 vs. 1)
> #0 0x7f830a5ecd4e base::debug::StackTrace::StackTrace()
> #1 0x7f830a502f8f logging::LogMessage::~LogMessage()
> #2 0x7f8307fee7ca
> gpu::gles2::GLES2Implementation::SingleThreadChecker::SingleThreadChecker()
> #3 0x7f8307ff63ff gpu::gles2::GLES2Implementation::ShallowFlushCHROMIUM()

Related, I'm getting the same CHECK() when debugging on Linux with
--single-process.

[nhiroki, 2015-11-05 05:50:54]

On 2015/11/05 05:48:08, Takayoshi Kochi wrote:
> On 2015/11/05 05:37:09, reveman wrote:
> > On 2015/11/05 at 03:50:21, commit-bot wrote:
> > > Patchset 9 (id:??) landed as
> > https://crrev.com/c7aff68a0c11820cc2b8d488851c682c0d32cd2b
> > > Cr-Commit-Position: refs/heads/master@{#357997}
> > 
> > FYI, all exosphere (https://codereview.chromium.org/1412093006) unittests
> > started failing with something like the following as a result of this
change:
> > 
> > 124868:124868:1105/003439:453320867172:FATAL:gles2_implementation.cc(66)]
> Check
> > failed: 0 == gles2_implementation_->use_count_ (0 vs. 1)
> > #0 0x7f830a5ecd4e base::debug::StackTrace::StackTrace()
> > #1 0x7f830a502f8f logging::LogMessage::~LogMessage()
> > #2 0x7f8307fee7ca
> > gpu::gles2::GLES2Implementation::SingleThreadChecker::SingleThreadChecker()
> > #3 0x7f8307ff63ff gpu::gles2::GLES2Implementation::ShallowFlushCHROMIUM()
> 
> Related, I'm getting the same CHECK() when debugging on Linux with
> --single-process.

As a sheriff, I'll revert this CL to fix webkit_tests.

[nhiroki, 2015-11-05 05:54:44]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1414793018/ by nhiroki@chromium.org.

The reason for reverting is: This could be breaking webkit_tests:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%...
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7%20%28db....