Currently, when the device scale factor is two, render surfaces are scaled up to the appropriate position on the page, but they've been rastered at half-width, half-height. We need to change the appropriate bounds and transforms in calcDrawTransformsAndVisibility to correct this.
Created attachment 143565 [details] Patch
Comment on attachment 143565 [details] Patch If we accept how WebCore gives us these scaling factors, the patch seems OK to me. But I spent some time thinking about this - I feel really uncomfortable with the way that deviceScaleFactor and pageScaleFactor are being encoded as a scaling factor inside of each layer - and then for most layers we don't even use its own contentsScale(), but rather inherit the contentsScale() from an ancestor. Yes, this patch looks like it will work correctly because we have "global" knowledge about what contentsScale() actually is for the entire tree... but what that really means is that it shouldn't be a variable that is unique for every layer in the first place. Is there ever a case where contentsScale() is unique for a subtree of the layer tree? Looking at the code, contentsScale() is a combination of deviceScaleFactor (totally global per layer tree) and pageScaleFactor (also totally global per layer tree ??). In my opinion, if there's a variable called "contentsScale" that is stored for every layer, then it should mean that every layer could have a unique scale for its contents within its bounds, and that would have a very different meaning than deviceScaleFactor and pageScaleFactor. People can easily trip over this confusion, costing more time and bugs. So, based on that, I wonder if maybe we should go through the trouble of talking with WebCore guys about passing deviceScaleFactor (and pageScaleFactor ?) to the layer tree host, rather than being given to each layer. View in context: https://bugs.webkit.org/attachment.cgi?id=143565&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:448 > + transformedLayerRect = IntRect(0, 0, contentsScale * bounds.width(), contentsScale * bounds.height()); So if I'm understanding this correctly: (1) layer that owns surface needs to be scaled so that its pixel size when drawing will match the device hi DPI (2) this scale is implicitly included in the entire subtree, because this matrix is also inherited by all children, and (3) the existing code for computing surface size will include the correct high DPI size because of (1) and (2). Is this correct? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:457 > + surfaceOriginTransform.scale(1 / contentsScale); I think a comment is badly needed here. At a glance, it looks like this 1/contentsScale is just canceling out the use of transformedLayerRect, making it equivalent to using bounds.width() directly, right? Is this really necessary? Why couldn't we just keep the original translate3d using bounds.width/height, and avoid the 1/contentsScale? > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1677 > + parent->setIsDrawable(true); We have the LayerChromiumWithForcedDrawsContent class, that could be used instead of calling setIsDrawable() > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1685 > + child->setBounds(IntSize(10, 10)); > + child->setPosition(IntPoint(2, 2)); > + child->setAnchorPoint(FloatPoint(0, 0)); and it should be possible to use setLayerPropertiesForTesting() for initializing each layer.
(In reply to comment #2) > (From update of attachment 143565 [details]) > If we accept how WebCore gives us these scaling factors, the patch seems OK to me. But I spent some time thinking about this - I feel really uncomfortable with the way that deviceScaleFactor and pageScaleFactor are being encoded as a scaling factor inside of each layer - and then for most layers we don't even use its own contentsScale(), but rather inherit the contentsScale() from an ancestor. Yes, this patch looks like it will work correctly because we have "global" knowledge about what contentsScale() actually is for the entire tree... but what that really means is that it shouldn't be a variable that is unique for every layer in the first place. Is there ever a case where contentsScale() is unique for a subtree of the layer tree? Looking at the code, contentsScale() is a combination of deviceScaleFactor (totally global per layer tree) and pageScaleFactor (also totally global per layer tree ??). In my opinion, if there's a variable called "contentsScale" that is stored for every layer, then it should mean that every layer could have a unique scale for its contents within its bounds, and that would have a very different meaning than deviceScaleFactor and pageScaleFactor. People can easily trip over this confusion, costing more time and bugs. > > So, based on that, I wonder if maybe we should go through the trouble of talking with WebCore guys about passing deviceScaleFactor (and pageScaleFactor ?) to the layer tree host, rather than being given to each layer. Yeh.. this is a huge issue, and something we can discuss but I think it belongs outside of this bug fix.. and it started in bug #86051. If we want to rethink contents scaling then we should do that separately from this change, which is just adjusting the surface transforms to match the current contentsScale system. > > View in context: https://bugs.webkit.org/attachment.cgi?id=143565&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:448 > > + transformedLayerRect = IntRect(0, 0, contentsScale * bounds.width(), contentsScale * bounds.height()); > > So if I'm understanding this correctly: (1) layer that owns surface needs to be scaled so that its pixel size when drawing will match the device hi DPI (2) this scale is implicitly included in the entire subtree, because this matrix is also inherited by all children, and (3) the existing code for computing surface size will include the correct high DPI size because of (1) and (2). Is this correct? > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:457 > > + surfaceOriginTransform.scale(1 / contentsScale); > > I think a comment is badly needed here. At a glance, it looks like this 1/contentsScale is just canceling out the use of transformedLayerRect, making it equivalent to using bounds.width() directly, right? Is this really necessary? Why couldn't we just keep the original translate3d using bounds.width/height, and avoid the 1/contentsScale? > > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1677 > > + parent->setIsDrawable(true); > > We have the LayerChromiumWithForcedDrawsContent class, that could be used instead of calling setIsDrawable() > > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1685 > > + child->setBounds(IntSize(10, 10)); > > + child->setPosition(IntPoint(2, 2)); > > + child->setAnchorPoint(FloatPoint(0, 0)); > > and it should be possible to use setLayerPropertiesForTesting() for initializing each layer.
Created attachment 143627 [details] Patch (In reply to comment #2) > (From update of attachment 143565 [details]) > If we accept how WebCore gives us these scaling factors, the patch seems OK to me. But I spent some time thinking about this - I feel really uncomfortable with the way that deviceScaleFactor and pageScaleFactor are being encoded as a scaling factor inside of each layer - and then for most layers we don't even use its own contentsScale(), but rather inherit the contentsScale() from an ancestor. Yes, this patch looks like it will work correctly because we have "global" knowledge about what contentsScale() actually is for the entire tree... but what that really means is that it shouldn't be a variable that is unique for every layer in the first place. Is there ever a case where contentsScale() is unique for a subtree of the layer tree? Looking at the code, contentsScale() is a combination of deviceScaleFactor (totally global per layer tree) and pageScaleFactor (also totally global per layer tree ??). In my opinion, if there's a variable called "contentsScale" that is stored for every layer, then it should mean that every layer could have a unique scale for its contents within its bounds, and that would have a very different meaning than deviceScaleFactor and pageScaleFactor. People can easily trip over this confusion, costing more time and bugs. > > So, based on that, I wonder if maybe we should go through the trouble of talking with WebCore guys about passing deviceScaleFactor (and pageScaleFactor ?) to the layer tree host, rather than being given to each layer. I does seem strange to have contentsScale per layer. What would you think of passing these values separately as parameters to calcDrawTransformsEtc? > View in context: https://bugs.webkit.org/attachment.cgi?id=143565&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:448 > > + transformedLayerRect = IntRect(0, 0, contentsScale * bounds.width(), contentsScale * bounds.height()); > > So if I'm understanding this correctly: (1) layer that owns surface needs to be scaled so that its pixel size when drawing will match the device hi DPI (2) this scale is implicitly included in the entire subtree, because this matrix is also inherited by all children, and (3) the existing code for computing surface size will include the correct high DPI size because of (1) and (2). Is this correct? I could certainly have this wrong, but It's my understanding that the transformedLayerRect needed to be in physical pixels so that we generate textures of the appropriate size. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:457 > > + surfaceOriginTransform.scale(1 / contentsScale); > > I think a comment is badly needed here. At a glance, it looks like this 1/contentsScale is just canceling out the use of transformedLayerRect, making it equivalent to using bounds.width() directly, right? Is this really necessary? Why couldn't we just keep the original translate3d using bounds.width/height, and avoid the 1/contentsScale? I'm a bit shaky on this too, but it looks like surfaceOriginTransform transforms from content space to its parent's content space. Since we're always in physical pixels, there was no need to scale by contentsScale, so I 'unscaled' by contentsScale. In any case, you're totally right about needing a comment here. > > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1677 > > + parent->setIsDrawable(true); > > We have the LayerChromiumWithForcedDrawsContent class, that could be used instead of calling setIsDrawable() I needed the layers to be ContentLayerChromium's so that setContentsScale would have an effect. I've added a helper function createDrawableContentLayerChromium to reduce the duplicated code. Please let me know if there's a better approach. > > > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:1685 > > + child->setBounds(IntSize(10, 10)); > > + child->setPosition(IntPoint(2, 2)); > > + child->setAnchorPoint(FloatPoint(0, 0)); > > and it should be possible to use setLayerPropertiesForTesting() for initializing each layer. Good call. Done.
I have to admit that this patch confuses me a little bit because the root surface is treated differently than render surfaces. The default render surface and viewport seems to be in layout space, but this patch makes it so that non-default render surfaces are in device pixel space. So, you have to do all this funky draw transform and origin transform manipulation to get layers to move to the right spot within those render surface trees. Maybe this is a ridiculous suggestion, but could we remove all this transformation complexity by simply treating non-default render surfaces more like we treat the default one? In other words, create render surfaces of a size that includes the device scale in CCRenderSurface::prepareContentsTexture, but use the layout size everywhere else in terms of transformations and the "viewport" when setting up our projection and window matrix?
(In reply to comment #5) > In other words, create render surfaces of a size that includes the device scale in CCRenderSurface::prepareContentsTexture, but use the layout size everywhere else in terms of transformations and the "viewport" when setting up our projection and window matrix? Or, even more specifically, (1) request a scaled texture size in CCRenderSurface::prepareContentsTexture and (2) use a scaled viewport in LayerRendererChromium::setDrawViewportRect for both the m_context->viewport and m_windowMatrix calls.
Summary of thoughts/discussions: This patch does do things on non-root surfaces the same way as is done for the root surface. The root surface is pixel-sized because the NCCH's bounds are in pixels. We could scale into the root via viewport transform.. Advantages to doing scale via viewport transform: We don't have to scale anything in CCLTHCommon. Disadvantages: We do this scaling in a different way than pageScale does (code reuse?). Non-root surfaces with pageScale != 1 continue to be wrong.
Ugh, page scale. Maybe it'd be best to do something that potentially solves this for page scale as well and just adjust transforms too. Sorry for the distracting suggestion.
Comment on attachment 143627 [details] Patch In reference to comments #5-#8, I'm OK with either approach. It sounds like the conclusion was to stick with the original approach because it works for pageScaleFactor as well, right? View in context: https://bugs.webkit.org/attachment.cgi?id=143627&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:457 > + // The surfaceOriginTransform transforms points in the surface's content space Question for myself, probably nothing needs to be changed here: So it seems like you and Dana often use this terminology of "content space" and "layout space" --> that makes a lot of sense to me for layers, but how does it work for surfaces? Is there ever a case where surface content space != surface layout space? In particular, the surface doesn't explicitly do anything related to these scaling factors, it simply adapts its existing semantics to the new scalings that we're applying to layers themselves. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:458 > + // to its parent's content space. Distances in these spaces are both in physical I think its a little bit ill-defined what a surface's "parent" is. are we talking about the surface's owning layer's parent? or are we talking about the surface's ancestor surface (i.e. the current surface's target surface) ? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:462 > + surfaceOriginTransform.scale(1 / contentsScale); > + surfaceOriginTransform.translate3d(-0.5 * transformedLayerRect.width(), -0.5 * transformedLayerRect.height(), 0); So after reading this new comment, I'm thinking more and more that this is equivalent to using the original bounds.width() and bounds.height(). And if that's correct, then its probably best do do that, to (a) avoid an extra division and matrix multiplication, and (b) so that along with the right comment the code is clearer about the role of render surfaces with respect to this new scaling behavior -- i.e. the role doesn't change from what it was before this patch. in the old code, surfaceOriginTransform = combinedTransform * translationMatrix(halfBoundsWidth, halfBoundsHeight) And in the new code, surfaceOriginTransform = combinedTransform * scaleMatrix(1/contentsScale) * translationMatrix( contentsScale * halfBoundsWidth, contentsScale * halfBoundsHeight) specifically, I am pretty sure that scaleMatrix(1/contentsScale) * translationMatrix(contentsScale * halfBoundsWidth, contentsScale*halfBoundsHeight) == translationMatrix(halfBoundsWidth, halfBoundsHeight) I think this is correct - if you still think we need this 1/contentsScale indirection, lets discuss =)
And a few replies that I had left hanging from earlier: (In reply to comment #4) > I does seem strange to have contentsScale per layer. What would you think of passing these values separately as parameters to calcDrawTransformsEtc? I think if its necessary, we should do it, and we've reached a threshold where we should consider grouping things in structs instead of having this ridiculous number of args. But what Dana replied also seems reasonable, that we could deal with it separately from this bug. > > > View in context: https://bugs.webkit.org/attachment.cgi?id=143565&action=review > > > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:448 > > > + transformedLayerRect = IntRect(0, 0, contentsScale * bounds.width(), contentsScale * bounds.height()); > > > > So if I'm understanding this correctly: (1) layer that owns surface needs to be scaled so that its pixel size when drawing will match the device hi DPI (2) this scale is implicitly included in the entire subtree, because this matrix is also inherited by all children, and (3) the existing code for computing surface size will include the correct high DPI size because of (1) and (2). Is this correct? > > I could certainly have this wrong, but It's my understanding that the transformedLayerRect needed to be in physical pixels so that we generate textures of the appropriate size. > Sorry, it wasn't clear by your response if my summary was correct or not? =) As for transformedLayerRect being in physical pixel sizes, that seems to fit with my understanding too...
Comment on attachment 143627 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143627&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:471 > + layer->setContentsScale(contentsScale()); This patch must be using contents scale in a different way than it's normally used. Contents scale is normally just the ratio between the content bounds and bounds. Is this number something else? If so, what is it?
Comment on attachment 143627 [details] Patch Keep in mind render surfaces can be whatever size we want - they're an artificial intermediate we use and so long as we apply the correct scales they can be any size we want. We want going through an intermediate render surface to be as lossless a process as we can afford, so it sounds reasonable that on a pixel-doubled device a viewport-sized layer should have a width/height of twice the CSS pixel width/height of the layer. It's less obvious to me how page scale on a layer should impact the render surface size. If there's a case where we should be changing the RS size based on page scale, would someone mind drawing up a brief diagram of how that should interact?
Created attachment 146638 [details] Patch Not ready for review. Still need to test on device (in particular, scrolling and pinch-zoom).
Comment on attachment 146638 [details] Patch Attachment 146638 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12923594 New failing tests: CCLayerTreeHostTestDeviceScaleFactorScalesViewportAndLayers.runMultiThread
Created attachment 146683 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #14) > (From update of attachment 146638 [details]) > Attachment 146638 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12923594 > > New failing tests: > CCLayerTreeHostTestDeviceScaleFactorScalesViewportAndLayers.runMultiThread Yeh that test isn't going to work anymore, which is good. Maybe we can do a test like Zeev's recent tests that check what quads are being drawn to GL and how?
Comment on attachment 146638 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146638&action=review I like this patch a *lot* better. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1517 > + IntRect scaledScissorRect = scissorRect; > + scaledScissorRect.scale(settings().deviceScaleFactor); Is deviceScaleFactor always an integer? If no, how does this scissor math work out when it's not? > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:125 > + contentSize.scale(layerRenderer->settings().deviceScaleFactor); > + ASSERT(m_contentSize == contentSize); This is kind of awkward. Maybe you should just compute m_contentSize in setContentRect. > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:151 > + // FIXME: This should be renamed since it is no longer in physical pixels. This seems like a small change. Can you just do this in this patch?
Created attachment 146920 [details] Patch (In reply to comment #17) > (From update of attachment 146638 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146638&action=review > > I like this patch a *lot* better. > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1517 > > + IntRect scaledScissorRect = scissorRect; > > + scaledScissorRect.scale(settings().deviceScaleFactor); > > Is deviceScaleFactor always an integer? If no, how does this scissor math work out when it's not? Good question. It may eventually be a non-integer, and may cause rounding issues. I've converted the rects being scaled to FloatRects, but these values are certainly squashed back into integers, currently. We will need to make the scissor rect use float rects, but this feels like a separate change. I will open a bug for this. > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:125 > > + contentSize.scale(layerRenderer->settings().deviceScaleFactor); > > + ASSERT(m_contentSize == contentSize); > > This is kind of awkward. Maybe you should just compute m_contentSize in setContentRect. The reason for the awkwardness is that we need the settings from the LRC to get the scale factor, and we done have it in setContentRect. > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:151 > > + // FIXME: This should be renamed since it is no longer in physical pixels. > > This seems like a small change. Can you just do this in this patch? Actually, it started to get pretty large and was making it tough to find the 'real' change in the patch. Shawn mentioned some other names that should be clarified. Maybe it would be useful to have a patch to clean up all the names and in the description of the patch gave a little glossary of the terms we use?
deviceScale will definitely be non-integer in many situations - on Win8, device scale factors are 1.4 and 1.8. On android 1.5 is not uncommon. Mac and CrOS appear to be the only platforms sticking to integer scales for now (1 / 2)
(In reply to comment #18) > Created an attachment (id=146920) [details] > Patch > > (In reply to comment #17) > > (From update of attachment 146638 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=146638&action=review > > > > I like this patch a *lot* better. > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1517 > > > + IntRect scaledScissorRect = scissorRect; > > > + scaledScissorRect.scale(settings().deviceScaleFactor); > > > > Is deviceScaleFactor always an integer? If no, how does this scissor math work out when it's not? > Good question. It may eventually be a non-integer, and may cause rounding issues. I've converted the rects being scaled to FloatRects, but these values are certainly squashed back into integers, currently. We will need to make the scissor rect use float rects, but this feels like a separate change. I will open a bug for this. Yeah, you can't have float scissor rects in gl. I guess CClayerTreeHostCommon's subtreeShouldRenderToSeparateSurface should theoretically catch this case, but in looking at code I don't think isScaleOrTranslation is the right check for determining if a scissor is pixel aligned. Can you put a link to the bug here? > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:125 > > > + contentSize.scale(layerRenderer->settings().deviceScaleFactor); > > > + ASSERT(m_contentSize == contentSize); > > > > This is kind of awkward. Maybe you should just compute m_contentSize in setContentRect. > The reason for the awkwardness is that we need the settings from the LRC to get the scale factor, and we done have it in setContentRect. Oh, I see. I think the weird part to me is that the two prepare functions aren't symmetrical. It's not obvious that the caller needs to call prepareContentsTexture before hasValidBackgroundTexture can return true, since the former sets m_contentSize used by the latter. > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:151 > > > + // FIXME: This should be renamed since it is no longer in physical pixels. > > > > This seems like a small change. Can you just do this in this patch? > Actually, it started to get pretty large and was making it tough to find the 'real' change in the patch. > Shawn mentioned some other names that should be clarified. Maybe it would be useful to have a patch to clean up all the names and in the description of the patch gave a little glossary of the terms we use? If it's larger than it sounds, then punting is fine. Can you file a dependent bug to fix this particular naming issue and put a link here? I don't know that everything has to be renamed at once.
Created attachment 147810 [details] Patch (In reply to comment #20) > (In reply to comment #18) > > Created an attachment (id=146920) [details] [details] > > Patch > > > > (In reply to comment #17) > > > (From update of attachment 146638 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=146638&action=review > > > > > > I like this patch a *lot* better. > > > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1517 > > > > + IntRect scaledScissorRect = scissorRect; > > > > + scaledScissorRect.scale(settings().deviceScaleFactor); > > > > > > Is deviceScaleFactor always an integer? If no, how does this scissor math work out when it's not? > > Good question. It may eventually be a non-integer, and may cause rounding issues. I've converted the rects being scaled to FloatRects, but these values are certainly squashed back into integers, currently. We will need to make the scissor rect use float rects, but this feels like a separate change. I will open a bug for this. > > Yeah, you can't have float scissor rects in gl. I guess CClayerTreeHostCommon's subtreeShouldRenderToSeparateSurface should theoretically catch this case, but in looking at code I don't think isScaleOrTranslation is the right check for determining if a scissor is pixel aligned. Can you put a link to the bug here? There are some glitches with non-integer device scale factors. Most have been ironed out, but there's still at least one subtle bug to fix. It worries me a bit that with this approach -- scaling in LRC rather than in the scene graph -- we expose ourselves to many subtle rounding bugs. Even if we get the current bugs fixed, I think it would be easy to introduce a new one. There was no such risk with the old approach, as gross as it looked; non-integer scale factors worked correctly. I have made a dependent bug for this. > > > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.cpp:125 > > > > + contentSize.scale(layerRenderer->settings().deviceScaleFactor); > > > > + ASSERT(m_contentSize == contentSize); > > > > > > This is kind of awkward. Maybe you should just compute m_contentSize in setContentRect. > > The reason for the awkwardness is that we need the settings from the LRC to get the scale factor, and we done have it in setContentRect. > > Oh, I see. I think the weird part to me is that the two prepare functions aren't symmetrical. It's not obvious that the caller needs to call prepareContentsTexture before hasValidBackgroundTexture can return true, since the former sets m_contentSize used by the latter. Yuck, good point. The functions are now symmetrical. > > > > > Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:151 > > > > + // FIXME: This should be renamed since it is no longer in physical pixels. > > > > > > This seems like a small change. Can you just do this in this patch? > > Actually, it started to get pretty large and was making it tough to find the 'real' change in the patch. > > Shawn mentioned some other names that should be clarified. Maybe it would be useful to have a patch to clean up all the names and in the description of the patch gave a little glossary of the terms we use? > > If it's larger than it sounds, then punting is fine. Can you file a dependent bug to fix this particular naming issue and put a link here? I don't know that everything has to be renamed at once. Great, I've made a bug for this (89212).
Created attachment 147811 [details] Patch Ack, sorry. Last patch was a dud.
Created attachment 147823 [details] Patch
Comment on attachment 147823 [details] Patch Attachment 147823 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12968113 New failing tests: compositing/scaling/tiled-layer-recursion.html
Created attachment 147863 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Sorry to have sent you off on such a wrong path, but I've come around to agreeing that calcDrawTransforms really needs to be operating in physical and not logical pixels. If we somehow allowed floating point logical pixel clips in the compositor, and then tried to convert them later to integer physical pixel clips, we'd run into any number of floating point issues. GL only operates in physical pixels and its clip rects are ints, so in some ways it makes a lot of sense to operate in that space directly. And, it's not just clips and scissoring. The culling system also assumes that enclosingIntRect/enclosedIntRect on pixels rounds to physical pixels. These are all systems that expect exactness and it would be a nightmare to make that work ok (or to fix bugs when it failed.) It would be helpful to have comments on LayerChromium.h describing what space various members are in, e.g. bounds() is in logical pixels, drawTransform() converts logical into physical pixels, transform() operates on logical pixels. I have a few worries that would make me feel better if you could investigate: What space is hit testing and input in? Do they need to be converted to physical pixel space? Are we correctly passing back scroll position from the compositor thread in logical pixel space?
Created attachment 148203 [details] Patch (In reply to comment #26) > Sorry to have sent you off on such a wrong path, but I've come around to agreeing that calcDrawTransforms really needs to be operating in physical and not logical pixels. > > If we somehow allowed floating point logical pixel clips in the compositor, and then tried to convert them later to integer physical pixel clips, we'd run into any number of floating point issues. GL only operates in physical pixels and its clip rects are ints, so in some ways it makes a lot of sense to operate in that space directly. And, it's not just clips and scissoring. The culling system also assumes that enclosingIntRect/enclosedIntRect on pixels rounds to physical pixels. These are all systems that expect exactness and it would be a nightmare to make that work ok (or to fix bugs when it failed.) > > It would be helpful to have comments on LayerChromium.h describing what space various members are in, e.g. bounds() is in logical pixels, drawTransform() converts logical into physical pixels, transform() operates on logical pixels. Cool, I've added some comments to LayerChromium. Please let me know if I should add some more. > > I have a few worries that would make me feel better if you could investigate: What space is hit testing and input in? Do they need to be converted to physical pixel space? Are we correctly passing back scroll position from the compositor thread in logical pixel space? Hit testing and input are done in logical pixels, except for the impl thread hit testing which is done in physical pixels due to the scaling of the matrices in calcDrawTransforms. Scroll deltas appear to always be in logical pixels (looking at CCLTHI::scrollBy).
Created attachment 148224 [details] Patch Updated some scrollbar code that is affected by changing the meaning of contentBounds.
Comment on attachment 148224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148224&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.h:90 > + // A layer's transform operates layer space. That is, entirely in logical pixels. ...except for the NCCH? > Source/WebCore/platform/graphics/chromium/LayerChromium.h:113 > + // A layer's bounds are in logical pixels. However, the root layer's bounds > + // may be scaled by the page scale factor. Just to clarify--the NCCH layer is special because page scale is already applied to it? Does that mean that the NCCH's layer's bounds are in logical pixels and other layer bounds are in CSS pixels? (From the language here: https://trac.webkit.org/wiki/ScalesAndZooms) > Source/WebCore/platform/graphics/chromium/LayerChromium.h:244 > + // This moves from layer space, with origin in the center to target space with origin in the top left. > + // That is, it converts from logical to target pixels (and if the target is the root render surface, > + // then this converts to physical pixels). Why aren't target pixels physical pixels? > Source/WebCore/platform/graphics/chromium/LayerChromium.h:253 > + // The contentsScale is the ratio of the width of the contentBounds and bounds (likewise for height). This isn't true for image layers or anything that overrides contentBounds(). Isn't this really the conversion from CSS pixels to physical pixels (except for the NCCH?). Is there a better name than contentsScale? I think this is what jamesr was trying to get at in comment 11. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:783 > + WebTransformationMatrix replicaLayerTransform = layer->replicaLayer()->transform(); > + replicaLayerTransform.setM41(replicaLayerTransform.m41() * contentsScale); > + replicaLayerTransform.setM42(replicaLayerTransform.m42() * contentsScale); This math looks really dodgy. Can you reply to Shawn's comment #9?
Created attachment 148377 [details] Patch Not for review. Currently breaks many unit tests.
Created attachment 148434 [details] Patch Ready for review. The 'fix' for replica transforms with different anchor points didn't work. Nested reflections were busted. This patch now maintains the current behavior, and the reflection issue can be looked at separately. I've also reworked the math for computing the replica tranforms so that it's more principled.
Comment on attachment 148434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148434&action=review > Source/WebCore/platform/graphics/chromium/LayerChromium.h:91 > + // A layer's transform operates layer space. That is, entirely in logical pixels. > + // The root layer is a special case -- it operates in physical pixels. Oh, quite right. The root layer is in physical pixels. However, I think layers are in CSS pixels since their contents scale involves both page scale and device scale? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:597 > + surfaceOriginTransform.scale(1 / contentsScale); > + surfaceOriginTransform.translate3d(-0.5 * transformedLayerRect.width(), -0.5 * transformedLayerRect.height(), 0); Many patches later, I think I've finally grokked this. The combinedTransform operates on layer rects, but surfaceOriginTransform operates on surface rects which are now in physical pixels. So, to convert combinedTransform into surfaceOriginTransform you need to unconvert from physical pixels first. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:780 > + WebTransformationMatrix screenSpaceTransform = layer->screenSpaceTransform(); > + screenSpaceTransform.scale(1 / contentsScale); > + renderSurface->setScreenSpaceTransform(screenSpaceTransform); And I understand that it's the same thing here, in that Layer::screenSpaceTransform() operates on layer bounds which are not physical pixels, but Surface::screenSpaceTransform operates on surface bounds, which are now physical pixels. (Just talking out loud so you can correct me if I'm misunderstanding...) > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:797 > WebTransformationMatrix replicaDrawTransform = renderSurface->originTransform(); > - replicaDrawTransform.translate(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y()); > - replicaDrawTransform.multiply(layer->replicaLayer()->transform()); > - replicaDrawTransform.translate(surfaceCenter.x() - anchorPoint.x() * bounds.width(), surfaceCenter.y() - anchorPoint.y() * bounds.height()); > + replicaDrawTransform.translate(layer->replicaLayer()->position().x() * contentsScale, layer->replicaLayer()->position().y() * contentsScale); > + > + // The replica layer transform is in layer space, so we need to compensate to > + // get a matrix that operates in content space. > + WebTransformationMatrix replicaLayerTransform = layer->replicaLayer()->transform(); > + FloatPoint3D layerSpaceOrigin = replicaLayerTransform.mapPoint(FloatPoint3D()); > + FloatPoint3D contentSpaceOrigin = layerSpaceOrigin * contentsScale; > + FloatPoint3D originOffset = layerSpaceOrigin - contentSpaceOrigin; > + replicaLayerTransform.translate3d(originOffset.x(), originOffset.y(), originOffset.z()); > + > + replicaDrawTransform.multiply(replicaLayerTransform); > + replicaDrawTransform.translate(surfaceCenter.x() - anchorPoint.x() * bounds.width() * contentsScale, surfaceCenter.y() - anchorPoint.y() * bounds.height() * contentsScale); > + If a transform is in layer space, is it possible to just add a scale to convert to surface space or vice versa rather than explicitly putting contentsScale in every operation that follows? In other words something more like (insert vague hand waving maths): WebTransformationMatrix replicaDrawTransform = renderSurface->originTransform(); + replicaDrawTransform.scale(contentsScale); replicaDrawTransform.translate(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y()); replicaDrawTransform.multiply(layer->replicaLayer()->transform()); replicaDrawTransform.translate(surfaceCenter.x() - anchorPoint.x() * bounds.width(), surfaceCenter.y() - anchorPoint.y() * bounds.height()); + replicaDrawTransform.scale(1 / contentsScale); I also wonder if the replicaOriginTransform calculation just below this diff is incorrect, as it needs an additional conversion from physical pixels to replica layer space. And, the same thing with replicaScreenSpaceTransform, I think. Alternatively, surfaceOriginToReplicaOriginTransform needs to operate in surface physical pixel space rather than replica layer space.
Comment on attachment 148434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148434&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:797 >> + > > If a transform is in layer space, is it possible to just add a scale to convert to surface space or vice versa rather than explicitly putting contentsScale in every operation that follows? In other words something more like (insert vague hand waving maths): > > WebTransformationMatrix replicaDrawTransform = renderSurface->originTransform(); > + replicaDrawTransform.scale(contentsScale); > replicaDrawTransform.translate(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y()); > replicaDrawTransform.multiply(layer->replicaLayer()->transform()); > replicaDrawTransform.translate(surfaceCenter.x() - anchorPoint.x() * bounds.width(), surfaceCenter.y() - anchorPoint.y() * bounds.height()); > + replicaDrawTransform.scale(1 / contentsScale); > > I also wonder if the replicaOriginTransform calculation just below this diff is incorrect, as it needs an additional conversion from physical pixels to replica layer space. And, the same thing with replicaScreenSpaceTransform, I think. Alternatively, surfaceOriginToReplicaOriginTransform needs to operate in surface physical pixel space rather than replica layer space. And sure, surfaceCenter really needs to be layerSpaceSurfaceCenter, but the idea still holds, I think.
Created attachment 148484 [details] Patch (In reply to comment #32) > (From update of attachment 148434 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148434&action=review > > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:91 > > + // A layer's transform operates layer space. That is, entirely in logical pixels. > > + // The root layer is a special case -- it operates in physical pixels. > > Oh, quite right. The root layer is in physical pixels. However, I think layers are in CSS pixels since their contents scale involves both page scale and device scale? > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:597 > > + surfaceOriginTransform.scale(1 / contentsScale); > > + surfaceOriginTransform.translate3d(-0.5 * transformedLayerRect.width(), -0.5 * transformedLayerRect.height(), 0); > > Many patches later, I think I've finally grokked this. The combinedTransform operates on layer rects, but surfaceOriginTransform operates on surface rects which are now in physical pixels. So, to convert combinedTransform into surfaceOriginTransform you need to unconvert from physical pixels first. Yes, exactly. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:780 > > + WebTransformationMatrix screenSpaceTransform = layer->screenSpaceTransform(); > > + screenSpaceTransform.scale(1 / contentsScale); > > + renderSurface->setScreenSpaceTransform(screenSpaceTransform); > > And I understand that it's the same thing here, in that Layer::screenSpaceTransform() operates on layer bounds which are not physical pixels, but Surface::screenSpaceTransform operates on surface bounds, which are now physical pixels. (Just talking out loud so you can correct me if I'm misunderstanding...) Yep. This seems helpful to note. I've added a comment. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:797 > > WebTransformationMatrix replicaDrawTransform = renderSurface->originTransform(); > > - replicaDrawTransform.translate(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y()); > > - replicaDrawTransform.multiply(layer->replicaLayer()->transform()); > > - replicaDrawTransform.translate(surfaceCenter.x() - anchorPoint.x() * bounds.width(), surfaceCenter.y() - anchorPoint.y() * bounds.height()); > > + replicaDrawTransform.translate(layer->replicaLayer()->position().x() * contentsScale, layer->replicaLayer()->position().y() * contentsScale); > > + > > + // The replica layer transform is in layer space, so we need to compensate to > > + // get a matrix that operates in content space. > > + WebTransformationMatrix replicaLayerTransform = layer->replicaLayer()->transform(); > > + FloatPoint3D layerSpaceOrigin = replicaLayerTransform.mapPoint(FloatPoint3D()); > > + FloatPoint3D contentSpaceOrigin = layerSpaceOrigin * contentsScale; > > + FloatPoint3D originOffset = layerSpaceOrigin - contentSpaceOrigin; > > + replicaLayerTransform.translate3d(originOffset.x(), originOffset.y(), originOffset.z()); > > + > > + replicaDrawTransform.multiply(replicaLayerTransform); > > + replicaDrawTransform.translate(surfaceCenter.x() - anchorPoint.x() * bounds.width() * contentsScale, surfaceCenter.y() - anchorPoint.y() * bounds.height() * contentsScale); > > + > > If a transform is in layer space, is it possible to just add a scale to convert to surface space or vice versa rather than explicitly putting contentsScale in every operation that follows? In other words something more like (insert vague hand waving maths): > > WebTransformationMatrix replicaDrawTransform = renderSurface->originTransform(); > + replicaDrawTransform.scale(contentsScale); > replicaDrawTransform.translate(layer->replicaLayer()->position().x(), layer->replicaLayer()->position().y()); > replicaDrawTransform.multiply(layer->replicaLayer()->transform()); > replicaDrawTransform.translate(surfaceCenter.x() - anchorPoint.x() * bounds.width(), surfaceCenter.y() - anchorPoint.y() * bounds.height()); > + replicaDrawTransform.scale(1 / contentsScale); > > I also wonder if the replicaOriginTransform calculation just below this diff is incorrect, as it needs an additional conversion from physical pixels to replica layer space. And, the same thing with replicaScreenSpaceTransform, I think. Alternatively, surfaceOriginToReplicaOriginTransform needs to operate in surface physical pixel space rather than replica layer space. This worked beautifully using the layerSpaceSurfaceCenter, thanks!
Comment on attachment 148484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148484&action=review What about my previous comment #32, re: surfaceOriginToReplicaOriginTransform? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:794 > + FloatPoint3D layerSpaceSurfaceCenter = surfaceCenter * (1 / contentsScale); FloatPoint3D => FloatPoint?
Created attachment 148495 [details] Patch (In reply to comment #35) > (From update of attachment 148484 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=148484&action=review > > What about my previous comment #32, re: surfaceOriginToReplicaOriginTransform? Sorry, I missed this. You're completely right, there was a scale missing. This should be fixed in the current patch. Confirmed with --show-composited-layer-borders and checked in the unit test. > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:794 > > + FloatPoint3D layerSpaceSurfaceCenter = surfaceCenter * (1 / contentsScale); > > FloatPoint3D => FloatPoint? Done.
Comment on attachment 148495 [details] Patch R=me. Thanks for bearing with all the changes. :)
Comment on attachment 148434 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148434&action=review >>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:91 >>> + // The root layer is a special case -- it operates in physical pixels. >> >> Oh, quite right. The root layer is in physical pixels. However, I think layers are in CSS pixels since their contents scale involves both page scale and device scale? > > Yes, exactly. I think that since the pixels include page zoom they would be logical pixels under zoom, but CSS pixels wrt scale? Need to fix page scale.. According to https://trac.webkit.org/wiki/ScalesAndZooms I think page scale /should/ be (but isn't) changing these bounds the same as page zoom would, making these bounds logical pixels.
Created attachment 148571 [details] Patch Sadly, it looks like we need to go one more round (thanks for your patience with this). As Dana was mentioning, bounds aren't quite in css pixels. They do have page zoom baked in (but not page scale), so they're logical pixels. I've confirmed this -- as I change the page scale, the viewport's bounds are the same but, the bounds of a layer or render surface do vary. I've updated the comments in LayerChromium and replaced instances of 'css pixels' with 'logical, non-page-scaled pixels'. There's also a more detailed explanation of the term where I first use it.
Comment on attachment 148571 [details] Patch Sounds good. R=me.
Comment on attachment 148571 [details] Patch Clearing flags on attachment: 148571 Committed r120837: <http://trac.webkit.org/changeset/120837>
All reviewed patches have been landed. Closing bug.