cc: Move RasterBuffer implementations from ResourceProvider to RasterWorkerPool implementations.

cc: Move RasterBuffer implementations from ResourceProvider to RasterWorkerPool implementations.

Each RasterWorkerPool implementation is tied to some
RasterBuffer implementation. By moving the RasterBuffer
code into RasterWorkerPool implementation this
relationship becomes more explicit and more easily
adjusted for the need of the RasterWorkerPool
implementation.

BUG=406404
Committed: https://crrev.com/47560aba9ae6e235795068d6e78dcf4ea0628e63
Cr-Commit-Position: refs/heads/master@{#295528}

[reveman, 2014-09-16 21:11:28]

reveman@chromium.org changed reviewers:
+ danakj@chromium.org, vmpstr@chromium.org

[reveman, 2014-09-16 21:11:29]

Please take a look. You can skip ImageCopyRasterWorkerPool for now as I'll move
most of these changes to a separate patch asap.

[vmpstr, 2014-09-16 22:15:56]

Overall looks like a nice cleanup, I didn't take a look at
ImageCopyRasterWorkerPool though, and some comments below.

https://codereview.chromium.org/562833004/diff/60001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:21: class RasterBufferImpl : public
RasterBuffer {
<nit>
Can we come up with a name that isn't using Impl somehow. I know this is a more
proper way of naming this type of class, but given that we have the whole
impl-thread thing, I think this makes it somewhat confusing. Maybe
ScopedRasterBuffer, since it locks/unlocks things in the ctor/dtor (or
LockedRasterBuffer, RasterBufferAutoLock)?

I think we should also file a bug and/or decide whether we allow FooImpl in cc
as an implementation class of Foo, or whether things like PictureLayerImpl have
to get a better name
</nit>

https://codereview.chromium.org/562833004/diff/60001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:36: :
skia::AdoptRef(SkCreateNullCanvas());
The previous code did "return skia::RefPtr<SkCanvas>();" type of thing. Is this
equivalent? That is to say, if the return value was used in DCHECKs or ifs,
would it evaluate to the same thing?

https://codereview.chromium.org/562833004/diff/60001/cc/resources/image_copy_...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:30: : worker_pool_(worker_pool),
client? Up to you.

https://codereview.chromium.org/562833004/diff/60001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:319: for (size_t i = 0; i < count;
++i) {
maybe:

while (count-- && !pending_resource_copy_operations_.empty()) {
  ...
}

https://codereview.chromium.org/562833004/diff/60001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:721:
resource_provider_->UnmapPixelBuffer(raster_task->resource()->id());
Can you explain why this is now needed?

https://codereview.chromium.org/562833004/diff/60001/cc/resources/raster_buff...
File cc/resources/raster_buffer.cc (left):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/raster_buff...
cc/resources/raster_buffer.cc:7: namespace media {
O_O

I'm guessing git decided that you copied this :)

https://codereview.chromium.org/562833004/diff/60001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:125: DCHECK(canvas);
Oh I see the change now from NULL ptr to NullCanvas. Is it possible to do this
as a separate change either before or after moving the code?

[reveman, 2014-09-17 14:40:57]

PTAL

https://codereview.chromium.org/562833004/diff/60001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:21: class RasterBufferImpl : public
RasterBuffer {
On 2014/09/16 22:15:55, vmpstr wrote:
> <nit>
> Can we come up with a name that isn't using Impl somehow. I know this is a
more
> proper way of naming this type of class, but given that we have the whole
> impl-thread thing, I think this makes it somewhat confusing. Maybe
> ScopedRasterBuffer, since it locks/unlocks things in the ctor/dtor (or
> LockedRasterBuffer, RasterBufferAutoLock)?

RasterBufferImpl is a more compact way of writing
GpuRasterWorkerPoolRasterBuffer in this case. We could just name it that,
although I find it a bit annoying that we can't use the "Impl" suffix in a way
that's consistent with the rest of the chromium code.

> 
> I think we should also file a bug and/or decide whether we allow FooImpl in cc
> as an implementation class of Foo, or whether things like PictureLayerImpl
have
> to get a better name
> </nit>

I don't think PictureLayerImpl is too bad. Yes, there's no explicit PictureLayer
interface but it's in some sense implementing a "picture layer".

We could instead use PictureLayerHost on the main thread and PictureLayer on the
compositor thread. That would probably be a bit more consistent with the rest of
chromium but LayerTreeHostImpl would become a bit confusing. Not sure what's the
best solution is. I think we should wait until all non impl-side painting code
has been removed before we make any changes like this though.

https://codereview.chromium.org/562833004/diff/60001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:36: :
skia::AdoptRef(SkCreateNullCanvas());
On 2014/09/16 22:15:55, vmpstr wrote:
> The previous code did "return skia::RefPtr<SkCanvas>();" type of thing. Is
this
> equivalent? That is to say, if the return value was used in DCHECKs or ifs,
> would it evaluate to the same thing?

The return value couldn't be used in DCHECKs as we need to be able to handle
failures here. It will happen in situations where we've lost the context.

In the past we had to conditionally skip raster, upload, etc in context lost
situations. By handling this here and still returning a valid canvas the
difference in code paths for context lost situations is reduced and that's in my
experience very valuable as significantly different code paths for lost context
handling often end up with bugs as they are never as well tested as the normal
code paths.

https://codereview.chromium.org/562833004/diff/60001/cc/resources/image_copy_...
File cc/resources/image_copy_raster_worker_pool.cc (right):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:30: : worker_pool_(worker_pool),
On 2014/09/16 22:15:55, vmpstr wrote:
> client? Up to you.

This is different in latest patch. PTAL.

https://codereview.chromium.org/562833004/diff/60001/cc/resources/image_copy_...
cc/resources/image_copy_raster_worker_pool.cc:319: for (size_t i = 0; i < count;
++i) {
On 2014/09/16 22:15:55, vmpstr wrote:
> maybe:
> 
> while (count-- && !pending_resource_copy_operations_.empty()) {
>   ...
> }

I prefer to pick one variable that is used as the conditional of loops when
possible and instead break out of the loop if additional conditions   need to be
satisfied. I find this a bit easier to read but that's probably just a personal
preference. It does allow for comments to be added in a clean way to explain
each condition that force us to break out of the loop.

https://codereview.chromium.org/562833004/diff/60001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:721:
resource_provider_->UnmapPixelBuffer(raster_task->resource()->id());
On 2014/09/16 22:15:55, vmpstr wrote:
> Can you explain why this is now needed?

It was done automatically by ResourceProvider::BeginSetPixels before. That's no
longer needed and I think it's much cleaner to do it explicitly from here if
possible.

https://codereview.chromium.org/562833004/diff/60001/cc/resources/raster_buff...
File cc/resources/raster_buffer.cc (left):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/raster_buff...
cc/resources/raster_buffer.cc:7: namespace media {
On 2014/09/16 22:15:55, vmpstr wrote:
> O_O
> 
> I'm guessing git decided that you copied this :)

I could have. It would be very efficient of me :)

https://codereview.chromium.org/562833004/diff/60001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/562833004/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:125: DCHECK(canvas);
On 2014/09/16 22:15:55, vmpstr wrote:
> Oh I see the change now from NULL ptr to NullCanvas. Is it possible to do this
> as a separate change either before or after moving the code?

Good idea. Making this change using https://codereview.chromium.org/578843002/
prior to this patch.

[danakj, 2014-09-17 14:47:20]

On Wed, Sep 17, 2014 at 10:40 AM, <reveman@chromium.org> wrote:

> PTAL
>
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/gpu_raster_worker_pool.cc
> File cc/resources/gpu_raster_worker_pool.cc (right):
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/gpu_raster_worker_pool.cc#newcode21
> cc/resources/gpu_raster_worker_pool.cc:21: class RasterBufferImpl :
> public RasterBuffer {
> On 2014/09/16 22:15:55, vmpstr wrote:
>
>> <nit>
>> Can we come up with a name that isn't using Impl somehow. I know this
>>
> is a more
>
>> proper way of naming this type of class, but given that we have the
>>
> whole
>
>> impl-thread thing, I think this makes it somewhat confusing. Maybe
>> ScopedRasterBuffer, since it locks/unlocks things in the ctor/dtor (or
>> LockedRasterBuffer, RasterBufferAutoLock)?
>>
>
> RasterBufferImpl is a more compact way of writing
> GpuRasterWorkerPoolRasterBuffer in this case. We could just name it
> that, although I find it a bit annoying that we can't use the "Impl"
> suffix in a way that's consistent with the rest of the chromium code.
>
>
>  I think we should also file a bug and/or decide whether we allow
>>
> FooImpl in cc
>
>> as an implementation class of Foo, or whether things like
>>
> PictureLayerImpl have
>
>> to get a better name
>> </nit>
>>
>
> I don't think PictureLayerImpl is too bad. Yes, there's no explicit
> PictureLayer interface but it's in some sense implementing a "picture
> layer".
>
> We could instead use PictureLayerHost on the main thread and
> PictureLayer on the compositor thread. That would probably be a bit more
> consistent with the rest of chromium but LayerTreeHostImpl would become
> a bit confusing. Not sure what's the best solution is. I think we should
> wait until all non impl-side painting code has been removed before we
> make any changes like this though.


+1 at some point after that, we stop having 2 layer trees altogether, so
this gets much easier to solve. But I agree with Vlad that "Impl" means
something specific about which thread things run on inside cc/ and we
should be consistent in that.


>
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/gpu_raster_worker_pool.cc#newcode36
> cc/resources/gpu_raster_worker_pool.cc:36: :
> skia::AdoptRef(SkCreateNullCanvas());
> On 2014/09/16 22:15:55, vmpstr wrote:
>
>> The previous code did "return skia::RefPtr<SkCanvas>();" type of
>>
> thing. Is this
>
>> equivalent? That is to say, if the return value was used in DCHECKs or
>>
> ifs,
>
>> would it evaluate to the same thing?
>>
>
> The return value couldn't be used in DCHECKs as we need to be able to
> handle failures here. It will happen in situations where we've lost the
> context.
>
> In the past we had to conditionally skip raster, upload, etc in context
> lost situations. By handling this here and still returning a valid
> canvas the difference in code paths for context lost situations is
> reduced and that's in my experience very valuable as significantly
> different code paths for lost context handling often end up with bugs as
> they are never as well tested as the normal code paths.
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/image_copy_raster_worker_pool.cc
> File cc/resources/image_copy_raster_worker_pool.cc (right):
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/image_copy_raster_worker_pool.cc#newcode30
> cc/resources/image_copy_raster_worker_pool.cc:30: :
> worker_pool_(worker_pool),
> On 2014/09/16 22:15:55, vmpstr wrote:
>
>> client? Up to you.
>>
>
> This is different in latest patch. PTAL.
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/image_copy_raster_worker_pool.cc#newcode319
> cc/resources/image_copy_raster_worker_pool.cc:319: for (size_t i = 0; i
> < count; ++i) {
> On 2014/09/16 22:15:55, vmpstr wrote:
>
>> maybe:
>>
>
>  while (count-- && !pending_resource_copy_operations_.empty()) {
>>    ...
>> }
>>
>
> I prefer to pick one variable that is used as the conditional of loops
> when possible and instead break out of the loop if additional conditions
>   need to be satisfied. I find this a bit easier to read but that's
> probably just a personal preference. It does allow for comments to be
> added in a clean way to explain each condition that force us to break
> out of the loop.
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/pixel_buffer_raster_worker_pool.cc
> File cc/resources/pixel_buffer_raster_worker_pool.cc (right):
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/pixel_buffer_raster_worker_pool.cc#newcode721
> cc/resources/pixel_buffer_raster_worker_pool.cc:721:
> resource_provider_->UnmapPixelBuffer(raster_task->resource()->id());
> On 2014/09/16 22:15:55, vmpstr wrote:
>
>> Can you explain why this is now needed?
>>
>
> It was done automatically by ResourceProvider::BeginSetPixels before.
> That's no longer needed and I think it's much cleaner to do it
> explicitly from here if possible.
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/raster_buffer.cc
> File cc/resources/raster_buffer.cc (left):
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/raster_buffer.cc#oldcode7
> cc/resources/raster_buffer.cc:7: namespace media {
> On 2014/09/16 22:15:55, vmpstr wrote:
>
>> O_O
>>
>
>  I'm guessing git decided that you copied this :)
>>
>
> I could have. It would be very efficient of me :)
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/tile_manager.cc
> File cc/resources/tile_manager.cc (right):
>
> https://codereview.chromium.org/562833004/diff/60001/cc/
> resources/tile_manager.cc#newcode125
> cc/resources/tile_manager.cc:125: DCHECK(canvas);
> On 2014/09/16 22:15:55, vmpstr wrote:
>
>> Oh I see the change now from NULL ptr to NullCanvas. Is it possible to
>>
> do this
>
>> as a separate change either before or after moving the code?
>>
>
> Good idea. Making this change using
> https://codereview.chromium.org/578843002/ prior to this patch.
>
> https://codereview.chromium.org/562833004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2014-09-17 15:53:02]

So have we decided to change RasterBufferImpl to something else? As it stands I
think it's misleading to someone familiar with impl-side code but looking at
this code for the first time.

Other than that and the comments below this change LG.

https://codereview.chromium.org/562833004/diff/100001/cc/resources/image_copy...
File cc/resources/image_copy_raster_worker_pool.cc (left):

https://codereview.chromium.org/562833004/diff/100001/cc/resources/image_copy...
cc/resources/image_copy_raster_worker_pool.cc:236:
state->SetInteger("pending_count", raster_task_states_.size());
We still have a boolean that indicates that we have tasks pending; maybe change
that to a bool instead? I find that the more we know in a trace the easier it is
to debug things

https://codereview.chromium.org/562833004/diff/100001/cc/resources/image_rast...
File cc/resources/image_raster_worker_pool.cc (left):

https://codereview.chromium.org/562833004/diff/100001/cc/resources/image_rast...
cc/resources/image_raster_worker_pool.cc:149:
resource_provider_->EnableReadLockFences(task->resource()->id());
This changed from being the last thing we do to being the first thing we do. I
think that's a good change, but consider making it separately as well (It's
easier to look at this change when it's mostly just moving code around instead
of reordering)

[reveman, 2014-09-17 16:22:54]

I think we should allow the use of Impl suffix in cc/. At least when used in
anonymous namespace where I think it's hard to be confused with the idea of an
"impl"-thread.

Looks like there are already a number of cases where we use this:
RasterTaskImpl, SyncPointClientImpl, RendererCapabilitiesImpl, ScrollbarImpl,
etc.

https://codereview.chromium.org/562833004/diff/100001/cc/resources/image_copy...
File cc/resources/image_copy_raster_worker_pool.cc (left):

https://codereview.chromium.org/562833004/diff/100001/cc/resources/image_copy...
cc/resources/image_copy_raster_worker_pool.cc:236:
state->SetInteger("pending_count", raster_task_states_.size());
On 2014/09/17 15:53:02, vmpstr wrote:
> We still have a boolean that indicates that we have tasks pending; maybe
change
> that to a bool instead? I find that the more we know in a trace the easier it
is
> to debug things

Done.

https://codereview.chromium.org/562833004/diff/100001/cc/resources/image_rast...
File cc/resources/image_raster_worker_pool.cc (left):

https://codereview.chromium.org/562833004/diff/100001/cc/resources/image_rast...
cc/resources/image_raster_worker_pool.cc:149:
resource_provider_->EnableReadLockFences(task->resource()->id());
On 2014/09/17 15:53:02, vmpstr wrote:
> This changed from being the last thing we do to being the first thing we do. I
> think that's a good change, but consider making it separately as well (It's
> easier to look at this change when it's mostly just moving code around instead
> of reordering)

Ok, keeping it as the last thing we do for now.

[vmpstr, 2014-09-17 16:43:09]

lgtm. I don't think we should block this review on the name of RasterBufferImpl.
I'll file a bug where we can discuss this and hopefully come up with an
agreement.

[reveman, 2014-09-17 19:08:04]

On 2014/09/17 16:43:09, vmpstr wrote:
> lgtm. I don't think we should block this review on the name of
RasterBufferImpl.
> I'll file a bug where we can discuss this and hopefully come up with an
> agreement.

sgtm, thanks!

[reveman, 2014-09-17 19:08:25]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-09-17 19:08:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562833004/120001

[commit-bot: I haz the power, 2014-09-17 19:53:33]

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

[commit-bot: I haz the power, 2014-09-17 19:53:34]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[reveman, 2014-09-18 18:06:41]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-09-18 18:07:24]

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

[commit-bot: I haz the power, 2014-09-18 19:39:30]

Committed patchset #8 (id:140001) as 6e421b0dce93e7e9d60ed0c46ab45bfac0e2308b

[commit-bot: I haz the power, 2014-09-18 19:40:09]

Patchset 8 (id:??) landed as
https://crrev.com/47560aba9ae6e235795068d6e78dcf4ea0628e63
Cr-Commit-Position: refs/heads/master@{#295528}