627273 - Implement (basic) tiling of (basic) ImageLayers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The first and possible only use case for this API is bug 593310, where gecko has full control over image size, tiling rect, visible region, etc., so we can keep things simple and fast initially.

Approximate spec
-----
the new parameter should be a "tiling rectangle". The image is
conceptually tiled to fill the rectangle before being
clipped/transformed/sampled.
-----

Additionally this API needs to be efficiently implementable in GL, for simple cases.  With power-of-two-sized images we can use GL_REPEAT everywhere and win.

I think this API implies that the visible region is an implicit clip on the (transformed) tiling rect.  I'm not sure in which situations clipping would require an intermediate surface.  I'm not sure what should happen if the (transformed) tiling rect doesn't contain the visible region (EXTEND_NONE-like behavior?).  Bas raised the issue of seaming.  I don't care initially about these issues.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Blocks a blocker.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: API for tiling ImageLayers

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Add some gfx path helpers

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Basic impl of ImageLayer tiling for basic layers

This patch just bails when the tile API would require a temporary surface.  We don't need that support yet.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Vlad, since roc is going on vacation later today, would you be available to steal his reviews?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> Created attachment 505512 [details] [diff] [review]
> part 3: Basic impl of ImageLayer tiling for basic layers
> 
> This patch just bails when the tile API would require a temporary surface.  We
> don't need that support yet.

s/would require/might require/, depending on how we resolve unknowns in comment 0.  At any rate, we don't need this for bug 593310.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Changing sr's to vlad in case he gets time to review before beta4 freeze.  (Went with roc initially because he and I discussed the API in bug 593310.)

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

blocks a blocker

Comment 9

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

We'll want more general tiling support to render repeated -moz-element and other things.

How about a property on all layers which consists of just a single (optional) "tile source rect", where the semantics are that the "tile source rect" subrectangle is extracted and tiled to fill the plane before transformation and clipping?

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 505512 [details] [diff] [review]
part 3: Basic impl of ImageLayer tiling for basic layers


