1461849 - Include the fractional transform when drawing blob images

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Jeff Muizelaar [:jrmuizel]]

This will fix a bunch of snapping issues.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Handle fractional offsets

This works on the basic test case that I was using. It needs further testing. Maybe some of the scale stuff is wrong.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Expose a snapping surface transform

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Handle fractional offsets

This should handle animations and invalidation properly

Comment 4

[Aria Beingessner [:Gankra]]

Comment on attachment 8976365 [details] [diff] [review]
Handle fractional offsets

Review of attachment 8976365 [details] [diff] [review]:
-----------------------------------------------------------------

frustrated jeff is bad code

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +314,5 @@
>    nsRect mGroupBounds;
>    int32_t mAppUnitsPerDevPixel;
>    gfx::Size mScale;
>    FrameMetrics::ViewID mScrollId;
> +  gfx::Point mOffset;

make this well-typed

@@ +574,2 @@
>      IntSize dtSize = layerBounds.Size().ToUnknownSize();
> +    LayoutDeviceRect bounds = (layerBounds / scale) - LayoutDevicePoint(mOffset.x, mOffset.y);

split this into two vars to avoid undoing the offset later

also: (bounds - offset) / scale

@@ +603,5 @@
>      RefPtr<gfxContext> context = gfxContext::CreateOrNull(dt);
>      GP("ctx-offset %f %f\n", bounds.x, bounds.y);
> +    context->SetMatrix(Matrix::Scaling(mScale.width, mScale.height)
> +                       .PreTranslate(-(bounds.x + mOffset.x), -(bounds.y + mOffset.y))
> +                       .PostTranslate(mOffset.x, mOffset.y));

would prefer Translate(x).PostScale(y).PostTranslate(z);

also maybe this should be Translate(x).PostTranslate(y).PostScale(z) so the mOffset is scaled.

possibly just revert

@@ +958,5 @@
>          // The group changed size
>          GP("Inner group size change\n");
>          groupData->mFollowingGroup.ClearItems();
>          if (groupData->mFollowingGroup.mKey) {
> +          groupData->mFollowingGroup.mInvalidRect = IntRect(IntPoint(0, 0), groupData->mFollowingGroup.mLayerBounds.Size().ToUnknownSize());

currentGroup?

@@ +1054,5 @@
> +ScaleToOutsidePixelsOffset(nsRect aRect, float aXScale, float aYScale,
> +                             nscoord aAppUnitsPerPixel, gfx::Point aOffset)
> +{
> +  mozilla::gfx::IntRect rect;
> +  rect.SetNonEmptyBox(NSToIntFloor(NSAppUnitsToFloatPixels(aRect.x,

This might not be NonEmpty...?

@@ +1063,5 @@
> +                                                          float(aAppUnitsPerPixel)) * aXScale + aOffset.x),
> +                      NSToIntCeil(NSAppUnitsToFloatPixels(aRect.YMost(),
> +                                                          float(aAppUnitsPerPixel)) * aYScale + aOffset.y));
> +  return rect;
> +}

Maybe comment this is just ScaleToOutsidePixels copy-pasted

@@ +1091,5 @@
>    auto p = group.mGroupBounds;
>    auto q = groupBounds;
>    gfx::Size scale = aSc.GetInheritedScale();
> +  auto trans = aSc.GetSnappingSurfaceTransform().GetTranslation();
> +  auto snappedTrans = IntPoint::Floor(trans); // floor or round?

remove comment

flooooor!!!!

@@ +1129,5 @@
>    group.mGroupBounds = groupBounds;
> +  group.mAppUnitsPerDevPixel = appUnitsPerDevPixel;
> +  group.mLayerBounds = LayerIntRect::FromUnknownRect(ScaleToOutsidePixelsOffset(group.mGroupBounds, scale.width, scale.height, group.mAppUnitsPerDevPixel, offset));
> +  Matrix residual = Matrix::Translation(offset);
> +  g.mTransform = Matrix::Scaling(scale.width, scale.height) * residual;

.Post?Translate

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1461849. Don't reset the invalid rect.

I believe this was a left-over from an old strategy where we didn't clear the
items, tried to reuse them but kept made an invalid rect for the old size. Now
that we clear the items this shouldn't be needed. I added the assert to make
sure things are what we expect.

Review commit: https://reviewboard.mozilla.org/r/244820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244820/

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1461849. Snapping surface transform.

Capture a snapping surface transform. This is used
by svg/blob code to compute an fractional offset
to the nearest intermediate surface.

Review commit: https://reviewboard.mozilla.org/r/244822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244822/

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1461849. Include a residual offset for blob images.

This lets us improve the illuision that there's not an intermediate surface
being used when drawing SVG. i.e. if content is transformed by 0.5px
the 0.5px transform is included when drawing the SVG content.

Review commit: https://reviewboard.mozilla.org/r/244824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244824/

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8976735 [details]
Bug 1461849. Don't reset the invalid rect.

https://reviewboard.mozilla.org/r/244820/#review251154

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8976736 [details]
Bug 1461849. Snapping surface transform.

https://reviewboard.mozilla.org/r/244822/#review251160

::: gfx/layers/wr/StackingContextHelper.h:76
(Diff revision 1)
>  
>  private:
>    wr::DisplayListBuilder* mBuilder;
>    gfx::Size mScale;
>    gfx::Matrix mInheritedTransform;
> +  gfx::Matrix mSnappingSurfaceTransform;

Would be good to have a description of what the snapping surface is.

// The "snapping surface" defines the space that we want to snap in.
// You can think of it as the nearest physical surface.
// Animated transforms create a new snapping surface, so that changes to their transform don't affect the snapping of their contents.
// Non-animated transforms do *not* create a new snapping surface,
// so that for example the existence of a non-animated identity transform does not affect snapping.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8976737 [details]
Bug 1461849. Include a residual offset for blob images.

https://reviewboard.mozilla.org/r/244824/#review251162

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:1051
(Diff revision 1)
>  
>      item = item->GetAbove();
>    }
>  }
>  
> +/* This is just a copy of nsRect::ScaleToOutsidePixels with an offset added in */

