Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23060011:
Pass the quad's rect (contents_rect) origin to skia image filters as an offset in the CTM. (Closed)
|
Created:
7 years, 4 months ago by Stephen White Modified:
7 years, 3 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPass the quad's rect (contents_rect) origin to skia image filters as an offset in the CTM.
This won't actually do anything until I land a change in Skia to use the CTM, so no layout tests yet. (Let me know if there's a clever way to unit test this instead.)
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220360
Patch Set 1 #
Total comments: 1
Patch Set 2 : #Patch Set 3 : Add pixel test. #
Total comments: 8
Patch Set 4 : Fix skia casts #Patch Set 5 : Modify test to cover incorrect clipping offsets, and to use blue_yellow.png as a result. #
Total comments: 6
Patch Set 6 : Fix comments re: review #
Total comments: 1
Messages
Total messages: 16 (0 generated)
I-don't-know-what-I'm-doing-dog but this seems to work.
On 2013/08/16 19:34:13, Stephen White wrote: > I-don't-know-what-I'm-doing-dog but this seems to work. That seems plausible, but should there be a scale from device coordinates to CSS coordinates? In terms of clever testing, if you land the Skia change first then you could write a cc pixel test to make sure this outputs the right pixels. See renderer_pixeltest.cc or layer_tree_pixel_test.cc for some options.
On 2013/08/16 19:42:14, enne wrote: > On 2013/08/16 19:34:13, Stephen White wrote: > > I-don't-know-what-I'm-doing-dog but this seems to work. > > That seems plausible, but should there be a scale from device coordinates to CSS > coordinates? There should be, but we're currently doing the devicePixelRatio over in Blink already, so it'll take a bit of #ifdef dancing to move it here. > In terms of clever testing, if you land the Skia change first then you could > write a cc pixel test to make sure this outputs the right pixels. See > renderer_pixeltest.cc or layer_tree_pixel_test.cc for some options. No offense, but having had to rebaseline those pixel tests before, I think I'll stick with a Blink layout test, or a non-pixel cc unit test if I can think of one.
On Fri, Aug 16, 2013 at 5:06 PM, <senorblanco@chromium.org> wrote: > On 2013/08/16 19:42:14, enne wrote: > >> On 2013/08/16 19:34:13, Stephen White wrote: >> > I-don't-know-what-I'm-doing-**dog but this seems to work. >> > > That seems plausible, but should there be a scale from device coordinates >> to >> > CSS > >> coordinates? >> > > There should be, but we're currently doing the devicePixelRatio over in > Blink > already, so it'll take a bit of #ifdef dancing to move it here. > > > In terms of clever testing, if you land the Skia change first then you >> could >> write a cc pixel test to make sure this outputs the right pixels. See >> renderer_pixeltest.cc or layer_tree_pixel_test.cc for some options. >> > > No offense, but having had to rebaseline those pixel tests before, I think > I'll > stick with a Blink layout test, or a non-pixel cc unit test if I can think > of > one. > If you can write a test without AA then they are not so much headache. It's the AA tests that make for sadtimes. > > https://codereview.chromium.**org/23060011/<https://codereview.chromium.org/2... >
I'd also like to see a pixel test for this. The latency on layout test failures is far greater for cc changes. Also, we've had to rebaseline tests without blur/AA in them approximately zero times. Invert filter or the like would be reasonable IMO (i've used invert elsewhere) https://codereview.chromium.org/23060011/diff/1/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/23060011/diff/1/cc/output/gl_renderer.cc#newc... cc/output/gl_renderer.cc:518: gfx::Rect rect, better variable name than the type of the variable, what this input represents to this function? why pass in a rect but only use the origin? how about passing in just the rect's OffsetFromOrigin()?
On 2013/08/19 21:09:08, danakj wrote: > I'd also like to see a pixel test for this. The latency on layout test failures > is far greater for cc changes. Also, we've had to rebaseline tests without > blur/AA in them approximately zero times. Invert filter or the like would be > reasonable IMO (i've used invert elsewhere) Done. > https://codereview.chromium.org/23060011/diff/1/cc/output/gl_renderer.cc > File cc/output/gl_renderer.cc (right): > > https://codereview.chromium.org/23060011/diff/1/cc/output/gl_renderer.cc#newc... > cc/output/gl_renderer.cc:518: gfx::Rect rect, > better variable name than the type of the variable, what this input represents > to this function? > > why pass in a rect but only use the origin? how about passing in just the rect's > OffsetFromOrigin()? Done, assuming that's the same as origin().
https://codereview.chromium.org/23060011/diff/18001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/23060011/diff/18001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:496: RenderSurfaceFilters::Apply(filters, Will this path do the right thing already? Are there just no filter operations that depend on knowing the origin of the filter? https://codereview.chromium.org/23060011/diff/18001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:581: canvas.translate(SkFloatToScalar(-origin.x()), SkFloatToScalar(-origin.y())); nit: I think you want SkIntToScalar? https://codereview.chromium.org/23060011/diff/18001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:582: canvas.drawSprite(source, 0, 0, &paint); Could you just pass the origin here instead of 0,0 and do the same thing? https://codereview.chromium.org/23060011/diff/18001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_filters.cc (right): https://codereview.chromium.org/23060011/diff/18001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_filters.cc:182: // region scrolls off. This ensures that the crop rect is being correctly If you translated by the origin twice in the renderer, this test would still pass, right? Moving something entirely out of the viewport is going to give less confidence than moving it inside the viewport and seeing it is in the right spot. Can we do the latter? Maybe using the blue_yellow.png?
I still need some convincing about the scale here. What space is the offset in?
https://codereview.chromium.org/23060011/diff/18001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/23060011/diff/18001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:496: RenderSurfaceFilters::Apply(filters, On 2013/08/28 22:26:58, danakj wrote: > Will this path do the right thing already? Are there just no filter operations > that depend on knowing the origin of the filter? No, there are none. There are no crop rects in filter operations, and all the filter parameters are translation-independent. https://codereview.chromium.org/23060011/diff/18001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:581: canvas.translate(SkFloatToScalar(-origin.x()), SkFloatToScalar(-origin.y())); On 2013/08/28 22:26:58, danakj wrote: > nit: I think you want SkIntToScalar? Done. Thanks! https://codereview.chromium.org/23060011/diff/18001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:582: canvas.drawSprite(source, 0, 0, &paint); On 2013/08/28 22:26:58, danakj wrote: > Could you just pass the origin here instead of 0,0 and do the same thing? Nope. We're drawing the bitmap in the correct place, it's just the filter parameters we want to affect. If we applied the origin here, the bitmap would be offset. https://codereview.chromium.org/23060011/diff/18001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_filters.cc (right): https://codereview.chromium.org/23060011/diff/18001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_filters.cc:182: // region scrolls off. This ensures that the crop rect is being correctly On 2013/08/28 22:26:58, danakj wrote: > If you translated by the origin twice in the renderer, this test would still > pass, right? Yes, it would still pass. But the layout tests I've got ready to go in my WebKit patch would all fail. Still let's make sure both the belt and suspenders work. :) > Moving something entirely out of the viewport is going to give less > confidence than moving it inside the viewport and seeing it is in the right > spot. Can we do the latter? Maybe using the blue_yellow.png? OK, I'll give that a shot.
On 2013/08/28 23:25:25, enne wrote: > I still need some convincing about the scale here. What space is the offset in? It's applied post-DPR and post-page scale (same as the RenderSurface, which is the source for the filter). So the scale is 1:1. We don't handle pinch-zoom yet, at which point we'll likely have to set a scale here. But that requires some infrastructure work in Skia and Blink.
PTAL. New test uses blue_yellow.png, and ensures that only the correct offset passes.
LGTM. enne@ ? https://codereview.chromium.org/23060011/diff/33001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/23060011/diff/33001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:581: canvas.translate(SkIntToScalar(-origin.x()), SkIntToScalar(-origin.y())); Can you add a TODO here stating that the filter should be scaled by the scale from the layer space of the root-most layer drawn into the render pass draw quad's texture (ie. the surface owning texture), to the render pass draw quad's texture? And point to a crbug where we will do this? https://codereview.chromium.org/23060011/diff/33001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_filters.cc (right): https://codereview.chromium.org/23060011/diff/33001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_filters.cc:178: // Make the foreground layer's render surface be clipped by the background layer. https://codereview.chromium.org/23060011/diff/33001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_filters.cc:183: // region scrolls to the top. This ensures that the crop rect is being nit: s/scrolls/is moved to/
PTAL https://codereview.chromium.org/23060011/diff/33001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/23060011/diff/33001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:581: canvas.translate(SkIntToScalar(-origin.x()), SkIntToScalar(-origin.y())); On 2013/08/29 02:07:16, danakj wrote: > Can you add a TODO here stating that the filter should be scaled by the scale > from the layer space of the root-most layer drawn into the render pass draw > quad's texture (ie. the surface owning texture), to the render pass draw quad's > texture? And point to a crbug where we will do this? Done. https://codereview.chromium.org/23060011/diff/33001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_pixeltest_filters.cc (right): https://codereview.chromium.org/23060011/diff/33001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_filters.cc:178: On 2013/08/29 02:07:16, danakj wrote: > // Make the foreground layer's render surface be clipped by the background > layer. Done. https://codereview.chromium.org/23060011/diff/33001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_pixeltest_filters.cc:183: // region scrolls to the top. This ensures that the crop rect is being On 2013/08/29 02:07:16, danakj wrote: > nit: s/scrolls/is moved to/ Done.
lgtm https://codereview.chromium.org/23060011/diff/43001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/23060011/diff/43001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:572: // TODO(senorblanco): in addition to the origin translation here, the canvas Thanks for the TODO. Scale doesn't need to be solved here, but I just wanted some reminder about what Skia was expecting (CSS pixels * device scale, I think?) when I look at this again in the future.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/senorblanco@chromium.org/23060011/43001
Message was sent while issue was closed.
Change committed as 220360 |