>+  if (!aTile) {
>+    aContext->NewPath();
>+    // No need to snap here; our transform has already taken care of it.
>+    // XXX true for arbitrary regions?  Don't care yet though
>+    gfxUtils::PathFromRegion(aContext, aVisible);
>+    aPattern->SetExtend(extend);
>+    aContext->SetPattern(aPattern);


>+    if (aOpacity != 1.0) {
>+      gfxContextAutoSaveRestore saveRestore(aContext);
>+      aContext->Clip();
>+      aContext->Paint(aOpacity);
>+    } else {
>+      aContext->Fill();
>+    }

You know, we do this pattern all over the place.  Can you add a gfxContext function called FillWithOpacity or something that does this?  That way if Cairo ever gets this operation we have one place to change it as well.

>+  } else if (aTile && aTile->Contains(aVisible.GetBounds())) {

Is Contains() the right check?  Pretty sure this means "only if the visible bounds are completely inside this tile", which I don't think is what you want.  aVisible.Intersects(aTile) ? Or maybe aTile->Intersects(aVisible.GetBounds()) if you don't want to do extra work for complex regions..

>+    gfxContextAutoSaveRestore saveRestore(aContext);
>+
>+    gfxUtils::ClipToRegion(aContext, aVisible);
>+
>+    aContext->Translate(gfxPoint(aTile->x, aTile->y));
>+
>+    aContext->NewPath();
>+    aContext->Rectangle(gfxRect(0, 0, aTile->width, aTile->height));
>+
>+    aPattern->SetExtend(gfxPattern::EXTEND_REPEAT);
>+    aContext->SetPattern(aPattern);
>+    if (aOpacity != 1.0) {
>+      gfxContextAutoSaveRestore saveRestore(aContext);

No need for this -- you already save/restored one block higher.  (But, I guess if you replace this with a FillWithOpacity call, then it'll remain anyway :-)

>+      aContext->Clip();
>+      aContext->Paint(aOpacity);
>+    } else {
>+      aContext->Fill();
>+    }
>+  } else {
>+    NS_WARNING("Cowardly refusing to create a temporary surface for tiling");

... but maybe i'm confused about the contains bit, or this interface is very confusing (especially the usage of the term "tile").  A tile is a small piece that's repeated; here you seem to be using aTile as the rectangle that's supposed to be covered by tiling a (smaller) image?  If so, then please s/aTile/aVirtualImageRect/g or similar to remove a lot of the confusion.

>+  }    
>+
>+  // Reset extend mode for callers that need to reuse the pattern
>   aPattern->SetExtend(extend);

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #9)
> How about a property on all layers which consists of just a single (optional)
> "tile source rect", where the semantics are that the "tile source rect"
> subrectangle is extracted and tiled to fill the plane before transformation and
> clipping?

How would that work if the source rect covers a non-rectangular valid region?  Would the plane would be defined by the visible region?  What happens there if the visible region is non-rectangular?  Use the plane defined by its bounds?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #10)
> You know, we do this pattern all over the place.  Can you add a gfxContext
> function called FillWithOpacity or something that does this?

Sure
 
> >+  } else if (aTile && aTile->Contains(aVisible.GetBounds())) {
> 
> Is Contains() the right check?  Pretty sure this means "only if the visible
> bounds are completely inside this tile", which I don't think is what you want. 
> aVisible.Intersects(aTile) ? Or maybe aTile->Intersects(aVisible.GetBounds())
> if you don't want to do extra work for complex regions..
>
[snip]
>
> ... but maybe i'm confused about the contains bit, or this interface is very
> confusing (especially the usage of the term "tile").  A tile is a small piece
> that's repeated; here you seem to be using aTile as the rectangle that's
> supposed to be covered by tiling a (smaller) image?

Yes, the layers API uses "TilingRect", but this local is a bad shortening of that.

>  If so, then please
> s/aTile/aVirtualImageRect/g or similar to remove a lot of the confusion.
> 

Sure, would s/aTile/aTilingRect/, as this originally would have been named, make things clearer?

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #11)
> How would that work if the source rect covers a non-rectangular valid region? 
> Would the plane would be defined by the visible region?  What happens there if
> the visible region is non-rectangular?  Use the plane defined by its bounds?

(Sorry if this was unclear, but I was asking two separate questions about non-rectangular valid regions and non-rectangular visible regions.)

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 3: Add gfxContext::FillWithOpacity

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Basic impl of ImageLayer tiling for basic layers

Going ahead and posting while API is being discussed.

Comment 16

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

(In reply to comment #11)
> (In reply to comment #9)
> > How about a property on all layers which consists of just a single (optional)
> > "tile source rect", where the semantics are that the "tile source rect"
> > subrectangle is extracted and tiled to fill the plane before transformation and
> > clipping?
> 
> How would that work if the source rect covers a non-rectangular valid region?
> Would the plane would be defined by the visible region?  What happens there if
> the visible region is non-rectangular?  Use the plane defined by its bounds?

Yes, you need to tile to fill the visible region, so tiling over its bounds would work, or you could be smarter if you wanted to.

It's up to the layer system to ensure valid regions as large as necessary, by issuing ThebesLayer paint callbacks. For tiled ThebesLayers, making the visible region all valid (like we do now) would be sufficient, although actually an overapproximation to what's truly needed.

For tiled ContainerLayers, the visible regions set on the children would need contain all areas needed by the parent --- that's basically what we have today.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 506475 [details] [diff] [review]
part 4: Basic impl of ImageLayer tiling for basic layers

works for me, pending API discussion with roc though

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #16)
> It's up to the layer system to ensure valid regions as large as necessary, by
> issuing ThebesLayer paint callbacks. For tiled ThebesLayers, making the visible
> region all valid (like we do now) would be sufficient, although actually an
> overapproximation to what's truly needed.
> 
> For tiled ContainerLayers, the visible regions set on the children would need
> contain all areas needed by the parent --- that's basically what we have today.

There are some impl details I'm not clear on in the general case, but this API works for bug 593310.

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: API for tiling Layers, v2

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Basic impl of ImageLayer tiling for basic layers, v3

Updated for new API.

Comment 21

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

+      NS_WARNING("Cowardly refusing to create a temporary surface for tiling");

NS_ABORT_IF_FALSE I think. Also, all the unimplemented stuff should be documented in Layers.h. Also, the other layer managers should be update to ABORT_IF_FALSE if tiled rects are used.

+      // Move the source-rect top-left to user-space 0, 0 in
+      // preparation for tiling outwards
+      gfxPoint offset = -gfxPoint(aTileSourceRect->x, aTileSourceRect->y);
+      aContext->Translate(offset);

Why are you doing this, since you've previously checked that aTileSourceRect->x/y are 0?

Otherwise looks OK.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #21)
> +      NS_WARNING("Cowardly refusing to create a temporary surface for
> tiling");
> 
> NS_ABORT_IF_FALSE I think. Also, all the unimplemented stuff should be
> documented in Layers.h. 

OK.

> Also, the other layer managers should be update to
> ABORT_IF_FALSE if tiled rects are used.

For ImageLayers or all layer types?  Neither excites me much :(.

> 
> +      // Move the source-rect top-left to user-space 0, 0 in
> +      // preparation for tiling outwards
> +      gfxPoint offset = -gfxPoint(aTileSourceRect->x, aTileSourceRect->y);
> +      aContext->Translate(offset);
> 
> Why are you doing this, since you've previously checked that
> aTileSourceRect->x/y are 0?

Less to change if/when this code is generalized.  I'll take this out though.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #22)
> (In reply to comment #21)
> > Why are you doing this, since you've previously checked that
> > aTileSourceRect->x/y are 0?
> 
> Less to change if/when this code is generalized.  I'll take this out though.

Nm, this is just dumb.  Disregard.  It's gone regardless :).

Comment 24

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

(In reply to comment #22)
> > Also, the other layer managers should be update to
> > ABORT_IF_FALSE if tiled rects are used.
> 
> For ImageLayers or all layer types?  Neither excites me much :(.

All, but it's not that hard. Do it on the root layer and the children of all ContainerLayers, so that's maybe 8 places.

I really don't want to have unimplemented functionality that people can silently trip over.

> > +      // Move the source-rect top-left to user-space 0, 0 in
> > +      // preparation for tiling outwards
> > +      gfxPoint offset = -gfxPoint(aTileSourceRect->x, aTileSourceRect->y);
> > +      aContext->Translate(offset);
> > 
> > Why are you doing this, since you've previously checked that
> > aTileSourceRect->x/y are 0?
> 
> Less to change if/when this code is generalized.  I'll take this out though.

Yeah, let's not have useless code lying around :-)

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 4: Basic impl of ImageLayer tiling for basic layers, v4

Removed useless code and added guard assertions.

Comment 26

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Note that the guard assertions are going to break fennec-on-GL for the time being.)

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This (or much more improbably, bug 593310) is failing on tryserver ... but only on windows, but both across d2d+d3d/non-d2d+non-d3d.  Bizarre!

Comment 28

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/656ed06c9a10
http://hg.mozilla.org/mozilla-central/rev/3bdbbd75eb04
http://hg.mozilla.org/mozilla-central/rev/da677975bab1
http://hg.mozilla.org/mozilla-central/rev/73bfa3627d0c

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The fix for windows reftests broke bug 593310, because the fix undid repeating the source image into a larger visible region.  The patch fixes the incorrect fix.

http://hg.mozilla.org/mozilla-central/rev/657f6dac1e0c

Comment 30

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Bustage fix, exported

roc r+'d on IRC.

Comment 31

[(no longer active)]

Fix for fix for fix for fix for fix for fix...  oh, sorry, got distracted, let's call it the last patch.  It landed as <http://hg.mozilla.org/mozilla-central/rev/0e42a4b9e952>.  ;-)