cc: Add mailbox support to ResourceProvider write locks.

cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Committed: https://crrev.com/3b0f0b8d3db0a9f66864d5b7da87c82f49e74a29
Cr-Original-Commit-Position: refs/heads/master@{#398204}
Cr-Commit-Position: refs/heads/master@{#399983}

[sunnyps, 2016-05-05 01:56:35]

Description was changed from

==========
gpu: Synchronize async worker context with compositor.

This CL adds support for synchronizing async worker context with the
compositor context by using a sync token that's inserted in
RasterBufferProvider::OrderingBarrier. The sync token is waited on in
RasterBuffer::Playback. Only GpuRasterBufferProvider and
OneCopyRasterBufferProvider need this.

BUG=525259
R=piman@chromium.org
==========

to

==========
gpu: Synchronize async worker context with compositor.

This CL adds support for synchronizing async worker context with the
compositor context by using a sync token that's inserted in
RasterBufferProvider::OrderingBarrier. The sync token is waited on in
RasterBuffer::Playback. Only GpuRasterBufferProvider and
OneCopyRasterBufferProvider need this.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunnyps, 2016-05-06 01:12:25]

sunnyps@chromium.org changed reviewers:
+ reveman@chromium.org, vmiura@chromium.org

[sunnyps, 2016-05-06 01:12:25]

This is a follow-up to https://codereview.chromium.org/1949023005/ which just
adds the flag for async worker context.

PTAL

This works (for a loose definition of "works") even without the gpu scheduler.

I don't like that the mailbox stuff is done by ScopedWriteLockGr/GL while the
sync token stuff is done by the RasterBufferProvider. I'm open to suggestions on
how to improve this.

[piman, 2016-05-06 01:55:30]

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1021: ->ContextGL();
This path is taken in ResourceProvider::CopyToResource, which is called on the
compositor thread, so in that case we can't use the worker context (doubly so
without lock). Maybe the best is to pass the GLES2Interface to this function?

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1023: resource_->target,
resource_->mailbox().name());
When called on the worker thread, it's not safe to access resource_, because
it's a pointer to an element of a std::vector, which may be modified
(reallocated) on the compositor thread.
Instead you can grab those values in the constructor and keep them as fields?

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1028: return resource_->gl_id;
(ditto)

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1035: ->ContextGL();
And ditto wrt which context.

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1158: resource_->target,
resource_->mailbox().name());
Same as above, unsafe to access resource_

[sunnyps, 2016-05-25 02:18:43]

PTAL

As discussed previously accessing Resource pointers into ResourceProvider's map
from the worker threads is not incorrect but there are a couple of reasons to
not do that:
1. Make it easier to detect any data race for the Resource's internal state.
2. Make it easier to detect invalidation of the pointer e.g. because we switch
container types.

Apart from this there are some other cleanups (see the CL description.)

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1021: ->ContextGL();
On 2016/05/06 01:55:30, piman wrote:
> This path is taken in ResourceProvider::CopyToResource, which is called on the
> compositor thread, so in that case we can't use the worker context (doubly so
> without lock). Maybe the best is to pass the GLES2Interface to this function?

Done.

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1023: resource_->target,
resource_->mailbox().name());
On 2016/05/06 01:55:30, piman wrote:
> When called on the worker thread, it's not safe to access resource_, because
> it's a pointer to an element of a std::vector, which may be modified
> (reallocated) on the compositor thread.
> Instead you can grab those values in the constructor and keep them as fields?

Done.

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1028: return resource_->gl_id;
On 2016/05/06 01:55:30, piman wrote:
> (ditto)

Done.

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1035: ->ContextGL();
On 2016/05/06 01:55:30, piman wrote:
> And ditto wrt which context.

Done.

https://codereview.chromium.org/1951193002/diff/40001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1158: resource_->target,
resource_->mailbox().name());
On 2016/05/06 01:55:30, piman wrote:
> Same as above, unsafe to access resource_

Done.

[sunnyps, 2016-05-25 02:19:13]

