Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1401053003:
change SkImage_Gpu to handle all filters (w/ and w/o gpu support (Closed)
Descriptionchange SkImage_Gpu to handle all filters (w/ and w/o gpu support
The result is that the set of "generic" imagefilters (e.g. SkColorFilterImageFilter) that use drawing commands to return their results will now stay in the same domain as their src (i.e. gpu-src --> gpu-dst).
ApplyFilterGM exercises this, and now asserts this same-domain invariant.
BUG=skia:4467
Committed: https://skia.googlesource.com/skia/+/59dc0d22f557a20669126fa425baefe6dd4b727a
Patch Set 1 #Patch Set 2 : check if we're still raster, and if so fall back to drawing version #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 9
Patch Set 5 : #
Messages
Total messages: 28 (10 generated)
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/patch-status/1401053003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401053003/1
reed@google.com changed reviewers: + robertphillips@google.com, senorblanco@google.com
just calling filterImage() requires a proxy that itself can implement filtering.
In the case of a classic proxy, one backed by an existing SkDevice, this is
deferred to SkGpuDevice::filterImage.
That code effectively just does the following:
if (filter->canUseGPU) {
filter->filterImageGPU(...)
} else ...
Just rolled that logic into SkImage_Gpu::onApplyFilter(), rather than allocating
a SkGpuDevice just for the purpose of that logic. As we desperately want to wean
imagefilters off of SkDevices, I also didn't want to add yet another user of
that mechanism.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
(Don't get me wrong -- I love the idea of being able to get rid of the call to drawSprite(), and even perform better.) https://codereview.chromium.org/1401053003/diff/20001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/1401053003/diff/20001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:231: bool forceResultToOriginalSize) const { As discussed, please change this bool to a nullable SkIRect param, so I don't have to change it later when implementing filter effects region. https://codereview.chromium.org/1401053003/diff/20001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:244: if (!filter->filterImageGPU(&proxy, src, ctx, &dst, offsetResult)) { This is going to bypass the caching logic in filterImage(), causing repeated re-processing of nodes in the graph with valence > 1. My preference would be that we create an old-school Proxy for now, and use the old code until we can switch all of it over to SkImage/SkSurface. You should still get GPU processing via filterImage(), and in fact you could then refactor a lot of this code with the CPU implementation, since it'll be much the same calls. If you really want to avoid creating a real device, we could try to put the filteImageGPU() logic in SkGpuImageFilterProxy::filterImage() instead. Then at least you'd get caching.
Current results of using this with the gl_renderer CL (which calls
applyFilter)....
3 tests failed:
ImageBackgroundFilter.BackgroundFilterRotated_GL
(../../cc/trees/layer_tree_host_pixeltest_filters.cc:405)
LayerTreeHostFiltersPixelTest.BackgroundFilterBlurOutsets
(../../cc/trees/layer_tree_host_pixeltest_filters.cc:57)
LayerTreeHostFiltersScaledPixelTest.StandardDpi_GL
(../../cc/trees/layer_tree_host_pixeltest_filters.cc:210)
Perhaps the failures (which I don't understand yet) are related to your
suggestion, so I will try it and see if these improve.
BTW -- the failure *seems* to be triggered just by a BlurImageFilter. I was
trying to inspect it, looking for something funny...
- no cropRect set
- 1 input (interesting...)
- getInput(0) == NULL (huh?)
1. Are there other aspects I should inspect to see why this Blur is behaving
poorly (only in the GL style of the test, not the SW version).
2. Its is at all suspect/interesting that it reports 1 input, but that input is
NULL?
On 2015/10/15 21:31:02, reed1 wrote:
> Current results of using this with the gl_renderer CL (which calls
> applyFilter)....
>
> 3 tests failed:
> ImageBackgroundFilter.BackgroundFilterRotated_GL
> (../../cc/trees/layer_tree_host_pixeltest_filters.cc:405)
> LayerTreeHostFiltersPixelTest.BackgroundFilterBlurOutsets
> (../../cc/trees/layer_tree_host_pixeltest_filters.cc:57)
> LayerTreeHostFiltersScaledPixelTest.StandardDpi_GL
> (../../cc/trees/layer_tree_host_pixeltest_filters.cc:210)
>
> Perhaps the failures (which I don't understand yet) are related to your
> suggestion, so I will try it and see if these improve.
I'm guessing you're ending up with CPU-rendered filters, which have
different pixels than the GPu expectations Just a guess, though.
I'd suggest also running the Blink layout tests, css3/filters in particular.
Then
at least you'll see some images.
> BTW -- the failure *seems* to be triggered just by a BlurImageFilter. I was
> trying to inspect it, looking for something funny...
> - no cropRect set
> - 1 input (interesting...)
> - getInput(0) == NULL (huh?)
>
> 1. Are there other aspects I should inspect to see why this Blur is behaving
> poorly (only in the GL style of the test, not the SW version).
>
> 2. Its is at all suspect/interesting that it reports 1 input, but that input
is
> NULL?
Nope, that's perfectly normal. A NULL input means "use the input primitive here"
(the thing referred to as "src" in filterImage()).
You'll notice every filter's onFilterImage() starts by setting all its input
bitmaps to "source" (the input primitive), then only overwriting
them if there's a non-NULL input:
bool SkBlurImageFilter::onFilterImage(Proxy* proxy,
const SkBitmap& source, const Context&
ctx,
SkBitmap* dst, SkIPoint* offset) const {
SkBitmap src = source;
...
if (!this->filterInput(0, proxy, source, ctx, &src, &srcOffset)) {
If input 0 is NULL, it'll leave src pointing at "source", which was the bitmap
passed to filterImage().
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/patch-status/1401053003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401053003/60001
moved gpu stuff into proxy, per stephen's suggestion. ptal
The CQ bit was unchecked by reed@google.com
https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:220: filter->filterImageGPU(this, src, ctx, dst, offset); This part looks good, I think. https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:232: bool forceResultToOriginalSize) const { I'd really appreciate if this could be turned into an optional SkIRect, and we pass NULL for now (meaning force to original size). Even if the non-NULL path doesn't work for now, it'll save me having to do a 3-sided patch later. https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:253: SkDebugf("------ got raster result from gpu-src using filter %s\n", This shouldn't happen. Is it? https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:265: surface->getCanvas()->drawImage(this, SkIntToScalar(-dstR.x()), SkIntToScalar(-dstR.y()), It looks like if we fall-through here (when fForceToOriginalSize is true, but we get a raster result), we'll filter it again and draw it again?
https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:232: bool forceResultToOriginalSize) const { On 2015/10/16 20:14:05, Stephen White wrote: > I'd really appreciate if this could be turned into an optional SkIRect, and we > pass NULL for now (meaning force to original size). Even if the non-NULL path > doesn't work for now, it'll save me having to do a 3-sided patch later. Adding a variant of the public API that takes a rect is just 1 CL in skia (making the intersting change), and 1 CL in chrome to pass in different information. (the public API is not virtual, and there are no subclass in chrome/blink, so this is not a multi-stage change I think). Until we have a tested impl somewhere, I would rather not extend our public API speculatively. https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:253: SkDebugf("------ got raster result from gpu-src using filter %s\n", On 2015/10/16 20:14:05, Stephen White wrote: > This shouldn't happen. Is it? Done. https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:265: surface->getCanvas()->drawImage(this, SkIntToScalar(-dstR.x()), SkIntToScalar(-dstR.y()), On 2015/10/16 20:14:05, Stephen White wrote: > It looks like if we fall-through here (when fForceToOriginalSize is true, but we > get a raster result), we'll filter it again and draw it again? Ah, good point. However, I think your previous point is correct that we shouldn't need the fall-through, so I'll clean up both parts.
https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:232: bool forceResultToOriginalSize) const { BTW, if we also added an SkMatrix param here, and just passed that instead of SkMatrix::I() below, I don't think we would need SkLocalMatrixImageFilter at all. Just sayin'. https://codereview.chromium.org/1401053003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:232: bool forceResultToOriginalSize) const { On 2015/10/16 20:41:52, reed1 wrote: > On 2015/10/16 20:14:05, Stephen White wrote: > > I'd really appreciate if this could be turned into an optional SkIRect, and we > > pass NULL for now (meaning force to original size). Even if the non-NULL path > > doesn't work for now, it'll save me having to do a 3-sided patch later. > > Adding a variant of the public API that takes a rect is just 1 CL in skia > (making the intersting change), and 1 CL in chrome to pass in different > information. (the public API is not virtual, and there are no subclass in > chrome/blink, so this is not a multi-stage change I think). Until we have a > tested impl somewhere, I would rather not extend our public API speculatively. It just seems like it would be very easy: SkImage_Gpu::onApplyFilter(SkImageFilter* filter, SkIPoint* offsetResult, SkIRect* destBounds = NULL) const { ... SkIRect srcBounds = destBounds ? *destBounds : SkIRect::MakeWH(this->width(), this->height)); ...
You are suggesting a change to the API and featureset of this api. I think that is orthogonal to this CL, which is trying to better implement the current one. If you want to change the feature, lets write up something that describes it, and we can iterate on that separately. I have some questions about how this new rect should interact with the current semantics of filters (e.g. that filters never return something larger than their input), so I'd like to discuss the two together.
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/patch-status/1401053003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401053003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from robertphillips@google.com Link to the patchset: https://codereview.chromium.org/1401053003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1401053003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1401053003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/59dc0d22f557a20669126fa425baefe6dd4b727a |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
