Bug 71225 - [chromium] composited layers are blurry with a zoom-in page scale factor
Summary: [chromium] composited layers are blurry with a zoom-in page scale factor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hin-Chung Lam
URL:
Keywords:
Depends on: 72778
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-31 09:55 PDT by Hin-Chung Lam
Modified: 2012-03-20 18:59 PDT (History)
13 users (show)

See Also:


Attachments
layout test (369 bytes, text/html)
2011-10-31 09:55 PDT, Hin-Chung Lam
no flags Details
good resolution (3.96 KB, image/png)
2011-10-31 09:56 PDT, Hin-Chung Lam
no flags Details
bad resolution (6.37 KB, image/png)
2011-10-31 09:56 PDT, Hin-Chung Lam
no flags Details
test case for page scale and position:fixed (479 bytes, text/html)
2011-11-02 13:46 PDT, Hin-Chung Lam
no flags Details
work in progress 1 (17.04 KB, patch)
2011-11-02 13:57 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
ignore this patch (5.47 KB, patch)
2011-11-03 06:25 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
work in progress 2 (13.14 KB, patch)
2011-11-03 08:51 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (35.75 KB, patch)
2011-11-03 10:00 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (44.77 KB, patch)
2011-11-04 09:00 PDT, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (42.52 KB, patch)
2011-11-07 09:22 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (36.00 KB, patch)
2011-11-11 12:52 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (36.50 KB, patch)
2011-11-14 10:57 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (55.88 KB, patch)
2011-11-15 04:53 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (55.96 KB, patch)
2011-11-15 14:22 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Fixed merge conflicts (77.05 KB, patch)
2011-11-18 08:57 PST, Iain Merrick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2011-10-31 09:55:36 PDT
Created attachment 113061 [details]
layout test

See the supplied test. Click on good.png which is when compositor is disabled. Click on bad.png which compositor is enabled and position:fixed is put on a separate layer.
Comment 1 Hin-Chung Lam 2011-10-31 09:56:09 PDT
Created attachment 113063 [details]
good resolution
Comment 2 Hin-Chung Lam 2011-10-31 09:56:27 PDT
Created attachment 113064 [details]
bad resolution
Comment 3 James Robinson 2011-10-31 17:35:40 PDT
I think your test case is the same as this:

<!DOCTYPE html>
<style>
.scaleUp {
  -webkit-transform:scale(2, 2);
  position:absolute;
  left:50px;
}
.compositedScaleUp {
  -webkit-transform:translateZ(0) scale(2,2);
  position:absolute;
  left:50px;
}
</style>
<div class="scaleUp">TEST 1</div>
<br><br>
<div class="compositedScaleUp">TEST 2</div>

which boils down to rastering at the pre-scale resolution rather than post-scale.
Comment 4 James Robinson 2011-11-01 13:41:34 PDT
I think we need to port http://trac.webkit.org/changeset/75639 (and http://trac.webkit.org/changeset/91137) to our compositor implementation.
Comment 5 Hin-Chung Lam 2011-11-02 13:46:07 PDT
Created attachment 113369 [details]
test case for page scale and position:fixed

James: The WebKit patches you pointed out only deals with page scale but not general css transform. I think we should limit the scope of this to just have better resolution for css transform caused by page scale. I've uploaded a test case to demonstrate this problem.
Comment 6 Hin-Chung Lam 2011-11-02 13:57:13 PDT
Created attachment 113374 [details]
work in progress 1

James: I also took your advice to look into how WebKit implements, please check the attached patch to see if this is the right approach to take. There are several things I don't understand so I added comments to the code:

1. GraphicsLayer::setAppliesPageScale() is called on GraphicsLayer associated to the render view. I don't know what this actually means because the layer transformation passed to the GraphicsLayer doesn't contain the scale transform. Actual painting of this layer goes through RenderLayer::paintLayer() which has transform applied to the painter.

2. It seems in RenderStlye::setPageScaleTransform() the page scale transform is applied as a CSS scale transform to be inherited by all objects. I observed that GraphicsLayer for render view doesn't have scale transform applied, but GraphicsLayer for position:fixed has scale transform applied. I had to counter-act this scale transform in GraphicsLayerChromium for position:fixed.... I'm sure there's a better way to do this..
Comment 7 James Robinson 2011-11-02 14:15:54 PDT
Comment on attachment 113374 [details]
work in progress 1

