1973192 - Some icons (mask-image) on https://wykop.pl/mikroblog/gorace/12 are moving 1px with layout.disable-pixel-alignment=true while autoscrolling

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

There's a recording to see this in bug 1971253 comment 12.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:hiro, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

The feature hasn't yet been enabled, but we'd like to enable as soon as possible.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP: Bug 1973192 - A reftest.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I think, the reftest in D257627 does replicate this bug. The test file is using mask-image, the reference is using background-image.

The mask-image is rendered at 9px on Y axis, whereas the background-image one is rendered at 10px on Y axis.

I am not sure where the underlying problem exists though, I guess it's either in display list building for mask-image or WR display list building for mask-image.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: gecko-display-items-for-mask-image

This is the Gecko display list dump on the mask-image one;

The relevant part is;

  nsDisplayContainer p=0x7b5002b97770 f=0x7b5002b91c68(Block(div id=mask)(2) id:mask) key=19 bounds(0,588,6000,6000) compone  ntAlpha(0,0,0,0) clip() asr(<0x7b5002b911b0>) clipChain()  reuse-state(None)
      Mask p=0x7b5002b97610 f=0x7b5002b91c68(Block(div id=mask)(2) id:mask) key=35 bounds(0,588,6000,6000) componentAlpha(0,0,  0,0) clip() asr(<0x7b5002b911b0>) clipChain(0x7b5002b97170 <0,0,48000,60000> [root asr])  reuse-state(None) effects=()
        BackgroundColor p=0x7b5002b974f0 f=0x7b5002b91c68(Block(div id=mask)(2) id:mask) key=5 bounds(0,588,6000,6000) compone  ntAlpha(0,0,0,0) clip(0,540,6000,6060) asr(<0x7b5002b911b0>) clipChain(0x7b5002b975b8 <0,540,6000,6060> [0x7b5002b911b0], 0x  7b5002b97170 <0,0,48000,60000> [root asr]) uniform  (opaque 0,588,6000,6000) hitTestInfo(0x3) hitTestArea(0,588,6000,6000) r  euse-state(None) (rgba 0,0,0,1) backgroundRect(x=0, y=588, w=6000, h=6000)

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: gecko-display-items-for-background-image

This is the Gecko display list dump on the background-image one;

The relevant part is;

    Background p=0x7b5002b974f0 f=0x7b5002a46c68(Block(div id=mask)(2) id:mask) key=4 bounds(0,588,6000,6000) componentAlpha(0  ,0,0,0) clip(0,588,6000,6000) asr(<0x7b5002a461b0>) clipChain(0x7b5002b975f0 <0,588,6000,6000> [0x7b5002a461b0], 0x7b5002b97  170 <0,0,48000,60000> [root asr])  hitTestInfo(0x1) hitTestArea(0,588,6000,6000) reuse-state(None)

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: wr-display-items-for-mask-image

This is the WR display list dump on the mask-image.

The relevant part is probably;

        BackgroundColor p=0x7b2f1ec424f0 f=0x7b2f2018ec68(Block(div id=mask)(2) id:mask) key=5 bounds(0,588,6000,6000) compone  ntAlpha(0,0,0,0) clip(0,540,6000,6060) asr(<0x7b2f2018e1b0>) clipChain(0x7b2f1ec425b8 <0,540,6000,6060> [0x7b2f2018e1b0], 0x  7b2f1ec42170 <0,0,48000,60000> [root asr]) uniform  (opaque 0,588,6000,6000) hitTestInfo(0x3) hitTestArea(0,588,6000,6000) r  euse-state(None) (rgba 0,0,0,1) backgroundRect(x=0, y=588, w=6000, h=6000)

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: wr-display-items-for-background-image

This is the WR display list dump on the background-image.

The relevant part is;

      Background p=0x7b2f1ec424f0 f=0x7b2f1ec29c68(Block(div id=mask)(2) id:mask) key=4 bounds(0,588,6000,6000) componentAlpha  (0,0,0,0) clip(0,588,6000,6000) asr(<0x7b2f1ec291b0>) clipChain(0x7b2f1ec425f0 <0,588,6000,6000> [0x7b2f1ec291b0], 0x7b2f1ec  42170 <0,0,48000,60000> [root asr])  hitTestInfo(0x1) hitTestArea(0,588,6000,6000) reuse-state(None)

Comment 9

[Hiroyuki Ikezoe (:hiro)]

A big difference between the dumps in comment 5 and comment 6 is the clip rect;

For the mask-image; clip(0,540,6000,6060)
For the background-image; clip(0,588,6000,6000)

For the mask one is apparently rounded.

Moving into WebPainting. There may be more suitable component, though.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I found out one of the sources of the reftest failure is this RoundOut() call in ComputeClipForMaskItem.

Without the RoundOut() call, both of the mask-image and background-image's clip are same. That's said, the reftest still fails.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Also, without the RoundOut() call, clipPath-on-thin-object.svg fails.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I found a new source of the roundings;

Without RoundOut() call, WR display items are;

For the mask-image;

    ImageMaskClip(ImageMaskClipDisplayItem { id: ClipId(3, PipelineId(1, 26)), spatial_id: SpatialId(3, PipelineId(1, 26))  , image_mask: ImageMask { image: ImageKey(IdNamespace(21), 1), rect: Box2D((0.0, 9.0), (100.0, 110.0)) }, fill_rule: Nonzero   })

