Implement (basic) tiling of (basic) ImageLayers

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b4+)

Details

Attachments

(5 attachments, 4 obsolete attachments)

3.13 KB, patch
Details | Diff | Splinter Review
3.90 KB, patch
Details | Diff | Splinter Review
4.60 KB, patch
Details | Diff | Splinter Review
11.16 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.27 KB, patch
cjones
: review+
Details | Diff | Splinter Review
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.
Blocks a blocker.
tracking-fennec: --- → ?
Attachment #505510 - Flags: superreview?(roc)
Attachment #505510 - Flags: review?
This patch just bails when the tile API would require a temporary surface.  We don't need that support yet.
Vlad, since roc is going on vacation later today, would you be available to steal his reviews?
(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.
Attachment #505510 - Flags: review? → review?(bas.schouten)
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.)
Attachment #505510 - Flags: superreview?(roc) → superreview?(vladimir)
Attachment #505512 - Flags: review?(roc) → review?(vladimir)
Attachment #505510 - Flags: review?(bas.schouten) → review+
blocks a blocker
tracking-fennec: ? → 2.0b4+
Attachment #505510 - Flags: superreview?(vladimir) → superreview?(roc)
Attachment #505511 - Flags: superreview?(vladimir) → superreview+
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 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);
(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?
(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?
(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.)
Attachment #505512 - Attachment is obsolete: true
Attachment #506474 - Flags: superreview?(vladimir)
Attachment #505512 - Flags: review?(vladimir)
Going ahead and posting while API is being discussed.
Attachment #506475 - Flags: review?(vladimir)
(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.
Attachment #506474 - Flags: superreview?(vladimir) → superreview+
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
Attachment #506475 - Flags: review?(vladimir) → review+
(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.
Attachment #505510 - Attachment is obsolete: true
Attachment #506579 - Flags: superreview?(roc)
Attachment #505510 - Flags: superreview?(roc)
Attachment #506579 - Flags: superreview?(roc) → superreview+
Attachment #506475 - Attachment is obsolete: true
+      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.
(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.
(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 :).
(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 :-)
Removed useless code and added guard assertions.
Attachment #506616 - Attachment is obsolete: true
Attachment #506631 - Flags: review?(roc)
Attachment #506616 - Flags: review?(roc)
(Note that the guard assertions are going to break fennec-on-GL for the time being.)
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!
Depends on: 628948
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 31

9 years ago
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>.  ;-)
Depends on: 629100
You need to log in before you can comment on or make changes to this bug.