Closed Bug 751187 Opened 12 years ago Closed 11 months ago

support tiled image background layers


(Core :: Graphics: Layers, defect)






(Reporter: gal, Unassigned)




(1 file, 8 obsolete files)

This allows us to support a wider range of background image scaling on the GPU.
Attached patch patch (obsolete) — Splinter Review
A couple things are still missing:
- a basic test case works but is broken (zoom in, the famous 990x10 px tiled background is all messed up)
- resampling restriction has to be applied if the source tile isn't the whole image/texture (maybe by copying out the pixels into a new texture?)
- need support for tiled image layers on windows
Blocks: 749062
Attached patch patch (obsolete) — Splinter Review
This fixes the issue. The visible rect wasn't set right. It has to be larger than the image since we are tiling, but its in untransformed coordinates.
Attachment #620324 - Attachment is obsolete: true
Alright, this patch is only minimally tested and is probably still all sorts of buggy, but with this CPU image scaling is basically no longer a cost on Definitely the right direction I would say.

Running (Self)		Symbol Name
282.0ms    3.5%	 	PR_GetThreadPrivate
202.0ms    2.5%	 	memmove$VARIANT$sse42
144.0ms    1.7%	 	malloc_rtree_get
119.0ms    1.4%	 	PR_GetCurrentThread
117.0ms    1.4%	 	malloc_rtree_get_locked
86.0ms    1.0%	 	objc_msgSend
84.0ms    1.0%	 	nsTArray_base<nsTArrayDefaultAllocator>::Length() const
Assignee: nobody → gal
Attachment #620335 - Attachment is obsolete: true
Attachment #620558 - Flags: feedback?(roc)
Summary: support tiled image layers → support tiled image background layers
Comment on attachment 620558 [details] [diff] [review]
Support tiled background image layers

Review of attachment 620558 [details] [diff] [review]:

I think it might be best to make this interface specific to ImageLayer. As you note it doesn't really make sense for other layer types. (Well, it would for tiling -moz-element(#id) but we can deal with that separately when we implement layers support for it ... I have a plan.)

Also, I don't think we have much need to tile subimages right now. In CSS you can only tile the whole image unless you do something fancy with media fragments or -moz-image-rect, and I don't think we need first-class layers support for those. (And when we do, I have a plan.)

Given that, I think the best API here, which matches most closely what we do for image drawing, would be to specify a rectangle *in tiled image space* (let's call it "tile rect") representing the extents of what we want to render. So the processing model for ImageLayers would be:
1) take the current Image of the ImageContainer and tile it over the plane, with the top-left corner of a tile at 0,0 (each image pixel mapped to one layer pixel)
2) composite that plane to the destination, clamping all sample coordinates to the tile rect

It appears we can't really do that texture clamping currently, at least not easily, so we'll have to document that although we should do it, we don't. And while that limitation remains, we shouldn't be using this API with rectangles contained by the image rect, because Web content may depend on that sampling restriction).

::: gfx/layers/Layers.h
@@ +721,5 @@
> +   *
> +   * The interpretation of the source rect varies depending on
> +   * underlying layer type.  For ImageLayers and CanvasLayers, it
> +   * doesn't make sense to set a source rect not fully contained by
> +   * the bounds of their underlying images.  For ThebesLayers, thebes

It does make sense --- you'd just repeat gaps around the image.

