Closed Bug 1461849 Opened 3 years ago Closed 3 years ago

Include the fractional transform when drawing blob images

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 2 obsolete files)

This will fix a bunch of snapping issues.
Blocks: 1458968
Attached patch Handle fractional offsets (obsolete) — Splinter Review
This works on the basic test case that I was using. It needs further testing. Maybe some of the scale stuff is wrong.
Assignee: nobody → jmuizelaar
Attachment #8976005 - Flags: review?(mstange)
Attachment #8976005 - Attachment is obsolete: true
Attachment #8976005 - Flags: review?(mstange)
Attachment #8976364 - Flags: review?(mstange)
This should handle animations and invalidation properly
Attachment #8976365 - Flags: review?(mstange)
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
Attachment #8976365 - Flags: review?(mstange) → review-
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.
Attachment #8976736 - Flags: review+
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.
Attachment #8976737 - Flags: review+
Attachment #8976735 - Flags: review?(a.beingessner) → review+
Attachment #8976736 - Flags: review?(a.beingessner) → review+
Attachment #8976737 - Flags: review?(a.beingessner) → review+
Pushed by jmuizelaar@mozilla.com:
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
Attachment #8976364 - Attachment is obsolete: true
Attachment #8976364 - Flags: review?(mstange)
You need to log in before you can comment on or make changes to this bug.