Issue 1832573004: Gpu Image Decode Controller

ericrk

Description was changed from ========== WIP: Gpu Image Decode Controller ========== to ========== WIP: Gpu ...

4 years, 9 months ago (2016-03-24 18:10:47 UTC) #1

ericrk

Description was changed from ========== WIP: Gpu Image Decode Controller CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== WIP: ...

4 years, 9 months ago (2016-03-24 22:27:08 UTC) #2

ericrk

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

4 years, 9 months ago (2016-03-25 00:09:26 UTC) #3

ericrk

Here's my current WIP image decode patch - still doing some work on it, so ...

4 years, 9 months ago (2016-03-25 00:09:27 UTC) #4

ericrk

Description was changed from ========== WIP: Gpu Image Decode Controller CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Adds ...

4 years, 9 months ago (2016-03-25 20:49:49 UTC) #5

ericrk

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

4 years, 9 months ago (2016-03-25 21:19:58 UTC) #6

ericrk

I think this is in good shape and ready for review :D - Thanks!

4 years, 9 months ago (2016-03-25 21:19:58 UTC) #7

vmpstr

https://codereview.chromium.org/1832573004/diff/120001/cc/raster/tile_task_runner.cc File cc/raster/tile_task_runner.cc (right): https://codereview.chromium.org/1832573004/diff/120001/cc/raster/tile_task_runner.cc#newcode50 cc/raster/tile_task_runner.cc:50: : dependency_(dependency) {} std::move(dependency) https://codereview.chromium.org/1832573004/diff/120001/cc/raster/tile_task_runner.h File cc/raster/tile_task_runner.h (right): https://codereview.chromium.org/1832573004/diff/120001/cc/raster/tile_task_runner.h#newcode68 ...

4 years, 9 months ago (2016-03-25 22:37:31 UTC) #8

ericrk

Thanks for the great feedback! Let me know how this looks now. https://codereview.chromium.org/1832573004/diff/120001/cc/raster/tile_task_runner.cc File cc/raster/tile_task_runner.cc ...

4 years, 8 months ago (2016-03-28 22:10:53 UTC) #10

vmpstr

https://codereview.chromium.org/1832573004/diff/200001/cc/raster/tile_task_runner.h File cc/raster/tile_task_runner.h (right): https://codereview.chromium.org/1832573004/diff/200001/cc/raster/tile_task_runner.h#newcode61 cc/raster/tile_task_runner.h:61: // Whether this ImageDecodeTask can be run at the ...

4 years, 8 months ago (2016-03-28 23:55:53 UTC) #11

vmiura

How do we deal with releasing uploaded GPU images when: a) Shutting down the TileManager? ...

4 years, 8 months ago (2016-03-29 01:33:17 UTC) #12

ericrk

Cleaned this up quite a bit: 1) We now only modify our bytes_used_ in two ...

4 years, 8 months ago (2016-03-29 23:11:30 UTC) #17

Cleaned this up quite a bit:
1) We now only modify our bytes_used_ in two places: when we add refs, and when
we free to make room for a new item.
2) We now clean up on visibility changes / destruction.

https://codereview.chromium.org/1832573004/diff/200001/cc/raster/tile_task_ru...
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1832573004/diff/200001/cc/raster/tile_task_ru...
cc/raster/tile_task_runner.h:61: // Whether this ImageDecodeTask can be run at
the same time as other tasks in
On 2016/03/28 23:55:52, vmpstr wrote:
> Maybe "Indicates whether ..."?

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/resources/resource_...
File cc/resources/resource_format.cc (right):