View in context: https://bugs.webkit.org/attachment.cgi?id=113374&action=review

> Source/WebCore/platform/graphics/GraphicsLayer.h:-468
> -

please don't make spurious changes to files like this. i think your editor is probably doing this, fix it

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:197
> +    float contentsScale() const { return m_contentsScale; }
> +    void setContentsScale(float contentsScale) { m_contentsScale = contentsScale; }

we already support the ability for layer bounds != content bounds via contentBounds()/bounds(). is it necessary to introduce another mechanism?

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:154
> +    canvasPainter.context()->scale(FloatSize(contentsScale, contentsScale));

if you're scaling the canvas you need to adjust the size of the allocated canvas as well, right? where is that being done?

> Source/WebKit/chromium/src/PageOverlay.cpp:81
> +    virtual float deviceScaleFactor() const
> +    {
> +        return 1;
> +    }
> +
> +    virtual float pageScaleFactor() const
> +    {
> +        return m_webViewImpl->pageScaleFactor();
> +    }

i don't understand this change at all. are you using PageOverlay?
Comment 8 Hin-Chung Lam 2011-11-02 14:26:08 PDT
Comment on attachment 113374 [details]
work in progress 1

View in context: https://bugs.webkit.org/attachment.cgi?id=113374&action=review

>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:197
>> +    void setContentsScale(float contentsScale) { m_contentsScale = contentsScale; }
> 
> we already support the ability for layer bounds != content bounds via contentBounds()/bounds(). is it necessary to introduce another mechanism?

This is used in the painter of texture updater to apply scale to the canvas.

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:154
>> +    canvasPainter.context()->scale(FloatSize(contentsScale, contentsScale));
> 
> if you're scaling the canvas you need to adjust the size of the allocated canvas as well, right? where is that being done?

I thought setting the bounds of LayerChromium will changes the size of canvas, I'll check again.

> Source/WebCore/rendering/RenderLayer.cpp:3846
> +    bool paintsForFixedPositionLayer = renderer()->isPositioned() && renderer()->style()->position() == FixedPosition;

Don't mind changes in this file, it shouldn't be here..

>> Source/WebKit/chromium/src/PageOverlay.cpp:81
>> +    }
> 
> i don't understand this change at all. are you using PageOverlay?