Description was changed from

==========
gpu: Synchronize async worker context with compositor.

This CL adds support for synchronizing async worker context with the
compositor context by using a sync token that's inserted in
RasterBufferProvider::OrderingBarrier. The sync token is waited on in
RasterBuffer::Playback. Only GpuRasterBufferProvider and
OneCopyRasterBufferProvider need this.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. Remove unused parameter from OneCopyRasterBufferProvider::Create.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunnyps, 2016-05-25 22:25:59]

Description was changed from

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. Remove unused parameter from OneCopyRasterBufferProvider::Create.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[piman, 2016-05-26 00:01:10]

https://codereview.chromium.org/1951193002/diff/100001/cc/raster/gpu_raster_b...
File cc/raster/gpu_raster_buffer_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/100001/cc/raster/gpu_raster_b...
cc/raster/gpu_raster_buffer_provider.cc:184: DCHECK(sync_token_.HasData());
If I understand correctly, OrderingBarrier() is called on the compositor thread,
and this is read on a worker thread. What ensures there is no data race on
sync_token_? Is OrderingBarrier() only ever called when there is no raster task
active (that's not obvious to me).

[sunnyps, 2016-05-26 01:39:27]

PTAL

https://codereview.chromium.org/1951193002/diff/100001/cc/raster/gpu_raster_b...
File cc/raster/gpu_raster_buffer_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/100001/cc/raster/gpu_raster_b...
cc/raster/gpu_raster_buffer_provider.cc:184: DCHECK(sync_token_.HasData());
On 2016/05/26 00:01:10, piman wrote:
> If I understand correctly, OrderingBarrier() is called on the compositor
thread,
> and this is read on a worker thread. What ensures there is no data race on
> sync_token_? Is OrderingBarrier() only ever called when there is no raster
task
> active (that's not obvious to me).

You're right. I overlooked the data race here. We never block the compositor
thread on the worker thread (except shutdown I think).

As per the offline discussion I've implemented indirection for accessing the
sync token - see SyncTokenProvider. I'm not very fond of this so I'll look at
GenSyncToken and see if we can remove the OrderingBarrier requirement there and
move the check to WaitSyncToken.

[piman, 2016-05-26 01:46:25]

lgtm

[sunnyps, 2016-05-26 02:15:36]

On 2016/05/26 01:46:25, piman wrote:
> lgtm

Looks like it should be possible to move the IsFenceSyncFlushed check to
WaitSyncToken instead of GenUnverifiedSyncToken. We would have to keep track of
the flushed fence sync release in StreamFlushInfo but that should be ok. WDYT?

[piman, 2016-05-26 03:37:26]

On Wed, May 25, 2016 at 7:15 PM, <sunnyps@chromium.org> wrote:

> On 2016/05/26 01:46:25, piman wrote:
> > lgtm
>
> Looks like it should be possible to move the IsFenceSyncFlushed check to
> WaitSyncToken instead of GenUnverifiedSyncToken. We would have to keep
> track of
> the flushed fence sync release in StreamFlushInfo but that should be ok.
> WDYT?
>

I think it would also have to be checked in VerifySyncTokensCHROMIUM. Maybe
the right place would actually be in CanWaitUnverifiedSyncToken?
This becomes very subtle and subject to race conditions if not done
carefully - folks already routinely forget to do OrderingBarrier - so I
would not want this to be the default. Maybe if we make it a separate API
with a scary name (GenNonFlushedSyncTokenUnsafe)?


> https://codereview.chromium.org/1951193002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sunnyps, 2016-05-26 17:12:20]

On 2016/05/26 03:37:26, piman wrote:
> On Wed, May 25, 2016 at 7:15 PM, <mailto:sunnyps@chromium.org> wrote:
> 
> > On 2016/05/26 01:46:25, piman wrote:
> > > lgtm
> >
> > Looks like it should be possible to move the IsFenceSyncFlushed check to
> > WaitSyncToken instead of GenUnverifiedSyncToken. We would have to keep
> > track of
> > the flushed fence sync release in StreamFlushInfo but that should be ok.
> > WDYT?
> >
> 
> I think it would also have to be checked in VerifySyncTokensCHROMIUM. Maybe
> the right place would actually be in CanWaitUnverifiedSyncToken?
> This becomes very subtle and subject to race conditions if not done
> carefully - folks already routinely forget to do OrderingBarrier - so I
> would not want this to be the default. Maybe if we make it a separate API
> with a scary name (GenNonFlushedSyncTokenUnsafe)?

In any case that work doesn't need to block this CL.

> 
> 
> > https://codereview.chromium.org/1951193002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sunnyps, 2016-05-26 17:12:30]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2016-05-26 17:13:01]

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

[commit-bot: I haz the power, 2016-05-26 17:44:42]

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

[commit-bot: I haz the power, 2016-05-26 17:44:43]

Failed to apply patch for cc/tiles/tile_task_manager.cc:
While running git apply --index -3 -p1;
  error: patch failed: cc/tiles/tile_task_manager.cc:35
  Falling back to three-way merge...
  Applied patch to 'cc/tiles/tile_task_manager.cc' with conflicts.
  U cc/tiles/tile_task_manager.cc

Patch:       cc/tiles/tile_task_manager.cc
Index: cc/tiles/tile_task_manager.cc
diff --git a/cc/tiles/tile_task_manager.cc b/cc/tiles/tile_task_manager.cc
index
9f78d388eb4d22808abc89635e94cae88bb49883..b0d9d0f2487ccddb7756b33d683e267b356a0d16
100644
--- a/cc/tiles/tile_task_manager.cc
+++ b/cc/tiles/tile_task_manager.cc
@@ -35,6 +35,8 @@ TileTaskManagerImpl::~TileTaskManagerImpl() {
 void TileTaskManagerImpl::ScheduleTasks(TaskGraph* graph) {
   TRACE_EVENT0("cc", "TileTaskManagerImpl::ScheduleTasks");
 
+  raster_buffer_provider_->WillScheduleRasterTasks();
+
   for (TaskGraph::Node::Vector::iterator it = graph->nodes.begin();
        it != graph->nodes.end(); ++it) {
     TaskGraph::Node& node = *it;
@@ -47,7 +49,7 @@ void TileTaskManagerImpl::ScheduleTasks(TaskGraph* graph) {
     }
   }
 
-  raster_buffer_provider_->OrderingBarrier();
+  raster_buffer_provider_->DidScheduleRasterTasks();
 
   task_graph_runner_->ScheduleTasks(namespace_token_, graph);
 }

[sunnyps, 2016-05-26 21:17:40]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2016-05-26 21:17:41]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1951193002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-26 21:18:21]

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

[commit-bot: I haz the power, 2016-05-26 21:22:28]

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

[commit-bot: I haz the power, 2016-05-26 21:22:29]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunnyps, 2016-05-26 21:52:21]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2016-05-26 21:52:22]

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

