Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24090003:
Enable general CSS filters in the software compositor (Closed)
|
Created:
7 years, 3 months ago by ajuma Modified:
7 years, 1 month ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable general CSS filters in the software compositor
The software compositor has only supported filters that include a
reference filter. This CL enables support for filters that don't
include a reference filter.
BUG=160302
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229569
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232476
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #
Total comments: 6
Patch Set 4 : Rebased #Patch Set 5 : Address review comments #Patch Set 6 : Rebased again #Patch Set 7 : Rebased again #Patch Set 8 : Rebased #Patch Set 9 : Fix Windows crash #
Total comments: 1
Messages
Total messages: 41 (0 generated)
https://chromiumcodereview.appspot.com/24090003/diff/1/cc/output/software_ren... File cc/output/software_renderer.cc (right): https://chromiumcodereview.appspot.com/24090003/diff/1/cc/output/software_ren... cc/output/software_renderer.cc:443: // TODO(ajuma): Pass the quad's rect origin when applying the filter. This is why ImageFilterClipped pixel test doesn't pass with the software renderer. We need something analogous to https://chromiumcodereview.appspot.com/23060011. Thoughts on how to approach this?
https://chromiumcodereview.appspot.com/24090003/diff/1/cc/output/software_ren... File cc/output/software_renderer.cc (right): https://chromiumcodereview.appspot.com/24090003/diff/1/cc/output/software_ren... cc/output/software_renderer.cc:443: // TODO(ajuma): Pass the quad's rect origin when applying the filter. On 2013/09/11 15:59:45, ajuma wrote: > This is why ImageFilterClipped pixel test doesn't pass with the software > renderer. We need something analogous to > https://chromiumcodereview.appspot.com/23060011. Thoughts on how to approach > this? It should just be a matter of getting the correct origin from quad->rect.origin(), and applying that as a transform in current_canvas_. This may require changing the draw call below from a drawRect() to a drawSprite(), however, since we want the CTM to apply to the filter, but not to the primitive's position. I suppose we could do a special case for filters here, and early out. That would skip the mask code below, but I'm not sure filters + mask does the correct thing in the existing code anyway -- TBD. The other option would be to do a drawSprite() with only the filter applied, and then use the resulting bitmap for the drawRect() call below. Then we could handle masks correctly, I think.
https://chromiumcodereview.appspot.com/24090003/diff/1/cc/output/software_ren... File cc/output/software_renderer.cc (right): https://chromiumcodereview.appspot.com/24090003/diff/1/cc/output/software_ren... cc/output/software_renderer.cc:443: // TODO(ajuma): Pass the quad's rect origin when applying the filter. On 2013/09/11 16:48:06, Stephen White wrote: > On 2013/09/11 15:59:45, ajuma wrote: > > This is why ImageFilterClipped pixel test doesn't pass with the software > > renderer. We need something analogous to > > https://chromiumcodereview.appspot.com/23060011. Thoughts on how to approach > > this? > > It should just be a matter of getting the correct origin from > quad->rect.origin(), and applying that as a transform in current_canvas_. This > may require changing the draw call below from a drawRect() to a drawSprite(), > however, since we want the CTM to apply to the filter, but not to the > primitive's position. I suppose we could do a special case for filters here, and > early out. That would skip the mask code below, but I'm not sure filters + mask > does the correct thing in the existing code anyway -- TBD. The other option > would be to do a drawSprite() with only the filter applied, and then use the > resulting bitmap for the drawRect() call below. Then we could handle masks > correctly, I think. AFAIK (I wrote the code) masks + filters do work correctly together with the current code. I think there might be a layout test or two testing that.
PTAL. Note that four layout tests from virtual/softwarecompositing/culling (specifically, filter-occlusion-alpha-large.html, filter-occlusion-alpha.html, filter-occlusion-blur-large.html, and filter-occlusion-blur.html) will need to be rebaselined, since their current expected images are failures but with this CL they now pass. https://codereview.chromium.org/24090003/diff/1/cc/output/software_renderer.cc File cc/output/software_renderer.cc (right): https://codereview.chromium.org/24090003/diff/1/cc/output/software_renderer.c... cc/output/software_renderer.cc:443: // TODO(ajuma): Pass the quad's rect origin when applying the filter. On 2013/09/11 16:48:06, Stephen White wrote: > The other option > would be to do a drawSprite() with only the filter applied, and then use the > resulting bitmap for the drawRect() call below. Then we could handle masks > correctly, I think. Done.
On 2013/10/15 19:36:20, ajuma wrote: > PTAL. > > Note that four layout tests from virtual/softwarecompositing/culling > (specifically, filter-occlusion-alpha-large.html, filter-occlusion-alpha.html, > filter-occlusion-blur-large.html, and filter-occlusion-blur.html) will need to > be rebaselined, since their current expected images are failures but with this > CL they now pass. You might want to let the gardener know about that, or land pre-emptive suppressions. > https://codereview.chromium.org/24090003/diff/1/cc/output/software_renderer.cc > File cc/output/software_renderer.cc (right): > > https://codereview.chromium.org/24090003/diff/1/cc/output/software_renderer.c... > cc/output/software_renderer.cc:443: // TODO(ajuma): Pass the quad's rect origin > when applying the filter. > On 2013/09/11 16:48:06, Stephen White wrote: > > The other option > > would be to do a drawSprite() with only the filter applied, and then use the > > resulting bitmap for the drawRect() call below. Then we could handle masks > > correctly, I think. > > Done.
LGTM. Nice! https://codereview.chromium.org/24090003/diff/21001/cc/output/software_render... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/24090003/diff/21001/cc/output/software_render... cc/output/software_renderer.cc:460: filter_bitmap = device->accessBitmap(false); Might want to log a bug that there's a possible optimization here, when the filter DAG is simple enough to apply in the same pass as the content (and there's no origin offset to apply). Could at least do an SkColorFilterImageFilter, probably more.
LGTM w/ 2 things. https://codereview.chromium.org/24090003/diff/21001/cc/output/software_render... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/24090003/diff/21001/cc/output/software_render... cc/output/software_renderer.cc:452: false)); can you use temporary variables so these boolean literals have names? https://codereview.chromium.org/24090003/diff/21001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_filters.cc (right): https://codereview.chromium.org/24090003/diff/21001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_filters.cc:198: RunPixelTest(SOFTWARE_WITH_BITMAP, Can you split this into a _GL and _Software versions of the test? A super class will help share code. It could just have a static method that takes GL_WITH_BITMAP or SOFTWARE_WITH_BITMAP as an argument.
On 2013/10/15 19:46:33, Stephen White wrote: > On 2013/10/15 19:36:20, ajuma wrote: > > Note that four layout tests from virtual/softwarecompositing/culling > > (specifically, filter-occlusion-alpha-large.html, filter-occlusion-alpha.html, > > filter-occlusion-blur-large.html, and filter-occlusion-blur.html) will need to > > be rebaselined, since their current expected images are failures but with this > > CL they now pass. > > You might want to let the gardener know about that, or land pre-emptive > suppressions. I've landed pre-emptive suppressions. https://codereview.chromium.org/24090003/diff/21001/cc/output/software_render... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/24090003/diff/21001/cc/output/software_render... cc/output/software_renderer.cc:452: false)); On 2013/10/16 15:48:59, danakj wrote: > can you use temporary variables so these boolean literals have names? Done. https://codereview.chromium.org/24090003/diff/21001/cc/output/software_render... cc/output/software_renderer.cc:460: filter_bitmap = device->accessBitmap(false); On 2013/10/15 19:49:19, Stephen White wrote: > Might want to log a bug that there's a possible optimization here, when the > filter DAG is simple enough to apply in the same pass as the content (and > there's no origin offset to apply). Could at least do an > SkColorFilterImageFilter, probably more. Done and added a TODO for this too. https://codereview.chromium.org/24090003/diff/21001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_filters.cc (right): https://codereview.chromium.org/24090003/diff/21001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_filters.cc:198: RunPixelTest(SOFTWARE_WITH_BITMAP, On 2013/10/16 15:48:59, danakj wrote: > Can you split this into a _GL and _Software versions of the test? A super class > will help share code. It could just have a static method that takes > GL_WITH_BITMAP or SOFTWARE_WITH_BITMAP as an argument. Done, though note that the superclass method must be non-static, since it calls other non-static methods (e.g. CreateSolidColorLayer, RunPixelTest).
Thanks, still LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/66001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/255001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/255001
Retried try job too often on win_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/255001
Message was sent while issue was closed.
Change committed as 229569
Reopening since this was reverted for causing a crash on Windows
content_browsertests with the following stack:
S32A_Opaque_BlitRow32_SSE2 [0x014F2E90+160]
Sprite_D32_S32::blitRect [0x014C2CE1+81]
SkScan::FillIRect [0x01484750+320]
SkScan::FillIRect [0x01484830+64]
SkDraw::drawSprite [0x0144E36A+314]
SkBitmapDevice::drawSprite [0x014262AB+27]
SkCanvas::drawSprite [0x014295DF+639]
cc::SoftwareRenderer::DrawRenderPassQuad [0x015C0E11+705]
cc::SoftwareRenderer::DoDrawQuad [0x015C13DB+715]
Interestingly, the same test (WebContentsViewAuraTest.OverscrollScreenshot)
doesn't crash on Linux Aura.
PTAL. https://codereview.chromium.org/24090003/diff/575001/cc/output/software_rende... File cc/output/software_renderer.cc (right): https://codereview.chromium.org/24090003/diff/575001/cc/output/software_rende... cc/output/software_renderer.cc:451: skia::AdoptRef(new SkBitmapDevice(SkBitmap::kARGB_8888_Config, Using createCompatibleDevice leads to a crash on Windows, since it creates a BitmapPlatformDevice. As a result, when applying the filter during canvas.drawSprite, the temporary device that's created in SkColorFilterImageFilter::onFilterImage is also a BitmapPlatformDevice. Such a device's pixel data cannot be assigned to another bitmap since the data is owned by the device rather than being refcounted (see comments in bitmap_platform_device_win.h). But onFilterImage does assign the device's pixel data to *result, meaning we have a use-after-free when we later try to access the result's pixel data. Using an SkBitmapDevice avoids this issue.
On 2013/10/31 17:36:09, ajuma wrote: > PTAL. > > https://codereview.chromium.org/24090003/diff/575001/cc/output/software_rende... > File cc/output/software_renderer.cc (right): > > https://codereview.chromium.org/24090003/diff/575001/cc/output/software_rende... > cc/output/software_renderer.cc:451: skia::AdoptRef(new > SkBitmapDevice(SkBitmap::kARGB_8888_Config, > Using createCompatibleDevice leads to a crash on Windows, since it creates a > BitmapPlatformDevice. As a result, when applying the filter during > canvas.drawSprite, the temporary device that's created in > SkColorFilterImageFilter::onFilterImage is also a BitmapPlatformDevice. Such a > device's pixel data cannot be assigned to another bitmap since the data is owned > by the device rather than being refcounted (see comments in > bitmap_platform_device_win.h). But onFilterImage does assign the device's pixel > data to *result, meaning we have a use-after-free when we later try to access > the result's pixel data. > > Using an SkBitmapDevice avoids this issue. Good sleuthing! LGTM
Indeed, LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/575001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajuma@chromium.org/24090003/575001
Message was sent while issue was closed.
Change committed as 232476 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