For the background-image;

   Image(ImageDisplayItem { common: CommonItemProperties { clip_rect: Box2D((0.0, 9.8), (100.0, 109.8)), clip_chain_id: C  lipChainId(5, PipelineId(1, 26)), spatial_id: SpatialId(3, PipelineId(1, 26)), flags: IS_BACKFACE_VISIBLE }, bounds: Box2D((  0.0, 9.8), (100.0, 109.8)), image_key: ImageKey(IdNamespace(21), 2), image_rendering: Auto, alpha_type: PremultipliedAlpha,   color: ColorF { r: 1.0, g: 1.0, b: 1.0, a: 1.0 } })

The former's ImageMask rect is rounded; Box2D((0.0, 9.0), (100.0, 110.0)
Wheares the latter is not.

It comes from LayerIntRect usages in WebRenderCommandBuilder::BuildWrMaskImage. I think we can directly use incoming aBounds here.

That's said, the reftest still fails.

Comment 13

[Glenn Watson [:gw]]

I did some debugging with the reftest. Looping in Nical and Tim, who know this code better than me. The problem seems to occur in [1].

aBounds comes in as 100 x 100 @ 0, 9.8.

By the time it calculates the size of the mask, it has calculated 100 x 101 though, during the calculations with ScaleToOutsidePixels and intersecting with the visible rect (which I guess is to reduce the potential size of the mask).

It's not really clear to me what the right option is here - can we apply logic here to snap as WR will do later? Should we directly take the size of the supplied rect (but that will make it trickier to intersect with visible rect)?

[1] https://searchfox.org/firefox-main/source/gfx/layers/wr/WebRenderCommandBuilder.cpp#2782

Comment 14

[Timothy Nikkel (:tnikkel)]

Blobs need to use a int size because they need to be rasterized to a draw target with a specific size, and that needs to be an int size. That part I think we are stuck with, but we have control over other things here. If we admit that blobs are creatures that live in integer space and they have the limitations that come with that, but we know about float space so we can place them intelligently, maybe that is a workable solution? The current design has blobs living in integer space, but falsely saying "these integer objects have rendered correctly at non-integer coordinates".

The current design is basically snapping to pixels and then saying "this blob primitive is drawn at exactly these integer coordinates", when that is not true, I think its more like "the primitives inside this blob are drawn at some subpixel offset inside these integer coordinates", and then the subpixel offset is lost/assumed to be 0.

Could we do something like this instead. Shift the item to the origin, then do scale to outside pixels, when drawing into the blob set the transform

https://searchfox.org/firefox-main/rev/e613f4df351a21871cfeadf7d5b4043ffad157b1/gfx/layers/wr/WebRenderCommandBuilder.cpp#2878

so that the start of the bounds are at an integer location (ie the origin), and then keep track of that subpixel offset with the blob, pass that through to webrender, and webrender can incorporate the subpixel offset to render better.

That's a little bit hand wavey and a bit larger of a change, to make a much smaller concrete suggestion what if here

https://searchfox.org/firefox-main/rev/e613f4df351a21871cfeadf7d5b4043ffad157b1/gfx/layers/wr/WebRenderCommandBuilder.cpp#2879

we change what we translate by. Currently it is itemRect which is the result of ScaleToOutsidePixels. For that translate only we could try the result of ScaleToNearestPixels, or just scale to pixels with no conversion to integer. I'm not sure if that will make things better or worse or neither.

Comment 15

[Nicolas Silva [:nical]]

I am not very familiar with the details of this stuff, I agree with Tim's general assessment.

Comment 16

[Timothy Nikkel (:tnikkel)]

I re-read Glenn's question, I might have strayed a little. Let me try to give a more focused answer.

The size of the blob has to be integer sized. The content we draw inside the blob is likely to be snapped. We have control over the offset of the drawing into the blob using a (float) offset/transform. The coords of the primitive we push to webrender are also float and we have control over those (they don't necessarily need to be linked to any rounding we do to get an int size for the blob draw target). So we can try adjusting these things to try to get matching snapping/rounding behaviour as webrender. What is the snapping behaviour of webrender and what do we do to match it here?

Comment 17

[Glenn Watson [:gw]]

Attachment: Bug 1973192 - Snap mask geometry via WebRender's frame-time pass when pixel alignment is disabled.

With layout.disable-pixel-alignment, the masked content is snapped to the
nearest device pixel by WebRender at frame time, but the mask blob's
placement was still rounded out (against a stale scroll offset) during
display list building, leaving the mask 1px out of step with what it masks.

Send the mask placement rect unrounded from BuildWrMaskImage so WebRender
snaps the mask clip in lockstep with the masked content, and rasterize the
mask blob on the nearest-pixel grid (rather than rounding out) so its alpha
lines up with the snapped placement.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b9bc8ac6f20f
https://hg.mozilla.org/integration/autoland/rev/62ca324b11d6
Snap mask geometry via WebRender's frame-time pass when pixel alignment is disabled. r=gfx-reviewers,lsalzman

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/62ca324b11d6