[commit-bot: I haz the power, 2016-05-26 21:53:32]

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

[commit-bot: I haz the power, 2016-05-26 22:19:54]

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

[commit-bot: I haz the power, 2016-05-26 22:19:55]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunnyps, 2016-06-04 02:44:06]

So a lot changed while this CL was in review. Now we don't do the
AcquireRasterBuffer in ScheduleOnOriginThread but instead do it when creating
the raster task.

I'm going to make the raster buffer provider keep track of pending raster
buffers which are cleared just before ScheduleTasks (or on
ReleaseRasterBuffer/Shutdown). The raster buffers will be given the sync token
at that time.

[sunnyps, 2016-06-06 22:19:54]

Description was changed from

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunnyps, 2016-06-06 22:21:49]

PTAL

I had to change quite a bit from the previous version:
1. Instead of using SyncTokenProvider, RasterBufferProvider sets the sync token
on all the pending raster buffers in OrderingBarrier.
2. TileTaskManager does not own RasterBufferProvider.

[piman, 2016-06-06 23:22:45]

https://codereview.chromium.org/1951193002/diff/200001/cc/raster/raster_buffer.h
File cc/raster/raster_buffer.h (right):

https://codereview.chromium.org/1951193002/diff/200001/cc/raster/raster_buffe...
cc/raster/raster_buffer.h:30: virtual void SetSyncToken(const gpu::SyncToken&
sync_token) = 0;
Does this need to be in the interface? Both Gpu- and
OneCopy-RasterBufferProvider know the concrete type at construction time, so
they can keep the concrete type in their std::set, allowing SetSyncToken to be
non-virtual (and inline for that matter). It also avoids the NOTREACHED in the
other implementations.

