support tiled image background layers

NEW
Unassigned

Status

()

Core
Graphics: Layers
6 years ago
5 years ago

People

(Reporter: gal, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
This allows us to support a wider range of background image scaling on the GPU.
(Reporter)

Comment 1

6 years ago
Created attachment 620324 [details] [diff] [review]
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
(Reporter)

Updated

6 years ago
Blocks: 749062
(Reporter)

Comment 2

6 years ago
Created attachment 620335 [details] [diff] [review]
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.
Attachment #620324 - Attachment is obsolete: true
(Reporter)

Comment 3

6 years ago
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
(Reporter)

Comment 4

6 years ago
Created attachment 620558 [details] [diff] [review]
Support tiled background image layers
Assignee: nobody → gal
Attachment #620335 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #620558 - Flags: feedback?(roc)
(Reporter)

Updated

6 years ago
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.

Comment 6

6 years ago
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.
(Reporter)

Comment 7

6 years ago
Created attachment 621354 [details] [diff] [review]
WIP. Shows artifacts on the background image of bugzilla
(Reporter)

Updated

6 years ago
Attachment #620558 - Attachment is obsolete: true
Attachment #620558 - Flags: feedback?(roc)
(Reporter)

Comment 8

6 years ago
Created attachment 621895 [details] [diff] [review]
patch
Attachment #621354 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Attachment #621895 - Flags: review?(roc)
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
(Reporter)

Updated

6 years ago
Blocks: 754542
(Reporter)

Updated

6 years ago
No longer blocks: 754542
Depends on: 754542
(Reporter)

Comment 10

6 years ago
Created attachment 623463 [details] [diff] [review]
patch
Attachment #621895 - Attachment is obsolete: true
Attachment #621895 - Flags: review?(roc)
(Reporter)

Updated

6 years ago
Attachment #623463 - Flags: review?(roc)
(Reporter)

Comment 11

6 years ago
Created attachment 623466 [details] [diff] [review]
patch
Attachment #623463 - Attachment is obsolete: true
Attachment #623463 - Flags: review?(roc)
(Reporter)

Updated

6 years ago
Attachment #623466 - Attachment is patch: true
(Reporter)

Comment 12

6 years ago
Comment on attachment 623466 [details] [diff] [review]
patch

Lost a hunk splitting patches.
Attachment #623466 - Flags: review?(roc)
Created attachment 625268 [details] [diff] [review]
Fixes video reftests
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.
Created attachment 627029 [details] [diff] [review]
Patch

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]
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.
(Reporter)

Comment 17

6 years ago
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.

Updated

6 years ago
Duplicate of this bug: 795250
Attachment #627029 - Flags: review?(roc)
I won't be working on this any time soon.
Assignee: dzbarsky → nobody
You need to log in before you can comment on or make changes to this bug.