Skip to content

[css-paint-api] Calls to paint function should provide the snapped concrete size. #508

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
flackr opened this issue Nov 7, 2017 · 2 comments

Comments

@flackr
Copy link
Contributor

flackr commented Nov 7, 2017

Section 8: Drawing an image specifies that the Concrete Object Size is used when invoking the paint callback, however in the case of subpixel sizes the actual paint size depends on the browser's subpixel snapping.

For example, given a device with a scale factor of 1.25 if drawing a series of boxes of size 9px, the physical screen size is 11.25px. This means that one in every 4 boxes will be drawn a physical pixel taller than the rest.

There are 4 options I can think of although I believe we should avoid the first two as neither allows developers to do the right thing:

  1. Don't change spec, causing all paintings to be blurry (scaled up or down to fit pixel snapping).
  2. Don't change spec, draw at subpixel position causing paintings to be blurry.
  3. Adjust concrete object size to be snapped size / devicePixelRatio (e.g. in this example pass in either 11 / 1.25 = 8.8px or 12 / 1.25 = 9.6px). Edges of the drawn object size continue to be sharp and developers scaling by devicePixelRatio and accounting for the object size get pixel perfect drawings.
  4. Add backingStorePixelRatio(X|Y) which maps concrete size to physical pixel size.

My preference is for option 3 given it is a small change, and it produces crisp edges for developers naively painting to their size, and do the right thing for developers taking into account the devicePixelRatio as well.

@flackr
Copy link
Contributor Author

flackr commented Nov 7, 2017

As en example of each of these strategies, see this code example and blown up image below:
paintworklet-subpixel-examples

@css-meeting-bot
Copy link
Member

The Working Group just discussed Concrete object size should be the snapped pixel size?, and agreed to the following resolutions:

  • RESOLVED: Accept proposal #3 for pixel-snapping the backing store
  • RESOLVED: Add an issue for exposing the browser's snapping behavior via an API in a future level.
