[CSS Paint API] Pass snapped concrete size to paint function #518

xidachen · 2017-11-14T15:51:35Z

This pull request address the issue here:
#508

Basically, instead of passing the concrete object size to the paint function, we let it pass the round(concrete object size) / device pixel ratio.

Please review. @bfgeek @flackr

Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0

bfgeek · 2017-11-14T22:20:03Z

css-paint-api/Overview.bs

@@ -630,7 +631,7 @@ When the user agent wants to <dfn>invoke a paint callback</dfn> given |name|, |i
        <a>computed value</a>'s for properties listed in |inputProperties|.

    8. Let |renderingContext| be the result of <a>create a PaintRenderingContext2D object</a> given:
-            - "width" - The width given by |concreteObjectSize|.
+            - "width" - The  width given by |concreteObjectSize|.


bfgeek

Looks great, @flackr ok?

flackr · 2017-11-14T22:33:50Z

css-paint-api/Overview.bs

-negotiation</a> algorithm which is responsible for rendering an <<image>>, with |concreteObjectSize|
-set to the <a>concrete object size</a> of the <a>box</a>.
+negotiation</a> algorithm which is responsible for rendering an <<image>>, with |snappedConcreteObjectSize|
+set to the rounded <a>concrete object size</a> of the <a>box</a>, divided by the


I don't think this phrasing captures pixel snapping correctly. Also, AFAICT, concrete object size is in CSS pixels. Rounding the concrete object size would suggests that

this only affects objects whose size is non-integral - this is not true as it affects integral sizes when they are rastered into a non-integral transformed space (i.e. non-integer devicePixelRatio).

we always snap an object of a particular size in the same direction, but it actually depends on the object's location.

all browsers snap the same way. This seems to be the case but should probably not be required.

Instead, I think we should specify this in a way that allows the browser to use its own subpixel snapping determination.

Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0

Right now when we set the PaintSize in CSSPaintDefinition::Paint(), the size is the concrete object size. There has been some discussion around here: w3c/css-houdini-drafts#508 and the solution is to pass the snapped concrete object size divided by device scale factor. The ongoing pull request to change the spec is here: w3c/css-houdini-drafts#518 This CL corrects the PaintSize to reflect the spec changes. It also adds a layout test to ensure the correctness. Bug: 774810 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I42b57db6ac0d40be2b408cb733a659f5ea14dfa0 Reviewed-on: https://chromium-review.googlesource.com/733862 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Xida Chen <xidachen@chromium.org> Cr-Commit-Position: refs/heads/master@{#516872}

xidachen · 2017-11-16T16:04:27Z

flackr@: I changed the description of |snappedConcreteSize|, let me know if it is better or worse.

flackr · 2017-11-16T17:56:14Z

css-paint-api/Overview.bs

+negotiation</a> algorithm which is responsible for rendering an <<image>>, with |snappedConcreteObjectSize|
+defined as follows. Let |concreateObjectSize| be the <a>concrete object size</a> of the <a>box</a>, and
+|deviceObjectSize| be the |concreateObjectSize| multiplied by the {{PaintWorkletGlobalScope/devicePixelRatio}}.
+If the |deviceObjectSize| is non-integral, then adjust it to align with pixel boundary. Finally, set


It's not a requirement to snap, but the browser is allowed to. How about something like this?

The |snappedConcreteObjectSize| is usually the same as the |concreteObjectSize|; however, the user agent may adjust the size such that it paints to pixel boundaries. If it does, the user agent should adjust the |snappedConcreteObjectSize| by the proportional change from its original size so that the paint function can adjust the drawing accordingly.

flackr · 2017-11-16T19:11:00Z

css-paint-api/Overview.bs

-If the |deviceObjectSize| is non-integral, then adjust it to align with pixel boundary. Finally, set
-|snappedConcreateObjectSize| to the adjusted |deviceObjectSize| divided by the
-{{PaintWorkletGlobalScope/devicePixelRatio}}.
+defined as follows. Let |concreateObjectSize| be the <a>concrete object size</a> of the <a>box</a>.


s/concreateObjectSize/concreteObjectSize/ here and below.

flackr · 2017-11-16T19:20:06Z

css-paint-api/Overview.bs

-defined as follows. Let |concreateObjectSize| be the <a>concrete object size</a> of the <a>box</a>.
-The |snappedConcreateObjectSize| is usually the same as the |concreateObjectSize|. However, the
+defined as follows. Let |concreteObjectSize| be the <a>concrete object size</a> of the <a>box</a>.
+The |snappedConcreateObjectSize| is usually the same as the |concreteObjectSize|. However, the


s/snappedConcreateObjectSize/snappedConcreteObjectSize here and below

changed to snapped concret size

09bd3e2

chromium-wpt-export-bot mentioned this pull request Nov 14, 2017

[PaintWorklet] Pass snapped concrete size to paint function web-platform-tests/wpt#8100

Merged

updated rendering context width height

ece4b77

bfgeek reviewed Nov 14, 2017

View reviewed changes

bfgeek approved these changes Nov 14, 2017

View reviewed changes

flackr reviewed Nov 14, 2017

View reviewed changes

change the description of snappedConcreteObjectSize

afc97a4

flackr reviewed Nov 16, 2017

View reviewed changes

change the description

5ae88e0

flackr reviewed Nov 16, 2017

View reviewed changes

fix typo

738c81b

flackr reviewed Nov 16, 2017

View reviewed changes

fix more typo

9413d54

flackr approved these changes Nov 16, 2017

View reviewed changes

bfgeek merged commit dcfdb44 into w3c:master Apr 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CSS Paint API] Pass snapped concrete size to paint function #518

[CSS Paint API] Pass snapped concrete size to paint function #518

xidachen commented Nov 14, 2017

bfgeek Nov 14, 2017

bfgeek left a comment

flackr Nov 14, 2017 •

edited

Loading

xidachen commented Nov 16, 2017

flackr Nov 16, 2017

xidachen Nov 16, 2017

flackr Nov 16, 2017

xidachen Nov 16, 2017

flackr Nov 16, 2017

xidachen Nov 16, 2017

[CSS Paint API] Pass snapped concrete size to paint function #518

[CSS Paint API] Pass snapped concrete size to paint function #518

Conversation

xidachen commented Nov 14, 2017

bfgeek Nov 14, 2017

Choose a reason for hiding this comment

bfgeek left a comment

Choose a reason for hiding this comment

flackr Nov 14, 2017 • edited Loading

Choose a reason for hiding this comment

xidachen commented Nov 16, 2017

flackr Nov 16, 2017

Choose a reason for hiding this comment

xidachen Nov 16, 2017

Choose a reason for hiding this comment

flackr Nov 16, 2017

Choose a reason for hiding this comment

xidachen Nov 16, 2017

Choose a reason for hiding this comment

flackr Nov 16, 2017

Choose a reason for hiding this comment

xidachen Nov 16, 2017

Choose a reason for hiding this comment

flackr Nov 14, 2017 •

edited

Loading