Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1785643003:
Switch SkBlurImageFilter over to new onFilterImage interface (Closed)
|
Created:
4 years, 9 months ago by robertphillips Modified:
4 years, 9 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSwitch SkBlurImageFilter over to new onFilterImage interface
This CL relies on:
https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap)
https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps)
https://codereview.chromium.org/1813813002/ (Add SkSpecialImage::makeTextureImage entry point)
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1785643003
Committed: https://skia.googlesource.com/skia/+/3c935bc87020bfd19a08922f7394db3a801d168b
Committed: https://skia.googlesource.com/skia/+/1579e3c376a9ee8c694a64f9c87457cea13ba442
Patch Set 1 #Patch Set 2 : Update #Patch Set 3 : update #Patch Set 4 : Clean up #Patch Set 5 : update to ToT #Patch Set 6 : update to ToT (again) #
Total comments: 2
Patch Set 7 : update to ToT #Patch Set 8 : force gpu path in drawBitmapAsSprite for SkGpuDevice #Patch Set 9 : Fix indent #
Total comments: 14
Patch Set 10 : Clean up #Patch Set 11 : Use SkBitmap #
Total comments: 7
Patch Set 12 : Clean up #
Total comments: 4
Patch Set 13 : Switch to using tryAllocPixels #Patch Set 14 : Rename method (and update to ToT) #Patch Set 15 : add cast #Patch Set 16 : Fix discardable memory bot #Patch Set 17 : update to ToT #
Messages
Total messages: 58 (21 generated)
Description was changed from ========== Switch SkBlurImageFilter over to new onFilterImage interface ========== to ========== Switch SkBlurImageFilter over to new onFilterImage interface GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Switch SkBlurImageFilter over to new onFilterImage interface GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (https://codereview.chromium.org/1787883002/) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
robertphillips@google.com changed reviewers: + bsalomon@google.com, senorblanco@chromium.org, senorblanco@google.com
Description was changed from ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (https://codereview.chromium.org/1787883002/) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) https://codereview.chromium.org/1813813002/ (Add SkSpecialImage::makeTextureImage entry point) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
It's great that you're tackling merging the two processing paths at the same time. I'm a little concerned that the SkGPUDevice callsites might not be entering this code at the right place, though: won't they still be calling filterImageGPUDeprecated? And in this case, losing GPU acceeleration? https://codereview.chromium.org/1785643003/diff/90001/src/effects/SkBlurImage... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/90001/src/effects/SkBlurImage... src/effects/SkBlurImageFilter.cpp:96: if (!input->peekTexture()) { Rather than indenting the subsequent ~90 lines, could we keep the GPU code in a separate function, and do if (input->peekTexture()) { filterImageGPUInternalOrWhatever(...); return; }
IIUC, when SkBlurImageFilter::canFilterImageGPU used to return true, SkGpuDevice::canHandleImageFilter would also return true and SkCanvas::internalDrawDevice and SkBaseDevice::drawBitmapAsSprite would call SkGpuDevice::drawDevice and SkGpuDevice::drawSprite, respectively. In the drawDevice case SkGpuDevice::filterTexture was called with the RenderTarget as the source. In the drawSprite case the source bitmap was manually uploaded to VRAM, if necessary, and then passed as the source to SkGpuDevice::filterTexture. --- In this patch SkGpuDevice::canHandleImageFilter will now return false for SkBlurImageFilters. SkCanvas::internalDrawDevice and SkBaseDevice::drawBitmapAsSprite will now call SkImageFilter::filterImage themselves. In the internalDrawDevice case the source device will be an SkGpuDevice so the source SkSpecialImage will be GrTexture-backed. So we will take the GPU-specific path in SkBlurImageFilter. In the drawBitmapAsSprite case the implementation in SkGpuDevice will, if necessary, upload the bitmap to VRAM and then invoke the SkBaseDevice implementation. Since the source bitmap is GrTexture-backed the GPU-specific path in SkBlurImageFilter will also be taken. https://codereview.chromium.org/1785643003/diff/90001/src/effects/SkBlurImage... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/90001/src/effects/SkBlurImage... src/effects/SkBlurImageFilter.cpp:96: if (!input->peekTexture()) { The parameter list to an internal method was cumbersome w/o duplicating the 'onFilterImage' preamble. Is the current solution palatable?
robertphillips@google.com changed reviewers: + reed@google.com
+reed for the virtualization of drawBitmapAsSprite
https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:98: if (0 == sigma.x() && 0 == sigma.y()) { Side note: we should probably consider optimizing the zero-sigma-with-croprect case into another filter in the Create() factory. Or at a minimum, combining the raster & GPU versions of this early-out. https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:126: tex).release(); Q: out of curiosity, is this release() necessary for correctness? https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:166: SkPMColor* t = (SkPMColor*) tmp.addr32(0, 0); Could we use writeable_addr() here to avoid a cast? https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:219: sk_free(addr); This feels really error-prone (requiring an sk_free() lambda). Is there a better way we could encapsulate the memory management here? Presumably we're going to need it in all the filters with pixel loops. https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:222: dst.release(); // 'result' now owns the pixels This also feels error-prone.
On 2016/03/21 16:44:32, robertphillips wrote:
> IIUC, when SkBlurImageFilter::canFilterImageGPU used to return true,
> SkGpuDevice::canHandleImageFilter would also return true and
> SkCanvas::internalDrawDevice and SkBaseDevice::drawBitmapAsSprite would call
> SkGpuDevice::drawDevice and SkGpuDevice::drawSprite, respectively.
>
> In the drawDevice case SkGpuDevice::filterTexture was called with the
> RenderTarget as the source.
>
> In the drawSprite case the source bitmap was manually uploaded to VRAM, if
> necessary, and then passed as the source to SkGpuDevice::filterTexture.
What if we have another old-school GPU-enabled filter upstream of a blur? It
looks like this will drop out of GPU acceleration.
E.g., SkMorphologyImageFilter -> SkBlurImageFilter
SkBlurImageFilter::onFilterImage() calls
SkImageFilter::filterInput(), which calls
SkImageFilter::onFilterImage() which calls
SkMorphologyImageFilter::filterImageDeprecated(), which is the raster
path.
SkImageFilter::filterInput() will upload the raster-rendered result to the
GPU, but this isn't what we want.
Am I wrong?
On 2016/03/21 18:26:31, Stephen White wrote:
> On 2016/03/21 16:44:32, robertphillips wrote:
> > IIUC, when SkBlurImageFilter::canFilterImageGPU used to return true,
> > SkGpuDevice::canHandleImageFilter would also return true and
> > SkCanvas::internalDrawDevice and SkBaseDevice::drawBitmapAsSprite would call
> > SkGpuDevice::drawDevice and SkGpuDevice::drawSprite, respectively.
> >
> > In the drawDevice case SkGpuDevice::filterTexture was called with the
> > RenderTarget as the source.
> >
> > In the drawSprite case the source bitmap was manually uploaded to VRAM, if
> > necessary, and then passed as the source to SkGpuDevice::filterTexture.
>
> What if we have another old-school GPU-enabled filter upstream of a blur? It
> looks like this will drop out of GPU acceleration.
>
> E.g., SkMorphologyImageFilter -> SkBlurImageFilter
>
> SkBlurImageFilter::onFilterImage() calls
> SkImageFilter::filterInput(), which calls
> SkImageFilter::onFilterImage() which calls
> SkMorphologyImageFilter::filterImageDeprecated(), which is the raster
> path.
>
> SkImageFilter::filterInput() will upload the raster-rendered result to the
> GPU, but this isn't what we want.
>
> Am I wrong?
Never mind; I'm wrong. This looks ok.
SkImageFilter::onFilterImage() calls
SkImageFilter::filterImageDeprecated() which checks the device proxy, and
switches over to GPU accel at that point.
(
SkImageFilter::DeviceProxy::filterImage() which calls
SkGpuDevice::filterImage() which calls
SkGpuDevice::filterTexture() which calls
SkDilateImageFilter::filterImageGPUDeprecated()
)
https://codereview.chromium.org/1785643003/diff/150001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/1785643003/diff/150001/src/gpu/SkGpuDevice.cp... src/gpu/SkGpuDevice.cpp:223: void SkGpuDevice::drawBitmapAsSprite(const SkDraw& draw, const SkBitmap& bitmap, // Want to ensure that we always have a texture-backed "bitmap" when we finally call through to the base impl, because ...
On 2016/03/21 18:42:13, Stephen White wrote: > On 2016/03/21 18:26:31, Stephen White wrote: > > On 2016/03/21 16:44:32, robertphillips wrote: > > > IIUC, when SkBlurImageFilter::canFilterImageGPU used to return true, > > > SkGpuDevice::canHandleImageFilter would also return true and > > > SkCanvas::internalDrawDevice and SkBaseDevice::drawBitmapAsSprite would call > > > SkGpuDevice::drawDevice and SkGpuDevice::drawSprite, respectively. > > > > > > In the drawDevice case SkGpuDevice::filterTexture was called with the > > > RenderTarget as the source. > > > > > > In the drawSprite case the source bitmap was manually uploaded to VRAM, if > > > necessary, and then passed as the source to SkGpuDevice::filterTexture. > > > > What if we have another old-school GPU-enabled filter upstream of a blur? It > > looks like this will drop out of GPU acceleration. > > > > E.g., SkMorphologyImageFilter -> SkBlurImageFilter > > > > SkBlurImageFilter::onFilterImage() calls > > SkImageFilter::filterInput(), which calls > > SkImageFilter::onFilterImage() which calls > > SkMorphologyImageFilter::filterImageDeprecated(), which is the raster > > path. > > > > SkImageFilter::filterInput() will upload the raster-rendered result to the > > GPU, but this isn't what we want. > > > > Am I wrong? > > Never mind; I'm wrong. This looks ok. > > SkImageFilter::onFilterImage() calls > SkImageFilter::filterImageDeprecated() which checks the device proxy, and > switches over to GPU accel at that point. > > ( > SkImageFilter::DeviceProxy::filterImage() which calls > SkGpuDevice::filterImage() which calls > SkGpuDevice::filterTexture() which calls > SkDilateImageFilter::filterImageGPUDeprecated() > ) Right - the device proxy ultimately kicks in.
https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:98: if (0 == sigma.x() && 0 == sigma.y()) { On 2016/03/21 18:11:28, Stephen White wrote: > Side note: we should probably consider optimizing the zero-sigma-with-croprect > case into another filter in the Create() factory. Or at a minimum, combining the > raster & GPU versions of this early-out. Acknowledged. I considered it for this CL but the raster path does a little bit of extra work. https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:126: tex).release(); On 2016/03/21 18:11:28, Stephen White wrote: > Q: out of curiosity, is this release() necessary for correctness? I think so. MakeFromGpu now returns an sk_sp which will unref its owned pointer in its destructor. https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:166: SkPMColor* t = (SkPMColor*) tmp.addr32(0, 0); On 2016/03/21 18:11:28, Stephen White wrote: > Could we use writeable_addr() here to avoid a cast? Done. https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:219: sk_free(addr); On 2016/03/21 18:11:28, Stephen White wrote: > This feels really error-prone (requiring an sk_free() lambda). Is there a better > way we could encapsulate the memory management here? Presumably we're going to > need it in all the filters with pixel loops. The MakeFromPixmap entry point is patterned on SkImage::MakeFromRaster which has the ReleaseProc & ReleaseContext pair. It appears that this is the approved way to deal with raster-specific pixel APIs. https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:222: dst.release(); // 'result' now owns the pixels On 2016/03/21 18:11:28, Stephen White wrote: > This also feels error-prone. The new SkPixmap-based APIs push the lifetime management onto the callers. We could store the dst pixels in something other than an SkAutoPixmapStorage but we would have to copy a bit of code. If it makes it any more palatable one can think of the SkAutoPixmapStorage as owning a ref on the pixel data which it is now releasing. https://codereview.chromium.org/1785643003/diff/150001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/1785643003/diff/150001/src/gpu/SkGpuDevice.cp... src/gpu/SkGpuDevice.cpp:223: void SkGpuDevice::drawBitmapAsSprite(const SkDraw& draw, const SkBitmap& bitmap, On 2016/03/21 20:41:35, reed1 wrote: > // Want to ensure that we always have a texture-backed "bitmap" when we finally > call through to the base impl, because ... Done.
https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:219: sk_free(addr); On 2016/03/21 21:32:40, robertphillips wrote: > On 2016/03/21 18:11:28, Stephen White wrote: > > This feels really error-prone (requiring an sk_free() lambda). Is there a > better > > way we could encapsulate the memory management here? Presumably we're going to > > need it in all the filters with pixel loops. > > The MakeFromPixmap entry point is patterned on SkImage::MakeFromRaster which has > the ReleaseProc & ReleaseContext pair. It appears that this is the approved way > to deal with raster-specific pixel APIs. Could we perhaps create an SkSpecialImage::MakeFromPixmap() that takes an SkAutoPixmapStorage? Then we could hide the sk_free() and the release() call in there. https://codereview.chromium.org/1785643003/diff/150001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:222: dst.release(); // 'result' now owns the pixels On 2016/03/21 21:32:40, robertphillips wrote: > On 2016/03/21 18:11:28, Stephen White wrote: > > This also feels error-prone. > > The new SkPixmap-based APIs push the lifetime management onto the callers. We > could store the dst pixels in something other than an SkAutoPixmapStorage but we > would have to copy a bit of code. It's more about copying these bits of code (the sk_free() lambda and the release()) into all raster image filters that I'm worried about.
PTAL - this now uses SkBitmap for the destination raster pixels
https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:217: dst).release(); Much better, thanks. I'm still a little concerned about the need for an explicit release(). Will this (hopefully) fail to compile if you forget it?
https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:217: dst).release(); Yep - you get: 'return': cannot convert from 'sk_sp<SkSpecialImage>' to 'SkSpecialImage *' Additionally, I believe Mike will be making a sweep through the image filters making them all return sk_sp's sometime soon.
https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:162: SkAutoPixmapStorage tmp; Could we keep the temp buffer as an SkBitmap as well, just for symmetry? It's a little confusing to see the mix of the two.
https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:180: * Nit: is this indentation intentional?
https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:217: dst).release(); On 2016/03/22 18:44:32, robertphillips wrote: > Yep - you get: > > 'return': cannot convert from 'sk_sp<SkSpecialImage>' to 'SkSpecialImage *' > > Additionally, I believe Mike will be making a sweep through the image filters > making them all return sk_sp's sometime soon. Awesome, tx.
https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:162: SkAutoPixmapStorage tmp; On 2016/03/22 18:44:46, Stephen White wrote: > Could we keep the temp buffer as an SkBitmap as well, just for symmetry? It's a > little confusing to see the mix of the two. Done. https://codereview.chromium.org/1785643003/diff/190001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:180: * On 2016/03/22 18:47:02, Stephen White wrote: > Nit: is this indentation intentional? Done.
https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (left): https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:118: SkAutoTUnref<SkBaseDevice> device(proxy->createDevice(dstBounds.width(), dstBounds.height())); BTW, there's actually a reason we were using createDevice() here. The idea was that it would provide a central pinch-point for creating backing stores that could come from discardable memory: https://codereview.chromium.org/1414843003 Anyway, that work hasn't been done yet; I just thought I'd mention it. We'll have to circle back and revisit this code when we do. Maybe we can request backing stores from the context's cache, rather than allocating them directly. https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:160: tmp.allocPixels(info); Shouldn't we be checking for allocation failure here?
https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (left): https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:118: SkAutoTUnref<SkBaseDevice> device(proxy->createDevice(dstBounds.width(), dstBounds.height())); On 2016/03/22 19:29:49, Stephen White wrote: > BTW, there's actually a reason we were using createDevice() here. The idea was > that it would provide a central pinch-point for creating backing stores that > could come from discardable memory: https://codereview.chromium.org/1414843003 > > Anyway, that work hasn't been done yet; I just thought I'd mention it. We'll > have to circle back and revisit this code when we do. Maybe we can request > backing stores from the context's cache, rather than allocating them directly. Acknowledged. https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... File src/effects/SkBlurImageFilter.cpp (right): https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... src/effects/SkBlurImageFilter.cpp:160: tmp.allocPixels(info); On 2016/03/22 19:29:49, Stephen White wrote: > Shouldn't we be checking for allocation failure here? It should throw on failure.
On 2016/03/22 19:40:50, robertphillips wrote: > https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... > File src/effects/SkBlurImageFilter.cpp (left): > > https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... > src/effects/SkBlurImageFilter.cpp:118: SkAutoTUnref<SkBaseDevice> > device(proxy->createDevice(dstBounds.width(), dstBounds.height())); > On 2016/03/22 19:29:49, Stephen White wrote: > > BTW, there's actually a reason we were using createDevice() here. The idea was > > that it would provide a central pinch-point for creating backing stores that > > could come from discardable memory: https://codereview.chromium.org/1414843003 > > > > Anyway, that work hasn't been done yet; I just thought I'd mention it. We'll > > have to circle back and revisit this code when we do. Maybe we can request > > backing stores from the context's cache, rather than allocating them directly. > > Acknowledged. > > https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... > File src/effects/SkBlurImageFilter.cpp (right): > > https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... > src/effects/SkBlurImageFilter.cpp:160: tmp.allocPixels(info); > On 2016/03/22 19:29:49, Stephen White wrote: > > Shouldn't we be checking for allocation failure here? > > It should throw on failure. Which in Chrome will provoke a renderer crash, no? We should probably use tryAllocPixels() and check the return value. Otherwise it might be fairly easy to construct an HTML page which will crash.
On 2016/03/22 19:53:50, Stephen White wrote: > On 2016/03/22 19:40:50, robertphillips wrote: > > > https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... > > File src/effects/SkBlurImageFilter.cpp (left): > > > > > https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... > > src/effects/SkBlurImageFilter.cpp:118: SkAutoTUnref<SkBaseDevice> > > device(proxy->createDevice(dstBounds.width(), dstBounds.height())); > > On 2016/03/22 19:29:49, Stephen White wrote: > > > BTW, there's actually a reason we were using createDevice() here. The idea > was > > > that it would provide a central pinch-point for creating backing stores that > > > could come from discardable memory: > https://codereview.chromium.org/1414843003 > > > > > > Anyway, that work hasn't been done yet; I just thought I'd mention it. We'll > > > have to circle back and revisit this code when we do. Maybe we can request > > > backing stores from the context's cache, rather than allocating them > directly. > > > > Acknowledged. > > > > > https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... > > File src/effects/SkBlurImageFilter.cpp (right): > > > > > https://codereview.chromium.org/1785643003/diff/210001/src/effects/SkBlurImag... > > src/effects/SkBlurImageFilter.cpp:160: tmp.allocPixels(info); > > On 2016/03/22 19:29:49, Stephen White wrote: > > > Shouldn't we be checking for allocation failure here? > > > > It should throw on failure. > > Which in Chrome will provoke a renderer crash, no? We should probably use > tryAllocPixels() and check the return value. Otherwise it might be fairly easy > to construct an HTML page which will crash. Done.
filters LGTM; leaving API & device for reed@
ping - Mike
can this method be called something with imagefilter in it? Does it *always* have an imagefilter present? lgtm modulo that question
The CQ bit was checked by robertphillips@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/1785643003/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1785643003/250001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by robertphillips@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/1785643003/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1785643003/270001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertphillips@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/1785643003/#ps270001 (title: "add cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1785643003/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1785643003/270001
Message was sent while issue was closed.
Description was changed from ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) https://codereview.chromium.org/1813813002/ (Add SkSpecialImage::makeTextureImage entry point) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) https://codereview.chromium.org/1813813002/ (Add SkSpecialImage::makeTextureImage entry point) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/3c935bc87020bfd19a08922f7394db3a801d168b ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001) as https://skia.googlesource.com/skia/+/3c935bc87020bfd19a08922f7394db3a801d168b
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:270001) has been created in https://codereview.chromium.org/1831603002/ by robertphillips@google.com. The reason for reverting is: serialize-8888 broken for some reason.
Message was sent while issue was closed.
Description was changed from ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) https://codereview.chromium.org/1813813002/ (Add SkSpecialImage::makeTextureImage entry point) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/3c935bc87020bfd19a08922f7394db3a801d168b ========== to ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) https://codereview.chromium.org/1813813002/ (Add SkSpecialImage::makeTextureImage entry point) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/3c935bc87020bfd19a08922f7394db3a801d168b ==========
The CQ bit was checked by robertphillips@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/1785643003/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1785643003/290001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertphillips@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/1785643003/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1785643003/310001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertphillips@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org, reed@google.com Link to the patchset: https://codereview.chromium.org/1785643003/#ps310001 (title: "update to ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1785643003/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1785643003/310001
Message was sent while issue was closed.
Description was changed from ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) https://codereview.chromium.org/1813813002/ (Add SkSpecialImage::makeTextureImage entry point) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/3c935bc87020bfd19a08922f7394db3a801d168b ========== to ========== Switch SkBlurImageFilter over to new onFilterImage interface This CL relies on: https://codereview.chromium.org/1787883002/ (Add SkSpecialImage::extractSubset & NewFromPixmap) https://codereview.chromium.org/1808743003/ (Allow SkGpuDevice::drawSprite to handle subset SkBitmaps) https://codereview.chromium.org/1813813002/ (Add SkSpecialImage::makeTextureImage entry point) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/3c935bc87020bfd19a08922f7394db3a801d168b Committed: https://skia.googlesource.com/skia/+/1579e3c376a9ee8c694a64f9c87457cea13ba442 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:310001) as https://skia.googlesource.com/skia/+/1579e3c376a9ee8c694a64f9c87457cea13ba442 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
