373572 - Implement Tile filter

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Implement feTile filter

Comment 1

[Robert Longson [:longsonr]]

Attachment: implement tile filter

Comment 2

[tor]

This patch will need carioification given the recent changes to filters.

Would "GetSourceRegion" be a more self-documenting name than "GetReferencedRegion"?

My reading of the SVG specification is that the union (ie, the old behavior of the region tracking) is what should be tiled, not the whole area.  If that is the case, your changes to add a subregion type seem unnecessary.

You are copying individual pixels - it seems with a bit more fun modular math you could copy entire rows.

Comment 3

[Robert Longson [:longsonr]]

(In reply to comment #2)
> My reading of the SVG specification is that the union (ie, the old behavior of
> the region tracking) is what should be tiled, not the whole area.  If that is
> the case, your changes to add a subregion type seem unnecessary.

I'm going by http://www.w3.org/TR/SVG/filters.html#FilterPrimitiveSubRegion where it talks about feTile using the entire region and not the previous filter.

This is borne out by the test case 
http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-filters-tile-01-b.html
in which the tile has no attributes so it would not tile at all if it did not use the whole area as a source.

> 
> You are copying individual pixels - it seems with a bit more fun modular math
> you could copy entire rows.
> 

Would it be all right to do that as a follow up?

Comment 4

[tor]

(In reply to comment #3)
> (In reply to comment #2)
> > My reading of the SVG specification is that the union (ie, the old behavior of
> > the region tracking) is what should be tiled, not the whole area.  If that is
> > the case, your changes to add a subregion type seem unnecessary.
> 
> I'm going by http://www.w3.org/TR/SVG/filters.html#FilterPrimitiveSubRegion
> where it talks about feTile using the entire region and not the previous
> filter.

I hate the SVG specification.

> > You are copying individual pixels - it seems with a bit more fun modular math
> > you could copy entire rows.
> 
> Would it be all right to do that as a follow up?

Ok.

Comment 5

[Robert Longson [:longsonr]]

I intend to address the review comment to change "GetSourceRegion" to "GetReferencedRegion" on checkin unless you want to see the change now roc.

Comment 6

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

Comment on attachment 258411 [details] [diff] [review]
implement tile filter

+                                        DefaultSubregionType defaultSubregionType,

aDefaultSubregionType

+      memcpy(targetData + y * stride + 4 * x,
+             sourceData + tileY * stride + 4 * tileX,
+             4);

Don't use memcpy, use *(PRUint32*)... = *(PRUint32*)...;

Comment 7

[Robert Longson [:longsonr]]

Attachment: update to tip

Integrate with lighting filter changes.

Comment 8

[Robert Longson [:longsonr]]

(In reply to comment #7)
> Created an attachment (id=273966) [details]

Oh, and also address first paragraph in comment 2 i.e. s/GetReferencedRegion/GetSourceRegion/

And address sr (comment 6).

Comment 9

[tor]

Checked in for Robert.