[sunnyps, 2016-06-07 01:11:31]

Needed to make sure that pending_raster_buffers_ doesn't have dangling pointers
- made the RasterBufferImpl ctor/dtor take care of that.

PTAL

https://codereview.chromium.org/1951193002/diff/200001/cc/raster/raster_buffer.h
File cc/raster/raster_buffer.h (right):

https://codereview.chromium.org/1951193002/diff/200001/cc/raster/raster_buffe...
cc/raster/raster_buffer.h:30: virtual void SetSyncToken(const gpu::SyncToken&
sync_token) = 0;
On 2016/06/06 23:22:45, piman OOO back 2016-6-2 wrote:
> Does this need to be in the interface? Both Gpu- and
> OneCopy-RasterBufferProvider know the concrete type at construction time, so
> they can keep the concrete type in their std::set, allowing SetSyncToken to be
> non-virtual (and inline for that matter). It also avoids the NOTREACHED in the
> other implementations.

Done.

[piman, 2016-06-07 01:20:16]

LGTM, yeah I like this better re: dangling pointers.

[sunnyps, 2016-06-07 01:33:36]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2016-06-07 01:33:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951193002/220001

[commit-bot: I haz the power, 2016-06-07 02:39:23]

Description was changed from

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-07 02:39:24]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-06-07 02:40:44]

Description was changed from

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Cr-Commit-Position: refs/heads/master@{#398204}
==========

[commit-bot: I haz the power, 2016-06-07 02:40:46]

Patchset 12 (id:??) landed as
https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Cr-Commit-Position: refs/heads/master@{#398204}

[Geoff Lang, 2016-06-07 17:38:51]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2046033002/ by geofflang@chromium.org.

The reason for reverting is: Failing context_lost_tests on mac:
https://build.chromium.org/p/chromium.gpu/builders/Mac%2010.10%20Retina%20Rel...
Failing webgl_conformance_tests on Nexus 5:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Android%20Debug%20%28N...
and Nexus 6:
https://build.chromium.org/p/chromium.gpu.fyi/builders/Android%20Debug%20%28N....

[sunnyps, 2016-06-09 23:47:20]

Description was changed from

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Cr-Commit-Position: refs/heads/master@{#398204}
==========

to

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Cr-Commit-Position: refs/heads/master@{#398204}
==========

[sunnyps, 2016-06-09 23:48:26]

PTAL

No more scheduling tasks if sync token creation fails.

[piman, 2016-06-10 01:02:47]

lgtm

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/gpu_raster_b...
File cc/raster/gpu_raster_buffer_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/gpu_raster_b...
cc/raster/gpu_raster_buffer_provider.cc:162: // Do not proceed with
ScheduleTasks if sync token was invalid.
nit: maybe add a DCHECK(sync_token.HasData() || gl->GetGraphicsResetStatusKHR()
!= GL_NO_ERROR)  ?

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/one_copy_ras...
File cc/raster/one_copy_raster_buffer_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/one_copy_ras...
cc/raster/one_copy_raster_buffer_provider.cc:129: // Do not proceed with
ScheduleTasks if sync token was invalid.
nit: same here

[sunnyps, 2016-06-10 22:13:55]

Fixed the invalid mailbox bug due to consuming the mailbox on the worker thread
without a wait. Also made some changes to ResourceProvider that make it more
robust re: sync tokens.

PTAL

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/gpu_raster_b...
File cc/raster/gpu_raster_buffer_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/gpu_raster_b...
cc/raster/gpu_raster_buffer_provider.cc:162: // Do not proceed with
ScheduleTasks if sync token was invalid.
On 2016/06/10 01:02:47, piman wrote:
> nit: maybe add a DCHECK(sync_token.HasData() ||
gl->GetGraphicsResetStatusKHR()
> != GL_NO_ERROR)  ?

Done.

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/one_copy_ras...
File cc/raster/one_copy_raster_buffer_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/one_copy_ras...
cc/raster/one_copy_raster_buffer_provider.cc:129: // Do not proceed with
ScheduleTasks if sync token was invalid.
On 2016/06/10 01:02:47, piman wrote:
> nit: same here

Done.

[sunnyps, 2016-06-10 22:13:56]

Fixed the invalid mailbox bug due to consuming the mailbox on the worker thread
without a wait. Also made some changes to ResourceProvider that make it more
robust re: sync tokens.

PTAL

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/gpu_raster_b...
File cc/raster/gpu_raster_buffer_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/gpu_raster_b...
cc/raster/gpu_raster_buffer_provider.cc:162: // Do not proceed with
ScheduleTasks if sync token was invalid.
On 2016/06/10 01:02:47, piman wrote:
> nit: maybe add a DCHECK(sync_token.HasData() ||
gl->GetGraphicsResetStatusKHR()
> != GL_NO_ERROR)  ?

Done.

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/one_copy_ras...
File cc/raster/one_copy_raster_buffer_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/240001/cc/raster/one_copy_ras...
cc/raster/one_copy_raster_buffer_provider.cc:129: // Do not proceed with
ScheduleTasks if sync token was invalid.
On 2016/06/10 01:02:47, piman wrote:
> nit: same here

Done.

[piman, 2016-06-10 22:57:01]

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
cc/resources/resource_provider.cc:920: // DCHECK_NE(Resource::NEEDS_WAIT,
resource->synchronization_state());
nit: I think you should leave this (and remove the wait).

This is used on the browser compositor, in the parent, and we do want to make
sure we correctly synchronized in GLRenderer::BeginDrawingFrame, because
otherwise we could cause a context switch in the middle of drawing, which has
very bad performance on mobile.

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
cc/resources/resource_provider.cc:997: // DCHECK_NE(Resource::NEEDS_WAIT,
resource->synchronization_state());
nit: remove

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
cc/resources/resource_provider.cc:1022: // resource->SetSynchronized();
nit: remove.

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
cc/resources/resource_provider.cc:1406: source->SetSynchronized();
Is this right?
If a resource comes from the raster thread, we would have generated the sync
token on the worker context, but nothing here would wait for it on the
compositor thread, so I don't think it's right to SetSynchronized here. It's
valid to do on the need_synchronization_resources ones, because they were
created on the compositor context, and we just generated a token for them.

[sunnyps, 2016-06-11 01:29:52]

Addressed the comments and also added a synchronized flag to ScopedWriteLockGL
so that we don't do unnecessary waits when we're not in async mode.

PTAL

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
cc/resources/resource_provider.cc:920: // DCHECK_NE(Resource::NEEDS_WAIT,
resource->synchronization_state());
On 2016/06/10 22:57:01, piman wrote:
> nit: I think you should leave this (and remove the wait).
> 
> This is used on the browser compositor, in the parent, and we do want to make
> sure we correctly synchronized in GLRenderer::BeginDrawingFrame, because
> otherwise we could cause a context switch in the middle of drawing, which has
> very bad performance on mobile.

Done.

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
cc/resources/resource_provider.cc:997: // DCHECK_NE(Resource::NEEDS_WAIT,
resource->synchronization_state());
On 2016/06/10 22:57:01, piman wrote:
> nit: remove

Done.

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
cc/resources/resource_provider.cc:1022: // resource->SetSynchronized();
On 2016/06/10 22:57:01, piman wrote:
> nit: remove.

