Bug 107359 – [Mac] Use pageScaleFactor * deviceScaleFactor in requiresTiledLayer() and computePixelAlignment() of GraphicsLayerCA.

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

[Dongseong Hwang]

Currently GraphicsLayerCA uses pageScaleFactor in requiresTiledLayer() and
computePixelAlignment(), but we must use pageScaleFactor * deviceScaleFactor in
them.
It is because:
1. requiresTiledLayer() uses the scale to compute an actual layer size in the device pixel unit.
2. computePixelAlignment() uses the scale to compute an aligned layer position in the device pixel unit.

[Dongseong Hwang]

Created attachment 183603 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 183603 [details]
Patch

Clearing flags on attachment: 183603

Committed r150047: <http://trac.webkit.org/changeset/150047>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 183603 [details]
Patch

This sis wrong. We used to do this, and it causes pages to have different behavior on Retina and non-retina devices, which is undesirable. We’re not required to fall into tiled layers for larger surfaces; it’s just a memory optimization.

Please revert the change.

[Dongseong Hwang]

(In reply to comment #4)
> (From update of attachment 183603 [details])
> This sis wrong. We used to do this, and it causes pages to have different behavior on Retina and non-retina devices, which is undesirable. We’re not required to fall into tiled layers for larger surfaces; it’s just a memory optimization.
> 
> Please revert the change.

Thank you for explanation. Can I ask a question?, because I don't fully catch up yet.

In my understand of your explanation, it means that On Ratina, WebKit draws content on the same sized backing store and scales 2 times on the device. is it right?

Let me say that the device scale factor on Ratina is 2.

My curiosity is as follow:

First of all, below code also causes different behavior on Retina and non-retina devices. afaik, layer's backing store on Ratina will be 2 times bigger than it on non-ratina, because contentsScale is 2 times bigger on Ratina.
void GraphicsLayerCA::updateContentsScale(float pageScaleFactor)
{
    float contentsScale = clampedContentsScaleForScale(pageScaleFactor * deviceScaleFactor());
    
    m_layer->setContentsScale(contentsScale);
    if (drawsContent())
        m_layer->setNeedsDisplay();
}

In addition, backing store on Ratina should be two times bigger to draw text clearly.

Even after rethinking, imo, my patch seems to be correct..

[Simon Fraser (smfr)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 183603 [details] [details])
> > This sis wrong. We used to do this, and it causes pages to have different behavior on Retina and non-retina devices, which is undesirable. We’re not required to fall into tiled layers for larger surfaces; it’s just a memory optimization.
> > 
> > Please revert the change.
> 
> Thank you for explanation. Can I ask a question?, because I don't fully catch up yet.
> 
> In my understand of your explanation, it means that On Ratina, WebKit draws content on the same sized backing store and scales 2 times on the device. is it right?

Correct. 

> Let me say that the device scale factor on Ratina is 2.
> 
> My curiosity is as follow:
> 
> First of all, below code also causes different behavior on Retina and non-retina devices. afaik, layer's backing store on Ratina will be 2 times bigger than it on non-ratina, because contentsScale is 2 times bigger on Ratina.

Actually 4x in memory use.

> void GraphicsLayerCA::updateContentsScale(float pageScaleFactor)
> {
>     float contentsScale = clampedContentsScaleForScale(pageScaleFactor * deviceScaleFactor());

Right, this is clamping based on the estimated memory use for the layer. Actually that could be pageScaleFactor * deviceScaleFactor() * deviceScaleFactor().

>     m_layer->setContentsScale(contentsScale);
>     if (drawsContent())
>         m_layer->setNeedsDisplay();
> }
> 
> In addition, backing store on Ratina should be two times bigger to draw text clearly.
> 
> Even after rethinking, imo, my patch seems to be correct..

It is theoretically correct, but introduces unwanted behavior. I want the layer size threshold at which requiresTiledLayer() returns true to be the same for retina and non-retina devices. That does mean that we allow layer memory use on retina to be larger before we fall into tiled layers, but we're willing to live with that.

I'm curious about what device you care about that's using GraphicsLayerCA?

[Dongseong Hwang]

(In reply to comment #6)
> It is theoretically correct, but introduces unwanted behavior. I want the layer size threshold at which requiresTiledLayer() returns true to be the same for retina and non-retina devices. That does mean that we allow layer memory use on retina to be larger before we fall into tiled layers, but we're willing to live with that.

I see. I'm ok to roll back this patch. I can not do it because I don't have committer permission.

> I'm curious about what device you care about that's using GraphicsLayerCA?

Nothing.
On this January, when I read GraphicsLayerCA to understand original GraphicsLayer implementation, I just found the point to conflict with what I understood.

I'm sorry for noise.

[Simon Fraser (smfr)]

Reverted in https://trac.webkit.org/r150135