https://codereview.chromium.org/1832573004/diff/200001/cc/resources/resource_...
cc/resources/resource_format.cc:32: SkColorType
ResourceFormatToClosestSkColorType(ResourceFormat format) {
On 2016/03/28 23:55:52, vmpstr wrote:
> The only difference between this function and the one above is an extra
> NOTREACHED(). Do you think the NOTREACHED is important in those cases? I'm
kind
> of hoping that we can just keep one of the functions. 

I think we can just drop the above one and use this version only - the above is
used in exactly one place,  so seems safe.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller.cc (right):

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:37: if
(draw_image.matrix_is_decomposable() &&
On 2016/03/28 23:55:52, vmpstr wrote:
> If matrix is not decomposable, then the scale is set to (1.f, 1.f) so I don't
> think you need this particular term. 

ah, good point.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:41:
std::numeric_limits<float>::epsilon()))
On 2016/03/28 23:55:53, vmpstr wrote:
> Can you add braces for this if-statement please?

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:202: // We had an existing upload task,
ref to the image and return the task.
On 2016/03/28 23:55:53, vmpstr wrote:
> either "add ref to the image" or "ref the image"

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:235: bytes_used_ += data->size;
On 2016/03/28 23:55:53, vmpstr wrote:
> There's some extremely rare but possible overflow here. I'm not sure if we
> should care or not, but if we should then we have a bunch of checked math
> classes in base. 

Updated CanFitSize to use checked math. Now that at-raster is not added to
bytes_used_, we gate all additions by CanFitSize, so we shouldn't have any
issues.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:238: // Ref image and create an upload
task. We will release this ref in
On 2016/03/28 23:55:52, vmpstr wrote:
> nit: create both upload and decode tasks.

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:246: RefImageWithClassLock(draw_image);
On 2016/03/28 23:55:52, vmpstr wrote:
> There's two Refs here (lines 240 and 246). Is that intentional? If so, why? 
> 
> Is this for the case that raster is cancelled but the decode task is already
> running? Maybe we should do something like this for software as well. In
> software, the decode function double checks the ref. 
> 
> Either that or gpu should do something like software does. Wdyt?

This was intentional - one ref is kept alive by this class, for the lifetime of
the decode/upload task. The other is the ref that's "returned" to the caller...
Adding a comment.

Keeping a ref alive simplifies the logic in a few places - I could see making SW
behave like this, although I think it's strictly worse, just a little easier to
reason about. Not sure if we want to go down that route.

If you'd like to not take these extra refs in GPU, I could see doing that as
well.. but I'd like to do it in a follow-up, as it adds a number of additional
checks (and there's also some interactions between the decode/upload tasks that
make it a bit annoying).

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:264:
context_->GetLock()->AssertAcquired();
On 2016/03/28 23:55:52, vmpstr wrote:
> Since we have public functions that need context lock to be acquired already,
> I'm wondering if we should rethink the whole name it with lock in the name and
> just assert things at the top of the function if they lock is already required
> and document it via comments. 

I agree.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:270: // We didn't find the image, we are
doing at-raster decode.
On 2016/03/28 23:55:53, vmpstr wrote:
> Is it possible that the raster task started running, but the rest of the image
> raster tasks were cancelled, which allowed us to get under the memory limit? I
> don't mind if we still mark this as at-raster, but I think the DCHECK below
> might fail in those cases.

Going to just always assume at-raster here.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:273:
DCHECK(!EnsureCapacityWithClassLock(data->size));
On 2016/03/28 23:55:52, vmpstr wrote:
> I'm not sure if I asked you to add this, but since this function has a side
> effect of freeing memory I don't think we should put it in a DCHECK (since it
> would run in debug but not in release). Maybe just check !CanFitSize ||
> !CanFitCount

Removed this, it works fine either way. We can investigate adding this in later.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:277: bytes_used_ += data->size;
On 2016/03/28 23:55:53, vmpstr wrote:
> This is a little bit questionable for me. If this is at raster, then why is it
> counting against the memory limit? That is, if I get one really large
at-raster
> image, then during the next schedule all of the smaller tasks would be
at-raster
> as well as long as this one is still pending.
> 
> That seems non-ideal to me, especially if we create and ref tasks before
> scheduling and cancelling the previous work. This might lead to a continuous
at
> raster schedule or just some bad behavior. 
> 
> 
> Also, if it's currently being used and is at raster, then accounting for
memory
> prevents the conversion to not-at-raster in GetTaskForImageAndRef (since it
> would check if we can fit another |size| bytes work of memory). 

Ah, fair point... I think a lot of stuff gets easier if we make at-raster not
budgeted. Updating this.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:363: // the pending bytes.
On 2016/03/28 23:55:52, vmpstr wrote:
> Otherwise, I guess the bytes are still accounted for in the fact that there's
> now a new image in the cache?

