Bug 93919 – [chromium] double-check high-DPI correctness when skipping layer2content scaling due to empty bounds

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Shawn Singh]

There are a few concerns about a few lines of code in CCLayerTreeHostCommon.cpp, where the drawTransform is scaled by layer2content transform.   Particularly, when in high DPI mode is when this transform matters:

(1) is it correct to skip it entirely on empty but non-zero layer bounds?  (i.e. the width scale may get skipped just because height is zero?)
(2) how does deviceScaleFactor come into play, versus directly computing the scale via (bounds/contentBounds)?

There's a chance something is wrong here in high DPI mode.  if thre is something wrong, it might show up when a renderSurface needed this high-DPI scaling when it was created by an empty but non-zero bounds layer.

[Shawn Singh]

just FYI if it matters, this bug stemmed from the high DPI questions on https://bugs.webkit.org/show_bug.cgi?id=93795

I'm looking at lines 548 552 of CCLTHCommon:

layerDrawTransform.scale(deviceScaleFactor);
if (!layer->contentBounds().isEmpty() && !layer->bounds().isEmpty()) {
    layerDrawTransform.scaleNonUniform(layer->bounds().width() / static_cast<double>(layer->contentBounds().width()),
    layer->bounds().height() / static_cast<double>(layer->contentBounds().height()));
}

This definitely looks fishy to me. First, I don't think we should ever scale by the device scale factor -- the scale needs to be non-uniform and based on the ratio of content to layer bounds. Second, if I'm understanding this code correctly, the scale(...) scales the layer up, and the scaleNonUniform(...) _sometimes_ scales the layer back down (by slightly different amounts). So in the common case, we get (almost) the identity transform, and when something goes wrong (that is, we have empty bounds) we get a scaled up matrix, which seems wrong. I understand why the scaleNonUniform(...) is conditional -- we can't scale by 0, nor can we divide by 0 -- but maybe we can eliminate this code altogether if the goal is to end up with an identity transform? (Things looked ok in high-DPI on gmail and poster circle with it gone).

[Shawn Singh]

Here is a detailed description of the math that the code implements to compute draw transforms and sublayer matrices:

layerLocalTranform = pageScaleDelta * parent2origin * origin2anchor * layerTransform * anchor2center
combined transform = parentMatrix * layerLocalTransform


NEW CODE:

If layer has a renderSurface:
  - renderSurface drawTransform:
    parentMatrix * (pageScaleDelta * parent2origin * origin2anchor * layerTransform * anchor2center) * center2origin * layerSpace2contentSpace * (origin2center moved to LRC)

  - layer drawTransform:
    deviceScaleFactor * layerSpace2contentSpace * (origin2center moved to LRC)

  - sublayerMatrix:
    (deviceScaleFactor * origin2center) * pseudoOrthographic * sublayerTransform * center2origin

If layer does not have surface:
  - layer drawTransform:
    parentMatrix * (pageScaleDelta * parent2origin * origin2anchor * layerTransform * anchor2center) * center2origin * layerSpace2contentSpace * (origin2center moved to LRC)

  - sublayerMatrix:
    parentMatrix * (pageScaleDelta * parent2origin * origin2anchor * layerTransform * anchor2center) * pseudoOrthographic * sublayerTransform * center2origin



OLD CODE that is probably in good condition, but has the disadvantage of not supporting high DPI deviceScaleFactor for renderSurfaces:

If layer has a renderSurface:
  - renderSurface drawTransform:
    parentMatrix * (pageScaleDelta * parent2origin * origin2anchor * layerTransform * anchor2center) * 1/contentsScale * center2origin * (origin2center * center2ClippedCenter)

  - layer drawTransform:
    contentsScale * origin2center

  - sublayerMatrix:
    (contentsScale * origin2center) * pseudoOrthographic * sublayerTransform * center2origin

If layer does not have surface:
  - layer drawTransform:
    parentMatrix * (pageScaleDelta * parent2origin * origin2anchor * layerTransform * anchor2center)

  - sublayerMatrix:
    parentMatrix * (pageScaleDelta * parent2origin * origin2anchor * layerTransform * anchor2center) * pseudoOrthographic * sublayerTransform * center2origin

[Dana Jansens]

(In reply to comment #0)
> (2) how does deviceScaleFactor come into play, versus directly computing the scale via (bounds/contentBounds)?

To clarify:
- bounds/contentBounds is always used for moving between layer space and layer content space.
- deviceScaleFactor is used when moving from layer space to the layer's target space.

[Shawn Singh]

this bug moved to https://code.google.com/p/chromium/issues/detail?id=230055