The full IRC log of that discussion <iank_> Topic: Concrete object size should be the snapped pixel size?
<iank_> github: https://github.com//issues/508
<TabAtkins> iank_: In th eissue, flackr has a detailed descriptionof what we want to do.
<TabAtkins> iank_: Involves pixel snapping of iamges
<TabAtkins> iank_: Happens a lot
<TabAtkins> iank_: Proposing #3 on this list, to adjust concrete object size to snapped pixel size / device pixel ratio
<TabAtkins> iank_: So if people just naively draw into the size get the expected beahvior.
<TabAtkins> iank_: If people want to draw a perfect pixel-aligned line, they can multiple by pixel ratio
<TabAtkins> TabAtkins: So they might get like a 10.5px value for th eobject size?
<TabAtkins> iank_: Yes
<TabAtkins> smfr: If you ahvea transformed ancestor this doesn't affect that, right?
<TabAtkins> iank_: Yeah
<TabAtkins> smfr: Like we do today with paintaing backgrounds?
<TabAtkins> iank_: Yes. Matches what we do internally.
<TabAtkins> dbaron: I think our transform behavior doesn't match this. We try in some cases to rasterize at higher resolutions.
<TabAtkins> dbaron: We might have changed that.
<TabAtkins> mwoodrow: We still do that.
<TabAtkins> smfr: We do that too
<TabAtkins> smfr: If you do scale(2) we'll rasterize at higher resolution
<TabAtkins> smfr: But I thin kthe snapping we do ignores transforms
<TabAtkins> TabAtkins: In the general case you have to; it might not be axis-aligned
<TabAtkins> dbaron: I think if the transform is only a translate/scale, we try to honor that with higher fidelity.
<TabAtkins> TabAtkins: Do we want to try and make the spec generic to platform behavior?
<TabAtkins> dbaron: Maybe - we should probably converge at some point.
<TabAtkins> iank_: Nice thing is that this approach lets naive behavior work right, regardless
<TabAtkins> TabAtkins: I can help with the spec text if we want to allow platform divergence here. Testing is harder tho.
<TabAtkins> TabAtkins: But if we want to spec a single behavior that's a different thing.
<TabAtkins> iank_: Yeah, that requires more convergence.
<TabAtkins> TabAtkins: So resolution is, *whatever* the browser's snapping bheavior is, it must apply in the way described in #3?
<TabAtkins> Rossen: And #4 means no snapping?
<TabAtkins> Rossen: Because backing stor epixel ratio is what gets you back to physical pixels, so conversion happens in user code, nobody has to muck around with adjust the size
<TabAtkins> Rossen: So whatever Gecko does...
<TabAtkins> iank_: If we give people physical pixels, that has a bunch more interop issues.
<TabAtkins> iank_: If we end up giving you a much larger backing store and Gecko doesn't, and you naively assume its the CSS size, it gives us interop pain.
<TabAtkins> iank_: It might be a better option later on to actually request physical pixels, but for now we have the naive possibly-fractional CSS px
<TabAtkins> TabAtkins: What does the backing store look like if we ahve a 2x pixel ratio?
<TabAtkins> iank_: First, it might be different from the device pixel ratio, due to scales
<TabAtkins> iank_: You can't see the underlying pixel ratio of the canvas in the worklet.
<TabAtkins> TabAtkins: Ah, that satisfies my concern.
<TabAtkins> iank_: And later we can add a flag to ask for th ephysical size if you really want to draw hairlines or something.
<TabAtkins> smfr: A general comment - you should get the same rendered result whether the UA stores a display list or a bitmap
<TabAtkins> smfr: Question - does the context start with a scale for the dpr?
<TabAtkins> iank_: That's how we do it internally. The backing size is the snapped pixel size scaled by the dpr.
<TabAtkins> iank_: So people can just use naive CSS px, which they like, and they still get sharp lines.
<TabAtkins> iank_: Can add a note suggesting that for impls.
<TabAtkins> smfr: Do you give the author enough info to reproduce the browser snapping behavior?
<TabAtkins> smfr: So when the brwoser pixel snaps, for us, we consult the rectangle origin - we'll snap both origin and width using some complex rules.
<TabAtkins> smfr: If the person using the paint worklet wants to snap in same way, that would be hard in a cross-browser way
<TabAtkins> iank_: I think we convinced ourselves internally that we didn't need to worry about th eorigin.
<flackr> Because the origin is snapped and you know the number of snapped pixels you can match browser snapping
<TabAtkins> smfr: So say you want to paint a line halfway in the background, matching the browser behavior for a 50% width box with a border .
<TabAtkins> smfr: Matching the browser snapping behavior there is non-obvious.
<TabAtkins> smfr: Not saying we need to solve immediately, and probably would be hard to do and expose unsavory browser internals, but it should be an issue.
<flackr> In the jsbin demo code I show how you'd convert to native size
<TabAtkins> iank_: Trying to read a big number-heavy thread right now for this.
<TabAtkins> smfr: could imagine a .snappedPoint/Rect() function, that you feed a point/rect and it returns a snapped one.
<TabAtkins> iank_: [reads out from teh thread, where a proposal matches what smfr just said]
<TabAtkins> iank_: So future level, but we should open an issue for that.
<TabAtkins> TabAtkins: So back to original proposal.
<TabAtkins> smfr: I like #3
<TabAtkins> dbaron: I don't think I understand all the details, but #3 seems to be okay.
<TabAtkins> TabAtkins: I don't either, but if you alter understand the details and disagree, we can revisit.
<TabAtkins> RESOLVED: Accept proposal #3 for pixel-snapping the backing store
<TabAtkins> RESOLVED: Add an issue for exposing the browser's snapping behavior via an API in a future level.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2017
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 14, 2017
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 15, 2017
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
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 15, 2017
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}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Nov 15, 2017
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}
MXEBot pushed a commit to mirror/chromium that referenced this issue Nov 16, 2017
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}
@bfgeek bfgeek closed this as completed in dcfdb44 Apr 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants