751187 - support tiled image background layers

Status: RESOLVED INVALID

(Core :: Graphics: Layers, defect)

Description

[Andreas Gal :gal]

This allows us to support a wider range of background image scaling on the GPU.

Comment 1

[Andreas Gal :gal]

Attachment: patch

A couple things are still missing:
- a basic test case works but CNN.com 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

Comment 2

[Andreas Gal :gal]

Attachment: patch

This fixes the cnn.com 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.

Comment 3

[Andreas Gal :gal]

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 cnn.com. 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

Comment 4

[Andreas Gal :gal]

Attachment: Support tiled background image layers

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[JP Rosevear [:jpr]]

A good test case for this work is timecube.com.  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 cnn.com is generally still fine though.

Comment 7

[Andreas Gal :gal]

Attachment: WIP. Shows artifacts on the background image of bugzilla

Comment 8

[Andreas Gal :gal]

Attachment: patch

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 621895 [details] [diff] [review]
patch

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 @@
> +   * CONSTRUCTION PHASE ONLY
> +   *
> +   * 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

Comment 10

[Andreas Gal :gal]

Attachment: patch

Comment 11

[Andreas Gal :gal]

Attachment: patch

Comment 12

[Andreas Gal :gal]

Comment on attachment 623466 [details] [diff] [review]
patch

Lost a hunk splitting patches.

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: Fixes video reftests

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[David Zbarsky (:dzbarsky)]

Attachment: Patch

You can only use the fillrect in nsDisplayList - the VideoFrame has a size, not a rect

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 627029 [details] [diff] [review]
Patch

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.

Comment 17

[Andreas Gal :gal]

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, I think we're setting the visible rect incorrectly as well.

Comment 20

[David Zbarsky (:dzbarsky)]

I won't be working on this any time soon.

Comment 21

[Gregory Pappas [:gregp]]

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