[webkit-reviews] review requested: [Bug 86882] [chromium] Make sure that render surfaces are not pixel doubled with a device scale factor of 2 : [Attachment 148484] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 86882: [chromium] Make sure that render surfaces are not pixel doubled with
a device scale factor of 2
https://bugs.webkit.org/show_bug.cgi?id=86882

Attachment 148484: Patch
https://bugs.webkit.org/attachment.cgi?id=148484&action=review

------- Additional Comments from vollick at chromium.org
(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!