Moved all memory tracking to RefCountChanged/EnsureCapacity

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:378: // Checks if an existing image
decode exists. If not, returns a task to produce
On 2016/03/28 23:55:53, vmpstr wrote:
> I guess normally it's not possible that we're trying to create this while we
> didn't have the upload task. However, the upload could have been cancelled and
> this one was already running?

We create this when we create the upload task - so I think there should be no
issues. We'd never just create this.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:523: return bytes_used_ + size <=
cached_bytes_limit_;
On 2016/03/28 23:55:52, vmpstr wrote:
> There's a possible overflow here which would erroneously return true for a
size
> that's exactly std::numeric_limit<size_t>::max()
> 
> I think the given size is computed by Skia, so I'm not sure if it can ever
> return something like that...

Added safe math. This function is used to gate all cache entry additions, so
safe math shouldn't be necessary other places.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.cc:585: if (!image_data->decode.data) {
On 2016/03/29 01:33:17, vmiura wrote:
> Can this be unconditional, since image_data->decode.data is reset above?

Not in this case, as we unlock during the actual decode, and another task could
have come in and modified this. Added a comment.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller.h (right):

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.h:8: #include <list>
On 2016/03/28 23:55:53, vmpstr wrote:
> I don't see list usage... Or am I missing something?

removed.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.h:31: // decoded/ uploaded images for later
re-use.
On 2016/03/28 23:55:53, vmpstr wrote:
> either "decoded / uploaded" or "decoded/uploaded" </ocd>

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.h:38: // lockmust happen on our
non-concurrent raster thread.
On 2016/03/28 23:55:53, vmpstr wrote:
> s/lockmust/lock and must/

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.h:45: // limits. Uploaded GPU memory, being
non- discardable, always counts against
On 2016/03/28 23:55:53, vmpstr wrote:
> s/non- discardable/non-discardable/ :P

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.h:127: // rather than the upload task.
On 2016/03/28 23:55:53, vmpstr wrote:
> Can you add a comment to the GetTaskForImageAndRef to indicate that it returns
> the upload task then?

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller.h:132: void
RefImageDecodeWithClassLock(const DrawImage& draw_image);
On 2016/03/28 23:55:53, vmpstr wrote:
> Are all private functions WithClassLock? :) I don't really mind the naming,
but
> I think you could also just make a general comment in the header saying "All
> private functions have to have the context lock acquired before calling" or
> something and just AssertAcquired in all of them.
> 
> That would only leave the ones that need a context lock, which would be named
> explicitly *WithContextLock.
> 
> Up to you though.

Done.

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
File cc/tiles/gpu_image_decode_controller_unittest.cc (right):

https://codereview.chromium.org/1832573004/diff/200001/cc/tiles/gpu_image_dec...
cc/tiles/gpu_image_decode_controller_unittest.cc:79:
CreateMatrix(SkSize::Make(0.5f, 0.5f), is_decomposable));
On 2016/03/28 23:55:53, vmpstr wrote:
> Can you change the scale here as well, since it should still result in the
same
> task?

sure.

ericrk

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

4 years, 8 months ago (2016-04-05 17:22:53 UTC) #19

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1832573004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1832573004/300001

4 years, 8 months ago (2016-04-05 17:23:22 UTC) #20

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-05 18:44:32 UTC) #21

commit-bot: I haz the power

Dry run: 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_android_rel_ng/builds/48613)

4 years, 8 months ago (2016-04-05 18:44:36 UTC) #22

ericrk

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the ...

4 years, 8 months ago (2016-04-05 23:27:26 UTC) #25

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-05 23:36:22 UTC) #28

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-06 01:33:13 UTC) #29

commit-bot: I haz the power

Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/84437)

4 years, 8 months ago (2016-04-06 01:33:14 UTC) #30

ericrk

Now that we skip YuV image decodes on Linux, had to re-base a number of ...

4 years, 8 months ago (2016-04-06 02:20:07 UTC) #31

ericrk

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

4 years, 8 months ago (2016-04-07 18:40:14 UTC) #33

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1832573004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1832573004/380001

