Bug 93919 - [chromium] double-check high-DPI correctness when skipping layer2content scaling due to empty bounds
Summary: [chromium] double-check high-DPI correctness when skipping layer2content scal...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-13 17:57 PDT by Shawn Singh
Modified: 2013-04-10 13:43 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-08-13 17:57:27 PDT
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.
Comment 1 Shawn Singh 2012-08-13 18:00:05 PDT
just FYI if it matters, this bug stemmed from the high DPI questions on https://bugs.webkit.org/show_bug.cgi?id=93795
Comment 2 vollick 2012-08-14 06:46:22 PDT
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).
Comment 3 Shawn Singh 2012-08-14 10:11:05 PDT
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
Comment 4 Dana Jansens 2012-08-15 10:34:20 PDT
(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.
Comment 5 Shawn Singh 2013-04-10 13:43:22 PDT
this bug moved to https://code.google.com/p/chromium/issues/detail?id=230055