754542 - support tiled image layers

Status: RESOLVED INVALID

(Core :: Graphics: Layers, defect)

Description

[Andreas Gal :gal]

Add support for tiled image layers

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Andreas Gal :gal]

Attachment: patch

Comment 3

[Andreas Gal :gal]

Bas, Joe, Jeff, can you guys help out with D3D?

Comment 4

[Andreas Gal :gal]

Attachment: patch

Comment 5

[Andreas Gal :gal]

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

Replaced the wrong patch. Sorry for the churn. Review request still valid.

Comment 6

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

https://bugzilla.mozilla.org/show_bug.cgi?id=751187#c9 still applies to this patch.

Comment 7

[Andreas Gal :gal]

Which part? Documenting it? (I thought the assert did, happy to spell it out)

Comment 8

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

(In reply to Andreas Gal :gal from comment #7)
> Which part? Documenting it? (I thought the assert did, happy to spell it out)

Document in the comment for SetFillRect.

Also, since you're requiring all callers to call SetFillRect, better document that the default fill rect is empty so this must be called in order to render anything.

Comment 9

[Andreas Gal :gal]

Attachment: patch

Comment 10

[Andreas Gal :gal]

Attachment: patch

Comment 11

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

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

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

::: gfx/layers/ImageLayers.h
@@ +619,5 @@
> +   * render. If the image is smaller, it is tiled to cover the entire
> +   * fill rect. The fill rect is empty for newly created layers, so
> +   * calling SetFillRect is not optional (without it nothing will be
> +   * rendered). Tiling is not supported when a scale mode is set
> +   * using SetScaleToSize (see above).

Always having a fillrect seems like the right thing, but since we're going to always have a fillrect, we need to work out the interaction between fillrects, asynchronous image changes, and SetScaleToSize.

Currently you're requiring the fillrect to be set to the size of the image. This isn't good when the image is being changed asynchronously, because then layout can't keep the fillrect in sync with image size changes.

I think the best approach is to get rid of mScaleToSize and say that when the scale mode is not SCALE_NONE, the image is scaled to fill the fillrect instead of being tiled. SetScaleToSize would change to just SetScaleMode.

When we asynchronously change the image we should not use SCALE_NONE since we don't want an async image change to temporarily do weird tiling. That means we need a SetScaleMode call in nsVideoFrame::BuildLayer. SCALE_STRETCH isn't right but it will do for now. Except that it's not implemented on GL and e10s yet!

Comment 12

[Andreas Gal :gal]

Can we land this patch and do that in a follow-up? I think I can implement stretch for GL. I need help with e10s though, or a roadmap.

Comment 13

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

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

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +994,5 @@
>    // No need to snap here; our transform has already taken care of it.
>    // XXX true for arbitrary regions?  Don't care yet though
> +
> +  nsIntRegion region(aVisible);
> +  region.And(region, aFillRect);

More efficient:
nsIntRegion region;
region.And(aVisible, aFillRect);

@@ +997,5 @@
> +  nsIntRegion region(aVisible);
> +  region.And(region, aFillRect);
> +  aContext->NewPath();
> +  gfxUtils::PathFromRegion(aContext, region);
> +  aPattern->SetExtend(region.GetBounds().IsEqualInterior(aFillRect) ? extend : gfxPattern::EXTEND_REPEAT);

region.Contains(aFillRect) I think

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +365,5 @@
>      GLContext::RectTriangles triangleBuffer;
>  
> +    // tesselate the visible region with tiles of image size
> +    for (int y = rect.y; y < rect.y + rect.height; y += iheight) {
> +      for (int x = rect.x; x < rect.x + rect.width; x += iwidth) {

rect.YMost(), rect.XMost()

@@ +374,5 @@
> +        if (visible.IsEmpty()) {
> +          continue;
> +        }
> +        nsIntRegionRectIterator iter(visible);
> +        while (const nsIntRect *r = iter.Next()) {

This is not particularly efficient. It would be better to iterate over the x and y tiles and just add one rect per tile, and I don't think it would be more complicated.

Comment 14

[David Zbarsky (:dzbarsky)]

This fails the following reftests on Mac (a few others on Linux, but they're also video tests so it may be the same cause).  The issue is that the video and controls aren't displayed.

REFTEST TEST-UNEXPECTED-FAIL | file:///Users/dzbarsky/mozilla/src/layout/reftests/ogg-video/aspect-ratio-1a.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/dzbarsky/mozilla/src/layout/reftests/ogg-video/aspect-ratio-1b.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/dzbarsky/mozilla/src/layout/reftests/ogg-video/aspect-ratio-2a.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/dzbarsky/mozilla/src/layout/reftests/ogg-video/aspect-ratio-2b.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/dzbarsky/mozilla/src/layout/reftests/ogg-video/basic-1.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/dzbarsky/mozilla/src/layout/reftests/ogg-video/offset-1.xhtml | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///Users/dzbarsky/mozilla/src/layout/reftests/ogg-video/zoomed-1.xhtml | image comparison (==)

We start in nsSVGForeignObjectFrame::PaintSVG and call into nsLayoutUtils::PaintFrame, which calls nsDisplayList::PaintRoot, which calls nsDisplayList::PaintForFrame and end up in FrameLayerBuilder::BuildContainerLayerFor, at which point I'm not sure what goes wrong.

Comment 15

[David Zbarsky (:dzbarsky)]

Nevermind, figured it out.  The issue was that there was no call to SetFillRect in nsVideoFrame::BuildLayer, so nothing was being displayed.  This seems like a footgun, I wonder if we can do something about it.

Comment 16

[David Zbarsky (:dzbarsky)]

Attachment: Patch

I'm still not quite sure what you meant by the x,y tiles.

Comment 17

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

(In reply to David Zbarsky from comment #15)
> Nevermind, figured it out.  The issue was that there was no call to
> SetFillRect in nsVideoFrame::BuildLayer, so nothing was being displayed. 
> This seems like a footgun, I wonder if we can do something about it.

One option would be to assert that the visible region is contained in the fillrect. In fact I like that idea; let's require it.

Comment 18

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

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

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

::: gfx/layers/ImageLayers.h
@@ +603,5 @@
>    void SetFilter(gfxPattern::GraphicsFilter aFilter) { mFilter = aFilter; }
>  
>    /**
>     * CONSTRUCTION PHASE ONLY
>     * Set the size to scale the image to and the mode at which to scale.

This comment needs to change to reflect that the image is scaled to fill mFillRect.

Also, you probably need to update the comment for ScaleMode as well.

::: gfx/layers/basic/BasicLayers.cpp
@@ +961,5 @@
>    }
>  
> +  NS_ASSERTION(mScaleMode == SCALE_NONE || (mFillRect.width == size.width &&
> +                                            mFillRect.height == size.height),
> +               "tiling of scaled image layers not supported");

This assert doesn't make sense anymore

@@ +1006,5 @@
>    // No need to snap here; our transform has already taken care of it.
>    // XXX true for arbitrary regions?  Don't care yet though
> +
> +  nsIntRegion region;
> +  region.And(aVisible, aFillRect);

If we require that the visible region be contained by the fillrect, you don't have to do this And.

::: gfx/layers/d3d10/ImageLayerD3D10.cpp
@@ +199,5 @@
> +  gfxIntSize size = mScaleMode == SCALE_NONE ? image->GetSize() : mFillRect.Size();
> +
> +  // We don't support tiling yet.
> +  NS_ASSERTION(size.width == mFillRect.width && size.height == mFillRect.height,
> +               "tiling not supported for D3D10");

you sould also assert that mFillRect is at 0,0. So I think mFillRect.IsEqualInterior(nsIntRect(0,0,size.width, size.height)).

::: gfx/layers/d3d9/ImageLayerD3D9.cpp
@@ +391,5 @@
> +  gfxIntSize size = mScaleMode == SCALE_NONE ? image->GetSize() : mFillRect.Size();
> +
> +  // We don't support tiling yet.
> +  NS_ASSERTION(size.width == mFillRect.width && size.height == mFillRect.height,
> +               "tiling not supported for D3D9");

same as above

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +381,5 @@
> +          triangleBuffer.addRect(r->x, r->y,
> +                                 r->x + r->width, r->y + r->height,
> +                                 tex_offset_u, tex_offset_v,
> +                                 tex_offset_u + float(r->width) / float(iwidth),
> +                                 tex_offset_v + float(r->height) / float(iheight));

So if iwidth is 10 and rect.x is 5, rect.XMost() is 15, this will do one addRect call and the horizontal texture coordinates will be 0.5, 1.5. Sampling at horizontal texture coordinates > 1.0 will be clamped to 1.0, so between x=10 and x=15 we'll just draw the color of the image at x=10. This is wrong.

Instead, we need an addRect call that adds a rect for x=5 to x=10, and another addRect all adding a rect for x=10 to x=15, since those need separate copies of the image.

Comment 19

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 20

[David Zbarsky (:dzbarsky)]

Attachment: PAtch

forgot to qref

Comment 21

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

Comment on attachment 627312 [details] [diff] [review]
PAtch

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

We need the nsVideoFrame change from bug 751187 in this patch.

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +341,5 @@
> +    PRUint32 bottomN = PR_MAX(0, ceil((rect.YMost() - visibleBounds.YMost()) / iheight));
> +
> +    // tesselate the visible region with tiles of image size
> +    for (int y = rect.y + iheight * topN; y < rect.YMost() - iheight * bottomN; y += iheight) {
> +      for (int x = rect.x + iwidth * leftN; x < rect.XMost() - iwidth * rightN; x += iwidth) {

This is still wrong. x and y must always be multiples of iwidth/iheight respectively, and you're not guaranteeing that.

Comment 22

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

I think we need something like this:

nsIntRegion visible;
visible.And(mVisibleRegion, mFillRect);

nsIntRect tiles = visible.GetBounds();
tiles.ScaleInverseRoundOut(iwidth, iheight);
nsIntRect tileBounds = tiles;
tileBounds.ScaleRoundOut(iwidth, iheight);

for (PRInt32 y = tileBounds.y; y < tileBounds.YMost(); y += iheight) {
  for (PRInt32 x = tileBounds.x; x < tileBounds.XMost(); x += iwidth) {
    // 'r' is the part of this image tile that we need to fill
    nsIntRect r = nsIntRect(x, y, iwidth, iheight).Intersect(mFillRect);
    triangleBuffer.addRect(r.x, r.y, r.XMost(), r.YMost(),
                           GLfloat(r.x - x)/iwidth, GLfloat(r.y - y)/iheight,
                           GLfloat(r.XMost() - x)/iwidth, GLfloat(r.YMost() - y)/iheight));
  }
}

Comment 23

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> nsIntRect tiles = visible.GetBounds();
> tiles.ScaleInverseRoundOut(iwidth, iheight);
> nsIntRect tileBounds = tiles;
> tileBounds.ScaleRoundOut(iwidth, iheight);

This isn't quite right. The second ScaleRoundOut shouldn't go through floating point and round out. Instead it should be all-integer arithmetic and not round at all. Probably nsIntRect should have a Scale(PRInt32 aScaleX, PRInt32 aScaleY) parameter.

Comment 24

[David Zbarsky (:dzbarsky)]

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

Comment 25

[Gregory Pappas [:gregp]]

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