Let's mention at what point the offset is applied:
// The offset is applied just before the rounding. It's in the scaled space.

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:1093
(Diff revision 1)
>    nsRect groupBounds = aWrappingItem->GetBounds(aDisplayListBuilder, &snapped);
>    DIGroup& group = groupData->mSubGroup;
>    auto p = group.mGroupBounds;
>    auto q = groupBounds;
>    gfx::Size scale = aSc.GetInheritedScale();
> +  auto trans = aSc.GetSnappingSurfaceTransform().GetTranslation();

Would be nice to give this the type LayerPoint.

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:1129
(Diff revision 1)
> +  group.mLayerBounds = LayerIntRect::FromUnknownRect(ScaleToOutsidePixelsOffset(group.mGroupBounds, scale.width, scale.height, group.mAppUnitsPerDevPixel, residualOffset));
> +  g.mTransform = Matrix::Scaling(scale.width, scale.height).PostTranslate(residualOffset.x, residualOffset.y);

Please break these long lines so that they're shorter.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8976736 [details]
Bug 1461849. Snapping surface transform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244822/diff/1-2/

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8976737 [details]
Bug 1461849. Include a residual offset for blob images.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244824/diff/1-2/

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8976735 [details]
Bug 1461849. Don't reset the invalid rect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244820/diff/1-2/

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8976736 [details]
Bug 1461849. Snapping surface transform.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244822/diff/2-3/

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8976737 [details]
Bug 1461849. Include a residual offset for blob images.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244824/diff/2-3/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea2c202529c9
Don't reset the invalid rect. r=mstange
https://hg.mozilla.org/integration/autoland/rev/51a37cdb1732
Snapping surface transform. r=mstange
https://hg.mozilla.org/integration/autoland/rev/649cf3f83a04
Include a residual offset for blob images. r=mstange

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/ea2c202529c9
https://hg.mozilla.org/mozilla-central/rev/51a37cdb1732
https://hg.mozilla.org/mozilla-central/rev/649cf3f83a04