support tiled image layers

NEW
Unassigned

Status

()

6 years ago
5 years ago

People

(Reporter: gal, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

6 years ago
Add support for tiled image layers
(Reporter)

Updated

6 years ago
Blocks: 751187
No longer depends on: 751187
(Reporter)

Comment 1

6 years ago
Created attachment 623459 [details] [diff] [review]
patch
Assignee: nobody → gal
(Reporter)

Comment 2

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

Updated

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

Comment 3

6 years ago
Bas, Joe, Jeff, can you guys help out with D3D?
(Reporter)

Comment 4

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

Comment 5

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

Replaced the wrong patch. Sorry for the churn. Review request still valid.
Attachment #623462 - Attachment is obsolete: false
Attachment #623462 - Flags: review?(roc)
(Reporter)

Updated

6 years ago
Attachment #623465 - Attachment is obsolete: true
(Reporter)

Comment 7

6 years ago
Which part? Documenting it? (I thought the assert did, happy to spell it out)
(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.
(Reporter)

Comment 9

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

Comment 10

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

Updated

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

Updated

6 years ago
Whiteboard: [autoland]
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!
(Reporter)

Comment 12

6 years ago
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 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.
Whiteboard: [autoland]
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.
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.
Created attachment 625266 [details] [diff] [review]
Patch

I'm still not quite sure what you meant by the x,y tiles.
Attachment #623539 - Attachment is obsolete: true
Attachment #623539 - Flags: review?(roc)
Attachment #625266 - Flags: review?(roc)
Assignee: gal → dzbarsky
No longer blocks: 749062, 751187
Blocks: 749062, 751187
(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 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.
Created attachment 627098 [details] [diff] [review]
Patch
Attachment #625266 - Attachment is obsolete: true
Attachment #625266 - Flags: review?(roc)
Attachment #627098 - Flags: review?(roc)
Created attachment 627312 [details] [diff] [review]
PAtch

forgot to qref
Attachment #627098 - Attachment is obsolete: true
Attachment #627098 - Flags: review?(roc)
Attachment #627312 - Flags: review?(roc)
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.
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));
  }
}
(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.
Attachment #627312 - 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.