Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2216403002:
use SkMakeBitmapShader to not force copies when possible (Closed)
Descriptionuse SkMakeBitmapShader to not force copies when possible
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2216403002
Committed: https://skia.googlesource.com/skia/+/f77c47b78217883e6f074dd7e3e5bed5f82e144d
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix ref-count manually for allocator #Messages
Total messages: 24 (9 generated)
Description was changed from ========== use SkMakeBitmapShader to not force copies when possible BUG=skia: ========== to ========== use SkMakeBitmapShader to not force copies when possible BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2216403002 ==========
reed@google.com changed reviewers: + fmalita@chromium.org
LGTM Do you suspect this is the root cause for the recent regressions?
On 2016/08/05 14:34:32, f(malita) wrote: > LGTM > > Do you suspect this is the root cause for the recent regressions? No idea, but it is a case where a mutable bitmap would have been copied (unnecessarily in this case, since the shader is stack-frame scoped). Still need to run their test to see if this sort of thing is related.
Actually, perhaps we shouldn't "fix" this at the call-site, since we (now) claim that we can be efficient (i.e. not-copy) assuming we're going to a raster backend.
On 2016/08/05 15:42:54, reed1 wrote: > Actually, perhaps we shouldn't "fix" this at the call-site, since we (now) claim > that we can be efficient (i.e. not-copy) assuming we're going to a raster > backend. Doh, that comment intended for a different CL, please ignore
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/2216403002/diff/1/src/core/SkBitmapDevice.cpp File src/core/SkBitmapDevice.cpp (right): https://codereview.chromium.org/2216403002/diff/1/src/core/SkBitmapDevice.cpp... src/core/SkBitmapDevice.cpp:329: SkTBlitterAllocator allocator; Perhaps as a follow up, I think this signal wants to become more explicit, e.g. kWillNotOutliveStackframe. It was already kind of cute to base this decision off the presence of an allocator, but this allocator is the extreme, entirely unused except as a bool.
https://codereview.chromium.org/2216403002/diff/1/src/core/SkBitmapDevice.cpp File src/core/SkBitmapDevice.cpp (right): https://codereview.chromium.org/2216403002/diff/1/src/core/SkBitmapDevice.cpp... src/core/SkBitmapDevice.cpp:329: SkTBlitterAllocator allocator; On 2016/08/05 15:46:43, mtklein wrote: > Perhaps as a follow up, I think this signal wants to become more explicit, e.g. > kWillNotOutliveStackframe. It was already kind of cute to base this decision > off the presence of an allocator, but this allocator is the extreme, entirely > unused except as a bool. We already have ForceCopyMode for SkImageShader::Make - maybe plumb that on SkMakeBitmapShader and use kNever_ForceCopyMode when we know the shader has limited scope?
https://codereview.chromium.org/2216403002/diff/1/src/core/SkBitmapDevice.cpp File src/core/SkBitmapDevice.cpp (right): https://codereview.chromium.org/2216403002/diff/1/src/core/SkBitmapDevice.cpp... src/core/SkBitmapDevice.cpp:329: SkTBlitterAllocator allocator; On 2016/08/05 15:46:43, mtklein wrote: > Perhaps as a follow up, I think this signal wants to become more explicit, e.g. > kWillNotOutliveStackframe. It was already kind of cute to base this decision > off the presence of an allocator, but this allocator is the extreme, entirely > unused except as a bool. It is actually used to avoid mallocs when we alloc the actual shader.
The CQ bit was checked by reed@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I will try adding the enum as an explicit parameter for clarity in the next upload. I think I'll want to assert that in the presence of an allocator, that the caller knew to ask for kNever...
actually, that change (using enum more, renaming it to be slightly clearer) is getting large, and orthogonal to this CL. I suggest I land it separately. ok?
On 2016/08/05 17:05:04, reed1 wrote: > actually, that change (using enum more, renaming it to be slightly clearer) is > getting large, and orthogonal to this CL. I suggest I land it separately. ok? ok by me
On 2016/08/05 17:05:04, reed1 wrote: > actually, that change (using enum more, renaming it to be slightly clearer) is > getting large, and orthogonal to this CL. I suggest I land it separately. ok? sgtm
The CQ bit was unchecked by reed@google.com
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/2216403002/#ps20001 (title: "fix ref-count manually for allocator")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== use SkMakeBitmapShader to not force copies when possible BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2216403002 ========== to ========== use SkMakeBitmapShader to not force copies when possible BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2216403002 Committed: https://skia.googlesource.com/skia/+/f77c47b78217883e6f074dd7e3e5bed5f82e144d ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/f77c47b78217883e6f074dd7e3e5bed5f82e144d |