Done.

https://codereview.chromium.org/1951193002/diff/340001/cc/resources/resource_...
cc/resources/resource_provider.cc:1406: source->SetSynchronized();
On 2016/06/10 22:57:01, piman wrote:
> Is this right?
> If a resource comes from the raster thread, we would have generated the sync
> token on the worker context, but nothing here would wait for it on the
> compositor thread, so I don't think it's right to SetSynchronized here. It's
> valid to do on the need_synchronization_resources ones, because they were
> created on the compositor context, and we just generated a token for them.

Done.

[piman, 2016-06-11 01:51:36]

lgtm

[sunnyps, 2016-06-13 22:02:07]

Made software/GMB write locks set the resource to synchronized to appease the
DCHECKS in PrepareSendToParent. This is what UnlockForWrite did before.

Also added calls to SetSynchronized in a few other places  such as
CopyToResource, GenerateSyncTokenForResource(s) to avoid unnecessary waits.

PTAL

[piman, 2016-06-14 00:32:21]

lgtm

[sunnyps, 2016-06-14 21:37:02]

I've made raster/decode tasks cleanup properly if we choose to not schedule
them.

Also adding vmpstr@ and ericrk@ to check if code in TileManager::ScheduleTasks
looks correct. We check if the sync token generated by the raster buffer
provider is invalid and if so raster/decode tasks are canceled (and completed)
immediately. However activation/draw/all completion tasks are still scheduled so
that their callbacks still run. I'm open to suggestions on how to improve this
and if we should do this at all.

PTAL

[ericrk, 2016-06-14 22:36:03]

ericrk@chromium.org changed reviewers:
+ ericrk@chromium.org

[ericrk, 2016-06-14 22:36:05]

LGTM - canceling the tile tasks makes sense to me.

https://codereview.chromium.org/1951193002/diff/420001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/420001/cc/resources/resource_...
cc/resources/resource_provider.cc:702: (style == FOR_SHUTDOWN &&
resource->exported_count > 0) ||
nit: It might be nice to break this into a couple bools with some explanation of
what each scenario means.

https://codereview.chromium.org/1951193002/diff/420001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1951193002/diff/420001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:907: required_for_activate_count);
Won't |required_for_activate_count| be wrong at this point? I believe this will
fail TaskGraphWorkQueue::DependencyMismatch if this is run in Debug mode and we
hit the context lost condition.

Maybe zero these values above if we lose context.

[sunnyps, 2016-06-14 23:22:52]

Thanks!

piman@ Can you please take a final look at this?

https://codereview.chromium.org/1951193002/diff/420001/cc/resources/resource_...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1951193002/diff/420001/cc/resources/resource_...
cc/resources/resource_provider.cc:702: (style == FOR_SHUTDOWN &&
resource->exported_count > 0) ||
On 2016/06/14 22:36:05, ericrk wrote:
> nit: It might be nice to break this into a couple bools with some explanation
of
> what each scenario means.

Done.

https://codereview.chromium.org/1951193002/diff/420001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1951193002/diff/420001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:907: required_for_activate_count);
On 2016/06/14 22:36:05, ericrk wrote:
> Won't |required_for_activate_count| be wrong at this point? I believe this
will
> fail TaskGraphWorkQueue::DependencyMismatch if this is run in Debug mode and
we
> hit the context lost condition.
> 
> Maybe zero these values above if we lose context.

Done.

[piman, 2016-06-15 00:30:22]

lgtm

[vmpstr, 2016-06-15 00:39:33]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2016-06-15 00:39:34]

lgtm with nit

https://codereview.chromium.org/1951193002/diff/440001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1951193002/diff/440001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:889: // Synchronize worker with compositor. If that
fails do not schedule tasks.
Can you expand this comment a little bit to mention why we do this? It's a bit
awkward that we don't bother freeing up resources here (like locked images that
we locked above). I think they will still be properly freed up in Finish
function, but it'd be nice to explain that the goal here is to just prevent new
raster from happening.

[sunnyps, 2016-06-15 00:42:00]