deviceScaleFactor and pageScaleFactor are methods in GraphicsLayerClient. This change http://trac.webkit.org/changeset/91137 queries client for these scale factors, where is a better spot for implementing these two methods?
Comment 9 James Robinson 2011-11-02 14:32:58 PDT
I see, so are the layer bounds pre-scale or post-scale?
Comment 10 Hin-Chung Lam 2011-11-02 14:34:11 PDT
(In reply to comment #9)
> I see, so are the layer bounds pre-scale or post-scale?

Layer bounds are post-scale.
Comment 11 James Robinson 2011-11-02 14:36:08 PDT
The way I was envisioning this is that layer bounds are pre-scale but the content bounds are post-scale and we raster with the scale applied into a post-scale canvas, then take care of the draw transform such that it displays at the right size.
Comment 12 Vangelis Kokkevis 2011-11-02 15:10:11 PDT
Comment on attachment 113374 [details]
work in progress 1

View in context: https://bugs.webkit.org/attachment.cgi?id=113374&action=review

I'm not sure I understand why we have to be applying the scale in all these places.  It seems to me that when the page is scaled there will be a scale transform at the top of the tree that will affect the positions and bounds of all layers.  Only when it comes time to paint the contents of an individual layer do we have to scale the canvas by the contentsScale and adjust the draw transform by 1/contentsScale to make sure it renders correctly.

That should work for all composited layers, not just the fixed position ones.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:509
> +    IntSize layerSize(m_size.width() * contentsScaleFactor, m_size.height() * contentsScaleFactor);

If the whole page is scaled via a scale transform at the very top of the tree, then you shouldn't have to worry about accommodating for scales down here.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:530
> +    primaryLayer()->setAnchorPointZ(m_anchorPoint.z() * contentsScaleFactor);

Not sure you need that.  You're not adjusting the x and y either...

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:541
> +    if (appliesPageScale()) {

style: no braces

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:542
> +        layerTransform = TransformationMatrix().scale(1 / contentsScale()) * m_transform;

Should be 1.0 to force a float division ? 

Also, the scale probably needs to be applied to all 3d axes so I think you need: 
layerTransform.scale3d(1/cs, 1/cs, 1/cs);

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:593
> +        m_layer->setContentsScale(contentsScale());

Isn't the scale going to be set by updateContentsScale() above?

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:191
>      PlatformContextSkia platformContext(canvas);

I believe you'll need to apply the scale here too.

> Source/WebCore/rendering/RenderLayer.cpp:3851
> +    return transform() && ((paintBehavior & PaintBehaviorFlattenCompositingLayers) || paintsToWindow || paintsForFixedPositionLayer);

I don't think you want to be special-casing fixed position elements...
Comment 13 Hin-Chung Lam 2011-11-03 06:25:55 PDT
Created attachment 113474 [details]
ignore this patch
Comment 14 Hin-Chung Lam 2011-11-03 08:51:00 PDT
Created attachment 113499 [details]
work in progress 2

So what I did in this patch:
1. Bump up the contentBounds() using the idea from James.
2. Apply scale transform in the painter.

This makes it much cleaner and not have to mess around with layer transform.. I'm working on some layout tests to make sure this works.
Comment 15 Hin-Chung Lam 2011-11-03 10:00:52 PDT
Created attachment 113511 [details]
Patch
Comment 16 Hin-Chung Lam 2011-11-03 10:05:29 PDT
I've updated the patch with some tests, please take another look.
Comment 17 James Robinson 2011-11-03 10:21:04 PDT
Comment on attachment 113511 [details]
Patch

This looks pretty nice!
Comment 18 Hin-Chung Lam 2011-11-03 10:28:10 PDT
Cool! Let's do a full review.
Comment 19 Gustavo Noronha (kov) 2011-11-03 13:17:35 PDT
Comment on attachment 113511 [details]
Patch

Attachment 113511 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10249412
Comment 20 Daniel Sievers 2011-11-03 15:03:56 PDT
Does this use the wrong (post-scale contentRect) when calling into webkit below?
  paintContents(*canvasPainter.context(), contentRect);

I guess with a scale smaller than 1 you could potentially test if this misses updating anything.

Interestingly, since before your patch contentBounds() and bounds() are the same, I think we end up scaling at draw time simply through the projection from layer to target render surface, i.e.:
    IntRect layerRect = surfaceToLayer.projectQuad(FloatQuad(FloatRect(minimalSurfaceRect))).enclosingBoundingBox();
Comment 21 James Robinson 2011-11-03 15:11:39 PDT
contentBounds != bounds today for scaled images, which are scaled at draw time. http://trac.webkit.org/browser/trunk/LayoutTests/compositing/color-matching/image-color-matching.html for example
Comment 22 James Robinson 2011-11-03 23:52:41 PDT
Comment on attachment 113511 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113511&action=review

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195
> +    updateContentsScale();

is there a specific reason you're doing this before the transform == m_transform check? do you need to run this code even if the transform hasn't changed?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:651
> +    // If page scale is applied as a transform in the compositor then there's nothing to do here.

not sure i grok this comment. looking at the code, it appears this bit is set on GraphicsLayers for RenderViews, which would include the main layer and iframes below it

have you tested scaled elements inside iframes, etc?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:655
> +    m_layer->setContentsScale(contentsScale());

should you be setting this on the m_contentsLayer?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:657
> +        m_layer->setNeedsDisplay();

hmm, this appears to invalidate the full layer every time this function is called even if nothing has changed - which would be bad.

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:137
> +IntSize TiledLayerChromium::contentBounds() const

ImageLayerChromium is a subclass of this and provides an implementation of contentBounds() as well. Will this do the right thing for those layers?
Comment 23 James Robinson 2011-11-03 23:53:19 PDT
there's nothing in this patch specific to fixed position elements that I can see - can you please retitle the bug to capture its scope?
Comment 24 Hin-Chung Lam 2011-11-04 09:00:54 PDT
Created attachment 113663 [details]
Patch
Comment 25 Hin-Chung Lam 2011-11-04 09:04:16 PDT
Comment on attachment 113663 [details]
Patch

I've added one more test to cover position:fixed in iframe.

I also added a virtual method to LayerChromium to determine if the layer needs content scaling. For example doing content scaling with ImageLayerChromium is bad because it's slower to do it in software, but for TiledLayerChromium for regular content content scaling is needed for the best quality.
Comment 26 Vangelis Kokkevis 2011-11-04 13:45:24 PDT
Comment on attachment 113663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113663&action=review

> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:64
> +    virtual void prepareToUpdate(const IntRect& contentRect, const IntSize& tileSize, int /* borderTexels */, float contentsScale)

You'll probably need to add contentsScale in /* */ comments to avoid compiler warnings about unused variable.

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:153
> +    canvasPainter.context()->save();

Adding a check for a scale of 1.0 would help avoid unnecessary saves and restores.

> LayoutTests/ChangeLog:6
> +        Added test cases that has position:fixed and page scale.

I would recommend getting rid of all mentions of fixed position in the Changelogs since this fix applies to all tiled layer types.  You can also make the layout tests independent of fixed position

> LayoutTests/platform/chromium-linux/compositing/geometry/fixed-position-transform-composited-page-scale-expected.txt:1
> +layer at (0,0) size 785x585

You'll need to change test_expectations.txt to mark win and mac as IMAGE until you can harvest results from the bots.
Comment 27 James Robinson 2011-11-04 16:47:51 PDT
Comment on attachment 113663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113663&action=review

I had feedback to previous patch that wasn't addressed.

> Source/WebCore/ChangeLog:3
> +        [chromium] when position:fixed in a separate layer it doesn't get full resolution with css transform

please update the changelog title to reflect the bug title

> Source/WebKit/chromium/ChangeLog:3
> +        [chromium] when position:fixed in a separate layer it doesn't get full resolution with css transform

same here

> LayoutTests/compositing/geometry/fixed-position-composited-page-scale.html:1
> +<html>

<!DOCTYPE html> before this please

please do not add new pixel results that include scrollbars since that makes the results platform-specific
Comment 28 Hin-Chung Lam 2011-11-07 09:20:19 PST
(In reply to comment #22)
> (From update of attachment 113511 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113511&action=review
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195
> > +    updateContentsScale();
> 
> is there a specific reason you're doing this before the transform == m_transform check? do you need to run this code even if the transform hasn't changed?\

Yes I do need to do this even if transform == m_transform as the content scale may change. 

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:651
> > +    // If page scale is applied as a transform in the compositor then there's nothing to do here.
> 
> not sure i grok this comment. looking at the code, it appears this bit is set on GraphicsLayers for RenderViews, which would include the main layer and iframes below it
> 
> have you tested scaled elements inside iframes, etc?

I added a test for this case.

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:655
> > +    m_layer->setContentsScale(contentsScale());
> 
> should you be setting this on the m_contentsLayer?

I tried with m_contentsLayer but it didn't fix the problem. I'm a bit confused with m_layer and m_contentsLayer.. what are the differences?

> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:657
> > +        m_layer->setNeedsDisplay();
> 
> hmm, this appears to invalidate the full layer every time this function is called even if nothing has changed - which would be bad.

Removed this.

> 
> > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:137
> > +IntSize TiledLayerChromium::contentBounds() const
> 
> ImageLayerChromium is a subclass of this and provides an implementation of contentBounds() as well. Will this do the right thing for those layers?

Addressed in latest patch.
Comment 29 Hin-Chung Lam 2011-11-07 09:22:14 PST
Created attachment 113880 [details]
Patch
Comment 30 Hin-Chung Lam 2011-11-07 09:24:14 PST
Comment on attachment 113663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113663&action=review

>> Source/WebCore/ChangeLog:3
>> +        [chromium] when position:fixed in a separate layer it doesn't get full resolution with css transform
> 
> please update the changelog title to reflect the bug title

done.

>> Source/WebKit/chromium/ChangeLog:3
>> +        [chromium] when position:fixed in a separate layer it doesn't get full resolution with css transform
> 
> same here

done.

>> Source/WebCore/platform/graphics/chromium/ImageLayerChromium.cpp:64
>> +    virtual void prepareToUpdate(const IntRect& contentRect, const IntSize& tileSize, int /* borderTexels */, float contentsScale)
> 
> You'll probably need to add contentsScale in /* */ comments to avoid compiler warnings about unused variable.

done.

>> Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.cpp:153
>> +    canvasPainter.context()->save();
> 
> Adding a check for a scale of 1.0 would help avoid unnecessary saves and restores.

done.

>> LayoutTests/ChangeLog:6
>> +        Added test cases that has position:fixed and page scale.
> 
> I would recommend getting rid of all mentions of fixed position in the Changelogs since this fix applies to all tiled layer types.  You can also make the layout tests independent of fixed position

done.

>> LayoutTests/compositing/geometry/fixed-position-composited-page-scale.html:1
>> +<html>
> 
> <!DOCTYPE html> before this please
> 
> please do not add new pixel results that include scrollbars since that makes the results platform-specific

done.
Comment 31 James Robinson 2011-11-07 17:30:18 PST
Comment on attachment 113880 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=113880&action=review

Nearly there!

> Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:144
> +    return IntSize(bounds().width() * contentsScale(), bounds().height() * contentsScale());

Are you sure this does the rounding we want?

> LayoutTests/ChangeLog:6
> +        Added test cases that has position:fixed and page scale.

since you are only adding cr-linux expectations, you need to add = MISSING expectations for windows and mac or the bots will go red when this lands.

> LayoutTests/compositing/geometry/fixed-position-composited-page-scale.html:23
> +      eventSender.scalePageBy(2, 0, 0);

could you add a few tests with a scale < 1?

> LayoutTests/platform/chromium-linux/compositing/geometry/fixed-position-composited-page-scale-expected.txt:2
> +layer at (0,0) size 800x585
> +  RenderView at (0,0) size 800x585

do you need the render tree dump here, or is the test just about checking the pixels? If the latter add this incantation to the test:

layoutTestController.dumpAsText(true);

so you don't need this stuff
Comment 32 Hin-Chung Lam 2011-11-09 10:32:54 PST
I found that this patch doesn't work with page scale factor less than 1.0.. :(

The problem can be illustrated by two separate cases:

1. Painting the root layer
   This path goes through RenderLayer::paintLayer. paintDirtyRect passed to this method is post-scaled and this method map the rect to a pre-scaled one at this line: http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp&type=cs&l=2721.

2. Painting a non-root tiled layer rect
   Painter of the layer goes through RenderLayerBacking::paintIntoLayer. The dirty rect passed to RenderLayerBacking needs to be pre-scaled because it doesn't handle transformation. However contentRect passed to LayerTextureUpdaterBitmap::prepareToUpdate is post-scaled so only part of the layer content is painted.

The sad part is that LayerTextureUpdaterBitmap::prepareToUpdate handles both cases, but a post-scaled dirt rect works for case (1) but not (2).

I feel that the proper way is to merge RenderLayer::paintLayer and RenderLayerBacking::paintIntoLayer, it is crazy to transform dirty rect in one case but not the other.. Suggestions?
Comment 33 Daniel Sievers 2011-11-09 11:10:24 PST
(In reply to comment #32)
> I found that this patch doesn't work with page scale factor less than 1.0.. :(
> 

Also see my comment #20 above. Isn't it possible to simply pass the pre-scaled rect to the paint routine, since webkit doesn't know or need to care about how we scale this for composited layers?
Comment 34 Hin-Chung Lam 2011-11-09 11:45:24 PST
(In reply to comment #33)
> (In reply to comment #32)
> > I found that this patch doesn't work with page scale factor less than 1.0.. :(
> > 
> 
> Also see my comment #20 above. Isn't it possible to simply pass the pre-scaled rect to the paint routine, since webkit doesn't know or need to care about how we scale this for composited layers?

It may be possible but we'll need to make sure WebKit in RenderLayer::paintLayer doesn't scale the rect up otherwise there'll be over painting.
Comment 35 Alexandre Elias 2011-11-10 09:53:28 PST
Minor comment: WebViewImpl has a newly added deviceScaleFactor() method, so you might want to call that from PageOverlay.

As for the sub-1.0 problem, merging the root layer/sublayer draw paths sounds like a good idea in principle, but if that proves hard for whatever reason, then it should be possible to hack the bounds sent to WebKit like Daniel says.
Comment 36 Hin-Chung Lam 2011-11-11 12:52:50 PST
Created attachment 114753 [details]
Patch
Comment 37 Hin-Chung Lam 2011-11-11 12:58:09 PST
Comment on attachment 114753 [details]
Patch

I've updated the code with changes included in our discussion. This patch doesn't include pixel test results yet because DumpRenderTree with page scaling is currently broken but I've tested this patch locally. I'm look to see what's wrong with DumpRenderTree now..
Comment 38 WebKit Review Bot 2011-11-11 13:01:08 PST
Attachment 114753 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1

Last 3072 characters of output:
atabase-interactions.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3863:  Path does not exist. fast/js/array-functions-non-arrays.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3864:  Path does not exist. fast/js/eval-cross-window.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3865:  Path does not exist. fast/js/regexp-caching.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3866:  Path does not exist. fast/js/toString-overrides.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3867:  Path does not exist. fast/js/toString-recursion.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3868:  Path does not exist. http/tests/security/xss-eval.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3870:  Path does not exist. fast/dom/javascript-url-exception-isolation.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3871:  Path does not exist. fast/borders/inline-mask-overlay-image-outset-vertical-rl.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3873:  Path does not exist. fast/dom/rtl-scroll-to-leftmost-and-resize.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3875:  Path does not exist. fast/canvas/canvas-transforms-fillRect-shadow.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3877:  Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3878:  Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3880:  Path does not exist. media/track/tracklist-is-reachable.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3883:  Path does not exist. compositing/geometry/fixed-position-composited-page-scale.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3884:  Path does not exist. compositing/geometry/fixed-position-iframe-composited-page-scale.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3885:  Path does not exist. compositing/geometry/fixed-position-transform-composited-page-scale.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3886:  Path does not exist. compositing/geometry/fixed-position-composited-page-scale-down.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3887:  Path does not exist. compositing/geometry/fixed-position-iframe-composited-page-scale-down.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3888:  Path does not exist. compositing/geometry/fixed-position-transform-composited-page-scale-down.html  [test/expectations] [2]
Total errors found: 2138 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 39 Hin-Chung Lam 2011-11-14 10:57:47 PST
Created attachment 114983 [details]
Patch
Comment 40 Hin-Chung Lam 2011-11-14 10:58:39 PST
The latest patch takes care of scale factor less than 1.0 and rounding error that creates empty gaps between tiles.
Comment 41 WebKit Review Bot 2011-11-14 11:00:54 PST
Attachment 114983 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1

Last 3072 characters of output:
/chromium/test_expectations.txt:3860:  Path does not exist. accessibility/adjacent-continuations-cause-assertion-failure.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3862:  Path does not exist. inspector/debugger/script-formatter.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3865:  Path does not exist. fast/js/mozilla/strict/15.4.5.1.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3867:  Path does not exist. fast/canvas/canvas-transforms-fillRect-shadow.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3869:  Path does not exist. media/track/tracklist-is-reachable.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3870:  Path does not exist. media/track/track-cues-cuechange.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3872:  Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3873:  Path does not exist. http/tests/inspector-enabled/dedicated-workers-list.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3875:  Path does not exist. http/tests/security/cross-frame-access-custom.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3877:  Path does not exist. fast/loader/javascript-url-in-embed.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3879:  Path does not exist. security/crypto-random-values-types.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3881:  Path does not exist. media/track/track-webvtt-tc004-magic-header.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3883:  Path does not exist. http/tests/inspector/resource-tree/resource-tree-frame-add.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3886:  Path does not exist. compositing/geometry/fixed-position-composited-page-scale.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3887:  Path does not exist. compositing/geometry/fixed-position-iframe-composited-page-scale.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3888:  Path does not exist. compositing/geometry/fixed-position-transform-composited-page-scale.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3889:  Path does not exist. compositing/geometry/fixed-position-composited-page-scale-down.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3890:  Path does not exist. compositing/geometry/fixed-position-iframe-composited-page-scale-down.html  [test/expectations] [2]
LayoutTests/platform/chromium/test_expectations.txt:3891:  Path does not exist. compositing/geometry/fixed-position-transform-composited-page-scale-down.html  [test/expectations] [2]
Total errors found: 2136 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 42 Hin-Chung Lam 2011-11-14 13:39:29 PST
Please ignore the errors for path does not exist. Eric has a patch to fix that.
Comment 43 James Robinson 2011-11-14 15:08:05 PST
Comment on attachment 114983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114983&action=review

This seems very close - the contentScale updating seems a bit off but everything else seems quite nice.  Could you please upload the .pngs for at least one platform? That's very useful for the gardener or whoever rebaselines the tests (assuming it isn't you) to have a baseline expectation of what the test should look like so they can eyeball if it's failing horribly on the other platforms or not.

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:63
> +            int x = floor(contentRect.x() * invScale);
> +            int y = floor(contentRect.y() * invScale);

doing all this math component-wise is pretty verbose and runs the risk of X/Y confusion errors that can be a pain to debug. Can you try to find the correct combination of (Int|Rect)/(Rect|Size) methods to do this?  I think it's something like:

scaledContentRect = FloatRect(contentRect).scale(invScale).enclosingIntRect();

or something along those lines (don't take my word for it, please test)

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195
> +    // Call this method first to assign contents scale to LayerChromium so the painter can apply the scale transform.
> +    updateContentsScale();

this still seems weird - setting the shouldn't change the contents scale.  contentsScale() appears to be a function of appliesPageScale(), pageScaleFactor(), and deviceScaleFactor(). Can we recalculate the scale factor when those things change, and not when setting the transform? we have a deviceOrPageScaleChanged() function already that seems like a good place to update this

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:600
> +        updateContentsScale();

this call also confuses me a bit - why do we need to update the contentsScale here when updating the transform hierarchy? we aren't changing the pageScale or deviceScale here

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:687
> +    return 1.0f;

nit: for constants like this in WebKit we typically just say '1'
Comment 44 Hin-Chung Lam 2011-11-15 04:51:34 PST
Comment on attachment 114983 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114983&action=review

>> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:63
>> +            int y = floor(contentRect.y() * invScale);
> 
> doing all this math component-wise is pretty verbose and runs the risk of X/Y confusion errors that can be a pain to debug. Can you try to find the correct combination of (Int|Rect)/(Rect|Size) methods to do this?  I think it's something like:
> 
> scaledContentRect = FloatRect(contentRect).scale(invScale).enclosingIntRect();
> 
> or something along those lines (don't take my word for it, please test)

Done. Thanks for suggestions!

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:195
>> +    updateContentsScale();
> 
> this still seems weird - setting the shouldn't change the contents scale.  contentsScale() appears to be a function of appliesPageScale(), pageScaleFactor(), and deviceScaleFactor(). Can we recalculate the scale factor when those things change, and not when setting the transform? we have a deviceOrPageScaleChanged() function already that seems like a good place to update this

Yes deviceOrPageScaleChanged() already submit the contentsScale to LayerChromium and it's a good place to do this stuff. However deviceOrPageScaleChanged() is not called when page_scale * device_scale is not 1.0 when the GraphicsLayer is created. GraphicsLayerChromium needs to call updateContentsScale() somewhere to fetch the current contents scale. I thought of doing this in the constructor, but that is not a good place because WebKit sets setAppliesPageScale() after GraphicsLayer is constructed here: http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderLayerBacking.cpp&exact_package=chromium&q=RenderLayerBacking.cpp&type=cs&l=124.

Since updateLayerTransform() is always called when creating a GraphicsLayer I thought it would be a OK place to read the current page scale. Otherwise we'll need to add a method to GraphicsLayer and call it from RenderLayerBacking::createPrimaryGraphicsLayer().

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:567
> +        updateContentsScale();

Remove this one too.

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:600
>> +        updateContentsScale();
> 
> this call also confuses me a bit - why do we need to update the contentsScale here when updating the transform hierarchy? we aren't changing the pageScale or deviceScale here

Removed.

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:687
>> +    return 1.0f;
> 
> nit: for constants like this in WebKit we typically just say '1'

Done.
Comment 45 Hin-Chung Lam 2011-11-15 04:53:57 PST
Created attachment 115144 [details]
Patch
Comment 46 James Robinson 2011-11-15 11:17:28 PST
This patch doesn't apply to ToT so none of the bots can run, would you mind updating it?
Comment 47 Hin-Chung Lam 2011-11-15 14:22:27 PST
Created attachment 115242 [details]
Patch
Comment 48 WebKit Review Bot 2011-11-15 14:27:14 PST
Attachment 115242 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu> in LayoutTests/platform/chromium/test_expectations.txt
ERROR: Line:2735 Path does not exist. plugins/invalidate_rect.html
LayoutTests/platform/chromium/test_expectations.txt:2735:  Path does not exist. plugins/invalidate_rect.html  [test/expectations] [2]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 Hin-Chung Lam 2011-11-15 14:27:41 PST
Comment on attachment 115242 [details]
Patch

Whoops.. silly test_expectations.txt.. It's updated now.
Comment 50 Hin-Chung Lam 2011-11-15 14:28:51 PST
Whoops.. I have nothing to do to plugins/invalidate_rect.html :(
Comment 51 James Robinson 2011-11-15 14:35:56 PST
Comment on attachment 115242 [details]
Patch

What invalidates layers when the contentsScale changes?
Comment 52 James Robinson 2011-11-16 15:30:19 PST
It looks like the CoreAnimation implementation pulls the page and device scales instead of relying on GraphicsLayer pushing them.  Since they use a deferred update model, on initialization WebCore has a chance to set up all state (including appliesPageScale) before the GraphicsLayer implementation tries to figure out the active page scale.  Triggering off of setTransform() is ugly and fragile (what if RenderLayerBacking reorders those two lines?) but I'm not sure of what else we could do.

One possibility would be to make GraphicsLayer::setAppliesPageScale virtual and override it in GraphicsLayerChromium to recalculate the scale factors there.
Comment 53 James Robinson 2011-11-16 15:32:32 PST
It looks like the CoreAnimation implementation pulls the page and device scales instead of relying on GraphicsLayer pushing them.  Since they use a deferred update model, on initialization WebCore has a chance to set up all state (including appliesPageScale) before the GraphicsLayer implementation tries to figure out the active page scale.  Triggering off of setTransform() is ugly and fragile (what if RenderLayerBacking reorders those two lines?) but I'm not sure of what else we could do.

One possibility would be to make GraphicsLayer::setAppliesPageScale virtual and override it in GraphicsLayerChromium to recalculate the scale factors there.
Comment 54 Alexandre Elias 2011-11-16 16:15:46 PST
We're currently using deviceOrPageScaleFactorChanged() to trigger an invalidate.  It should be applied to sublayers as well.

m_appliesPageScale looks like it's a flag set only during initialization to denote the root layer -- it doesn't seem to have much to do with invalidation.
Comment 55 James Robinson 2011-11-16 16:28:17 PST
Comment on attachment 115242 [details]
Patch

Yeah, it's not clear cut.

Anyway I think we can iterate on that point.  This patch looks good, R=me.
Comment 56 WebKit Review Bot 2011-11-16 16:59:21 PST
Comment on attachment 115242 [details]
Patch

Rejecting attachment 115242 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
iting/geometry/fixed-position-transform-composited-page-scale-expected.txt
patching file LayoutTests/compositing/geometry/fixed-position-transform-composited-page-scale.html
patching file LayoutTests/platform/chromium/test_expectations.txt
Hunk #1 FAILED at 3922.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'James Robinson', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/10487914
Comment 57 Iain Merrick 2011-11-18 08:57:32 PST
Created attachment 115821 [details]
Fixed merge conflicts
Comment 58 Iain Merrick 2011-11-18 09:03:45 PST
Uploaded a patch with the same code, merge conflicts resolved.

I left the author unchanged in the ChangeLogs, not sure of the protocol there.

The main change is that the change that split up LayerTextureUpdaterCanvas.cpp (http://trac.webkit.org/changeset/99813) has been reverted, so I moved the BitmapCanvasLayerTextureUpdater and SkPictureCanvasLayerTextureUpdater diffs back to that file.

Ran unit tests and new-run-webkit-tests --accelerated-drawing --threaded-compositing compositing. Alpha's new tests and a few others fail with what look like invisible sub-pixel differences. Not sure if that's the new code, or existing failures. I guess the tests need to be rebaselined? But I'm not sure how to do that.

James, hopefully this is at least in a more useful state for you. :)
Comment 59 James Robinson 2011-11-18 15:57:58 PST
(In reply to comment #58)
> Uploaded a patch with the same code, merge conflicts resolved.
> 
> I left the author unchanged in the ChangeLogs, not sure of the protocol there.
> 
> The main change is that the change that split up LayerTextureUpdaterCanvas.cpp (http://trac.webkit.org/changeset/99813) has been reverted, so I moved the BitmapCanvasLayerTextureUpdater and SkPictureCanvasLayerTextureUpdater diffs back to that file.
> 
> Ran unit tests and new-run-webkit-tests --accelerated-drawing --threaded-compositing compositing. Alpha's new tests and a few others fail with what look like invisible sub-pixel differences. Not sure if that's the new code, or existing failures. I guess the tests need to be rebaselined? But I'm not sure how to do that.

I don't know why you ran with -accelerated-drawing, we don't really support that configuration today.  The useful configuration to test is the default one.

 
> James, hopefully this is at least in a more useful state for you. :)

Yes, this is helpful. Thanks.
Comment 60 James Robinson 2011-11-18 19:05:20 PST
http://trac.webkit.org/changeset/100839
Comment 61 Simon Fraser (smfr) 2012-03-20 18:59:08 PDT
Use of ::webkit-scrollbar to hide scrollbars doesn't work in WK1, so the WK1 pixel results don't match.