Closed Bug 373572 Opened 14 years ago Closed 14 years ago

Implement Tile filter

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 1 obsolete file)

Implement feTile filter
Attached patch implement tile filter (obsolete) — Splinter Review
Attachment #258411 - Flags: review?(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.
Blocks: 311029
(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?

(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.
Attachment #258411 - Flags: review?(tor) → review+
Attachment #258411 - Flags: superreview?(roc)
I intend to address the review comment to change "GetSourceRegion" to "GetReferencedRegion" on checkin unless you want to see the change now roc.

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*)...;
Attachment #258411 - Flags: superreview?(roc) → superreview+
Attached patch update to tipSplinter Review
Integrate with lighting filter changes.
Attachment #258411 - Attachment is obsolete: true
(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).


Checked in for Robert.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Blocks: 389814
You need to log in before you can comment on or make changes to this bug.