https://codereview.chromium.org/1951193002/diff/440001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1951193002/diff/440001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:889: // Synchronize worker with compositor. If that
fails do not schedule tasks.
On 2016/06/15 00:39:34, vmpstr wrote:
> Can you expand this comment a little bit to mention why we do this? It's a bit
> awkward that we don't bother freeing up resources here (like locked images
that
> we locked above). I think they will still be properly freed up in Finish
> function, but it'd be nice to explain that the goal here is to just prevent
new
> raster from happening.

Calling task->OnTaskCompleted will free up resources. I'll add a comment
explaining that we want to avoid new raster work.

[sunnyps, 2016-06-15 01:56:58]

Thanks for all the reviews!

https://codereview.chromium.org/1951193002/diff/440001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1951193002/diff/440001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:889: // Synchronize worker with compositor. If that
fails do not schedule tasks.
On 2016/06/15 00:42:00, sunnyps wrote:
> On 2016/06/15 00:39:34, vmpstr wrote:
> > Can you expand this comment a little bit to mention why we do this? It's a
bit
> > awkward that we don't bother freeing up resources here (like locked images
> that
> > we locked above). I think they will still be properly freed up in Finish
> > function, but it'd be nice to explain that the goal here is to just prevent
> new
> > raster from happening.
> 
> Calling task->OnTaskCompleted will free up resources. I'll add a comment
> explaining that we want to avoid new raster work.

Done.

[sunnyps, 2016-06-15 01:57:08]

The CQ bit was checked by sunnyps@chromium.org

[sunnyps, 2016-06-15 01:57:09]

The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org,
piman@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1951193002/#ps460001
(title: "vmpstr's review")

[commit-bot: I haz the power, 2016-06-15 01:57:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951193002/460001

[commit-bot: I haz the power, 2016-06-15 03:09:15]

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

[commit-bot: I haz the power, 2016-06-15 03:09:17]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sunnyps, 2016-06-15 18:31:08]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2016-06-15 18:31:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1951193002/460001

[commit-bot: I haz the power, 2016-06-15 19:07:16]

Description was changed from

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Cr-Commit-Position: refs/heads/master@{#398204}
==========

to

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Cr-Commit-Position: refs/heads/master@{#398204}
==========

[commit-bot: I haz the power, 2016-06-15 19:07:21]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2016-06-15 19:07:35]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 19:10:05]

Description was changed from

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Cr-Commit-Position: refs/heads/master@{#398204}
==========

to

==========
cc: Add mailbox support to ResourceProvider write locks.

This adds support for mailboxes to ScopedWriteLockGL. Using the mailbox
requires using ScopedTextureProvider/ScopedSkSurfaceProvider which
ensures that the texture id for the mailbox is destroyed after use on
the worker context.

This CL also includes the following cleanup:
1. ResourceProvider locks don't keep resource pointers around.
2. ScopedSamplerGL does not inherit from ScopedReadLockGL.
3. GpuRasterizer is folded back into GpuRasterBufferProvider.
4. TileTaskManager does not own RasterBufferProvider.

BUG=525259
R=piman@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/5fa5dbdf25bbec21b84f752d3f0642cd184467e2
Committed: https://crrev.com/3b0f0b8d3db0a9f66864d5b7da87c82f49e74a29
Cr-Original-Commit-Position: refs/heads/master@{#398204}
Cr-Commit-Position: refs/heads/master@{#399983}
==========

[commit-bot: I haz the power, 2016-06-15 19:10:07]

Patchset 24 (id:??) landed as
https://crrev.com/3b0f0b8d3db0a9f66864d5b7da87c82f49e74a29
Cr-Commit-Position: refs/heads/master@{#399983}

[sunnyps, 2016-06-21 00:10:49]

A revert of this CL (patchset #24 id:460001) has been created in
https://codereview.chromium.org/2081863002/ by sunnyps@chromium.org.

The reason for reverting is: Bug 621422, 621130

Some tasks can be completed twice which leads to crashes..