@@ +736,5 @@
> +  {
> +    mUseTileSourceRect = aRect != nsnull;
> +    if (aRect) {
> +      mTileSourceRect = *aRect;
> +    }

It doesn't make sense to set an empty tiling source rect, so we could dispense with the boolean and just use an empty source rect to indicate that there's no tiling.

::: layout/base/nsDisplayList.cpp
@@ +1221,5 @@
> +  nsIntRect clip = nsIntRect(PRInt32(mFillRect.X()),
> +                             PRInt32(mFillRect.Y()),
> +                             PRInt32(mFillRect.Width()),
> +                             PRInt32(mFillRect.Height()));
> +  aLayer->SetClipRect(&clip);

Using clipping here isn't quite going to work since FrameLayerBuilder can adjust the transform on this layer and the clip rect won't be updated to match.
A good test case for this work is  It doesn't match the heuristics in bug 750598 (with good reason, the thin lines disappear sometimes in the tiled image).  This caused us to regress on tcheckerboard - tcheckerboard2 on is generally still fine though.
Attachment #620558 - Attachment is obsolete: true
Attachment #620558 - Flags: feedback?(roc)
Attached patch patch (obsolete) — Splinter Review
Attachment #621354 - Attachment is obsolete: true
Attachment #621895 - Flags: review?(roc)
Comment on attachment 621895 [details] [diff] [review]

Review of attachment 621895 [details] [diff] [review]:

You can split this patch up into at least three:
-- layer-system changes
-- nsDisplayCanvasBackground changes
-- nsDisplayList changes to use SetFillRect

We need to do something for D3D9/D3D10 here --- at least an assertion that fill rects aren't supported yet.

::: gfx/layers/ImageLayers.h
@@ +616,5 @@
> +   *
> +   * Define a fill rect that represents the extends of what we want to
> +   * render. If the image is smaller, it is tiled to cover the entire
> +   * fill rect.

Need to document that this is only used when mScaleMode is SCALE_NONE.

::: gfx/layers/basic/BasicLayers.cpp
@@ +812,5 @@
> +  {
> +    nsIntRect rect = GetFillRect();
> +    if (!rect.IsEmpty())
> +      return rect;
> +    return nsIntRect(0, 0, mSize.width, mSize.height);

Treating an empty fill rect specially isn't really a good way to go here. We could get into trouble if the fill rect gets rounded down to empty or something like that.

Instead, either every user of ImageLayers should call SetFillRect, or else we could do what we do for clipping and distinguish "no fill rect" from "has fill rect" explicitly.

::: layout/generic/nsCanvasFrame.cpp
@@ +239,5 @@
> +mozilla::LayerState
> +nsDisplayCanvasBackground::GetLayerState(nsDisplayListBuilder* aBuilder,
> +                                  LayerManager* aManager,
> +                                  const ContainerParameters& aParameters)

Fix indent
Blocks: 754542
No longer blocks: 754542
Depends on: 754542
Attached patch patch (obsolete) — Splinter Review
Attachment #621895 - Attachment is obsolete: true
Attachment #621895 - Flags: review?(roc)
Attachment #623463 - Flags: review?(roc)
Attached patch patch (obsolete) — Splinter Review
Attachment #623463 - Attachment is obsolete: true
Attachment #623463 - Flags: review?(roc)
Attachment #623466 - Attachment is patch: true
Comment on attachment 623466 [details] [diff] [review]

Lost a hunk splitting patches.
Attachment #623466 - Flags: review?(roc)
Attached patch Fixes video reftests (obsolete) — Splinter Review
Attachment #623466 - Attachment is obsolete: true
Attachment #623466 - Flags: review?(roc)
Attachment #625268 - Flags: review?(roc)
No longer depends on: 754542
Depends on: 754542
Assignee: gal → dzbarsky
No longer blocks: 749062
Blocks: 749062
Comment on attachment 625268 [details] [diff] [review]
Fixes video reftests

Review of attachment 625268 [details] [diff] [review]:

::: layout/base/nsDisplayList.cpp
@@ +1245,1 @@
>    aLayer->SetFillRect(rect);

This is a bit dodgy. we should just be passing mFillRect here.

@@ +1245,2 @@
>    aLayer->SetFillRect(rect);
> +  aLayer->SetVisibleRegion(rect);

Here too.
Attached patch PatchSplinter Review
You can only use the fillrect in nsDisplayList - the VideoFrame has a size, not a rect
Attachment #625268 - Attachment is obsolete: true
Attachment #625268 - Flags: review?(roc)
Attachment #627029 - Flags: review?(roc)
Comment on attachment 627029 [details] [diff] [review]

Review of attachment 627029 [details] [diff] [review]:

Doesn't the nsVideoFrame change belong in the patch that adds the SetFillRect API?

::: layout/base/nsDisplayList.cpp
@@ +1209,5 @@
>    transform.Scale(mDestRect.width/imageSize.width,
>                    mDestRect.height/imageSize.height);
>    aLayer->SetTransform(gfx3DMatrix::From2D(transform));
> +  aLayer->SetFillRect(mFillRect);
> +  aLayer->SetVisibleRegion(mFillRect);

mFillRect is a gfxRect (floating poin) so this won't really work. And I don't think supporting non-integer mFillRects is something we want to do in Layers.

Also, mFillRect needs to be in the pre-transform coordinate space. So you need to transform mFillRect back to tiled-image-space and then check if its edges lie on integer pixel boundaries (or very close to). if not, then I suggest we don't try to optimize this at all --- have TryOptimizeToImageLayer return false for that case.

Probably we should just compute the entire transform in TryOptimizeToImageLayer and store the transform and the extra rects in their own struct which we keep a pointer to in nsDisplayBackground, so we don't bloat nsDisplayBackground when we're doing this. You can allocate the struct from the nsDisplayListBuilder's arena.
roc, I am pretty confused by the instructions in comment 16. What exactly is the problem? FillRect and VisibleRect should be in the same coordinate space.
Yes, I think we're setting the visible rect incorrectly as well.
Attachment #627029 - Flags: review?(roc)
I won't be working on this any time soon.
Assignee: dzbarsky → nobody
Severity: normal → S3

Most of the code this patch touches isn't around anymore

Closed: 11 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.