4 years, 8 months ago (2016-04-07 18:40:56 UTC) #34

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-07 18:44:31 UTC) #35

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_gn/builds/15724) ios_dbg_simulator_ninja on ...

4 years, 8 months ago (2016-04-07 18:44:32 UTC) #36

ericrk

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

4 years, 8 months ago (2016-04-08 06:07:17 UTC) #38

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1832573004/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1832573004/420001

4 years, 8 months ago (2016-04-08 06:07:34 UTC) #39

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-08 06:11:27 UTC) #40

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_rel/builds/92444)

4 years, 8 months ago (2016-04-08 06:11:28 UTC) #41

ericrk

Description was changed from ========== Adds a GPU version of the ImageDecodeController to the compositor. ...

4 years, 8 months ago (2016-04-08 18:09:41 UTC) #43

ericrk

Switching to updating LayoutTests rather than manually uploading rebaselined images. The cases in question all ...

4 years, 8 months ago (2016-04-08 18:12:36 UTC) #44

vmpstr

lgtm https://codereview.chromium.org/1832573004/diff/480001/cc/tiles/gpu_image_decode_controller.cc File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/1832573004/diff/480001/cc/tiles/gpu_image_decode_controller.cc#newcode606 cc/tiles/gpu_image_decode_controller.cc:606: scoped_ptr<base::DiscardableMemory> backing_memory; Probably unique_ptr at this point. https://codereview.chromium.org/1832573004/diff/480001/cc/tiles/gpu_image_decode_controller.cc#newcode630 ...

4 years, 8 months ago (2016-04-08 18:24:59 UTC) #45

ericrk

https://codereview.chromium.org/1832573004/diff/480001/cc/tiles/gpu_image_decode_controller.cc File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/1832573004/diff/480001/cc/tiles/gpu_image_decode_controller.cc#newcode606 cc/tiles/gpu_image_decode_controller.cc:606: scoped_ptr<base::DiscardableMemory> backing_memory; On 2016/04/08 18:24:59, vmpstr wrote: > Probably ...

4 years, 8 months ago (2016-04-08 20:55:18 UTC) #46

ericrk

The CQ bit was checked by ericrk@chromium.org to run a CQ dry run

4 years, 8 months ago (2016-04-08 20:55:39 UTC) #47

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1832573004/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1832573004/500001

4 years, 8 months ago (2016-04-08 20:56:07 UTC) #48

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-08 22:13:02 UTC) #49

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 8 months ago (2016-04-08 22:13:04 UTC) #50

ericrk

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the ...

4 years, 8 months ago (2016-04-11 18:27:40 UTC) #52

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-11 18:28:31 UTC) #53

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-11 18:32:52 UTC) #54

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-11 18:32:53 UTC) #55

ericrk

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the ...

4 years, 8 months ago (2016-04-11 18:46:32 UTC) #57

commit-bot: I haz the power

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

4 years, 8 months ago (2016-04-11 18:46:59 UTC) #58

commit-bot: I haz the power

Description was changed from ========== Adds a GPU version of the ImageDecodeController to the compositor. ...

4 years, 8 months ago (2016-04-11 22:16:11 UTC) #59

commit-bot: I haz the power

Description was changed from ========== Adds a GPU version of the ImageDecodeController to the compositor. ...

4 years, 8 months ago (2016-04-11 22:17:56 UTC) #61

commit-bot: I haz the power

4 years, 8 months ago (2016-04-11 22:17:59 UTC) #62

Message was sent while issue was closed.

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

Issue 1832573004: Gpu Image Decode Controller (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : formatting and minor renames to improve readability #

Patch Set 6 : Add lock checks minor fixes #

Patch Set 7 : comments #

Patch Set 8 : feedback #

Patch Set 9 : unify skcolortype conversions #

Patch Set 10 : small fixes #

Patch Set 11 : feedback Visibility / Destructor Memory Cleanup #

Patch Set 12 : fix leak #

Patch Set 13 : YuV LayoutTests + Allow for image decode failures #

Patch Set 14 : rebase #

Patch Set 15 : rebaseline layout tests due to YuV rather than uploading images #

Patch Set 16 : rebase #

Patch Set 17 : scoped_ptr > std::unique_ptr #

Patch Set 18 : rebase #

Messages