1214889 - Incorrect edge lighting of content that touches the edge of the filter region for lighting filters in nightly

Status: NEW

(Core :: SVG, defect)

Description

[Tom Klein]

Attachment: testcase.svg

This is present in Nightly since bug 1203376 landed, for both software and D2D lighting filters.

Essentially what's happening is that the lighting region is getting padded by transparent black pixels, so if input content reaches the edge of the lighting region, a lighting highlight gets added along the edge between the content and the transparent black, and that shows up as a 1-pixel edge highlight inside the region.

(I'm pretty sure I ran this test when I was testing 1203376, but I think I was looking to make sure the lighting region was the correct size and missed the 1-pixel highlighting.)

Comment 1

[Tom Klein]

Attachment: Patch v1

Comment 2

[Tom Klein]

Attachment: lightingTests.html

Here are some extra tests.

Comment 3

[Tom Klein]

Attachment: Enlarged screenshot of test item 10.png

The tests all look good, but I do see some effects as in the screenshot here on Windows.  I assume that's just a pixel-snapping kind of issue?  (The output should be 50x50 but if you include the semi-green top and bottom edges in the screenshot it's 50x51.)

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8673909 [details] [diff] [review]
Patch v1

Review of attachment 8673909 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/FilterNodeSoftware.cpp
@@ +3339,5 @@
>  template<typename LightType, typename LightingType>
> +void
> +FilterNodeLightingSoftware<LightType, LightingType>::SetAttribute(uint32_t aIndex, const IntRect &aSourceRect)
> +{
> +  // D2D lighting filters need to save the source rectangle, but we don't, so

I don't think this is true. I think the software filter also needs to save the source rectangle. If you think it doesn't, can you convince me?

In the convolve filter, I remember I had to add the source rect attribute in order to make layout/reftests/svg/filters/feConvolveMatrix-2.svg pass (even with software filters). In that test, the primitive subregion was much larger than the SourceGraphic input surface. A similar argument can probably be made for lighting filters.

Comment 5

[Tom Klein]

Attachment: lightingTests v2.html

Added tests 14 and 15.

Comment 6

[Tom Klein]

Attachment: Patch v2

This version has two basic changes.

1) I changed the naming of FilterNodeSourceRectExtendD2D1 to FilterNodeRenderRectD2D1 - as I mention in bug 1215287 comment 0 item 2), it seems (to me) like when "source rect" is used at https://dxr.mozilla.org/mozilla-central/rev/e193b4da0a8c1025aa76a403c64663ff1cd41709/gfx/2d/FilterNodeD2D1.cpp#867, most of the uses read as though what is meant is the convolve primitive subregion (not the source rect, which is the primitive subregion of the input to the convolve filter).  And I think using the primitive subregion of the convolve filter is the correct thing there instead of using the primitive region of the input filter (so this is slightly awkward, since I'm suggesting we make the wording change here based on a change that I think will eventually be made in a different bug - but then the alternative of keeping the current sourceRect terminology doesn't make sense for the new lighting class...).

2) Based on the new tests 14 and 15 added in comment 5, I do now think software lighting filters need to know their primitive subregion.  When a lighting filter sends output to another filter, the code asks the lighting filter to give its output only for the primitive subregion of the other filter.  Every filter is wrapped in a crop filter to crop to its primitive subregion: https://dxr.mozilla.org/mozilla-central/rev/d374d16cbb251c9dac5af69f8e186e821ce82fe2/gfx/src/FilterSupport.cpp#1313
so before that request for output in a certain rectangle reaches the actual lighting filter the request rectangle first gets cropped to the primitive subregion of the lighting filter.  But then in certain cases (as in tests 14 and 15) the lighting filter needs to light at least a one pixel edge more than the region asked for (to get edge highlights correct), but *only* when that extra edge of pixel is inside the primitive subregion of the lighting filter, so I think the lighting filter still needs access to its primitive subregion for that.  (Actually I guess the same situation could theoretically arise if the lighting filter is the final output of the filter chain and invalidation asks DoRender for just part of the output of the lighting filter - in that case too we need to request at least one extra pixel edge around aRect, but only when that edge is contained in the primitive subregion (outside of the primitive subregion we need to DUPLICATE from the edge of the primitive subregion).)

Comment 7

[Tom Klein]

Attachment: lightingTests v3.html

Added an invalidation test (test 16) that tests the scenario theorized at the end of comment 6.  Patch v1 fails that test, but v2 passes.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8674704 [details] [diff] [review]
Patch v2

Review of attachment 8674704 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great.

Should we maybe limit the output of lighting FilterNodes to the render rect, or does it ever make sense to output something larger? If not, should we do the same for the convolve matrix filter? We could rename SOURCE_RECT to RENDER_RECT for that one, too.

::: gfx/2d/Filters.h
@@ +346,5 @@
>  
>    ATT_LIGHTING_COLOR,                       // Color
>    ATT_LIGHTING_SURFACE_SCALE,               // Float
>    ATT_LIGHTING_KERNEL_UNIT_LENGTH,          // Size
> +  ATT_LIGHTING_PRIMITIVE_SUBREGION,         // IntRect

At the Moz2D level, FilterNodes don't know anything about "primitives" or "primitive subregions". Can we call this RENDER_RECT instead?

Comment 9

[Tom Klein]

Attachment: Patch v3

Thanks for the review!

(In reply to Markus Stange [:mstange] from comment #8)
> Should we maybe limit the output of lighting FilterNodes to the render rect,
> or does it ever make sense to output something larger?

I think it would be an error to output something larger than the render rect (assuming render rect == primitive subregion), but I think the crop to primitive subregion mentioned in comment 6 2) that wraps every filter takes care of ensuring that we're not making that error: at https://dxr.mozilla.org/mozilla-central/rev/1a157155a4fe0074b3d03b54fe9e466472c2cd56/gfx/2d/FilterNodeSoftware.cpp#3115 when we call render on the lighting (or convolve) wrapped in a crop, the crop::render restricts the region to be rendered to be contained in the primitive subregion, and then it's just up to the lighting and convolve filters to not render outside of the aRect they're passed in ::render, and I think they both do that.  Does that sound good?

> We could rename SOURCE_RECT to
> RENDER_RECT for that one [convolve], too.

Can we wait for bug 1215287 to do that?  As this patch stands with respect to convolve, we're still passing the primitive subregion of the input filter to convolve as the render region, but that may not even intersect the primitive subregion of the convolve filter, so it seems a little misleading to refer to it as the render rectangle in software filters at this point (even though that's pretty much what wound up happening with D2D filters, because of the change I made to support lighting along with convolve in that case).  I can work on finishing 1215287 right away (if you agree that's the way convolve should work), so hopefully it won't make much difference either way...

I did change all "primitive subregion" references in lighting to "render rect" references instead - those are the only changes in this patch.

Rerequesting review just to make sure we're on the same page.

Comment 10

[Tom Klein]

Interdiff of v2 and v3: https://bugzilla.mozilla.org/attachment.cgi?oldid=8674704&action=interdiff&newid=8675901&headers=1

Comment 11

[Tom Klein]

Attachment: Patch v4

Sorry, I lost one s/ATT_LIGHTING_PRIMITIVE_SUBREGION/ATT_LIGHTING_RENDER_RECT/ change when I switched between windows and linux; that's the only change here.

Comment 12

[Tom Klein]

Interdiff of v2 and v4: https://bugzilla.mozilla.org/attachment.cgi?oldid=8674704&action=interdiff&newid=8675937&headers=1

Comment 13

[Tom Klein]

Just noticed I've been bitrotted by Bug 1207245, which landed yesterday - I'll post an updated patch tomorrow.

Comment 14

[Tom Klein]

Attachment: Patch v5

Bitrot update.

Comment 15

[Tom Klein]

Attachment: WIP convolve output fix

Another option from a different point of view :) (None of this has been fully tested yet.)

(In reply to Markus Stange [:mstange] from comment #8)
> Should we maybe limit the output of lighting FilterNodes to the render rect,
> or does it ever make sense to output something larger? If not, should we do
> the same for the convolve matrix filter?
If we keep the current convolve behavior of only convolving the primitive region of the input to convolve, then yes, I think we do need to do more to limit the output of convolve to just the input region (I think lighting is fine as is).  I threw this wip together to make that happen: in software, in convolve::DoRender, we have to output a rect of size aRect, but we're free to only draw non-transparent black on the portion of it we want, so that's what I did (I think - I haven't checked it carefully and I'm kind of surprised it seems to work); in D2D I added a crop to the output of the convolve filter to crop to mRenderRect in FilterNodeConvolveD2D1 (I'm not sure if I'd want to keep the structure this way, but in principle it works).

This seems to do what I'd expect on the tests in bug 1215287 bug 0, with one caveat.  In testcase 1 (and 2), the input to convolve is the sourceGraphic, and in that case we assign the source graphic a primitive subregion equal to the filter region: https://dxr.mozilla.org/mozilla-central/rev/daa7d98525e859d32a3b3e97101e129a897192a1/gfx/src/FilterSupport.cpp#1249 (I guess that's because the spec says that the primitive subregion of a given primitive defaults to the filter region when its input is SourceGraphic).  So in fact all along we should have been drawing output for that case in the 200x200 filter region and were not only because functions like FilterSupport::PostFilterExtentsForPrimitive weren't taking into account the case where convolve turns transparent into non-transparent (those fixes in this patch came from the patch in bug 1215287).

> At the Moz2D level, FilterNodes don't know anything about "primitives" or
> "primitive subregions". Can we call this RENDER_RECT instead?
Yes!  Sorry, I was hung up on thinking at the user level instead of the Moz2D level.  At the Moz2D level convolve can choose to convolve whatever region it wants, and currently we want that region to be the primitive subregion of its input, so I'm now fine with referring to that region at that level as the "render rect" (though it's still *slightly* confusing to have a different notion of render region higher up the chain).  So this patch includes those changes as well.

If you'd like to go in this direction just let me know and I'll work on finishing this version up.

Comment 16

[Tom Klein]

Hmm, actually maybe for convolve, using ATT_CONVOLVE_MATRIX_PROCESSING_RECT (or something) instead of ATT_CONVOLVE_MATRIX_RENDER_RECT might be less likely to cause confusion at that intermediate stage between user and Moz2D?

Comment 17

[Markus Stange [:mstange]]

(In reply to Tom Klein from comment #9)
> Created attachment 8675901 [details] [diff] [review]
> Patch v3
> 
> Thanks for the review!
> 
> (In reply to Markus Stange [:mstange] from comment #8)
> > Should we maybe limit the output of lighting FilterNodes to the render rect,
> > or does it ever make sense to output something larger?
> 
> I think it would be an error to output something larger than the render rect
> (assuming render rect == primitive subregion), but I think the crop to
> primitive subregion mentioned in comment 6 2) that wraps every filter takes
> care of ensuring that we're not making that error

Right, exactly. The crop takes care of SVG filter correctness. My question was more about the Moz2D FilterNode API, which in theory could be used in other scenarios than just rendering SVG filters. But those other consumers don't exist yet, so it's a little hard to guess what they would expect.

> > We could rename SOURCE_RECT to
> > RENDER_RECT for that one [convolve], too.
> 
> Can we wait for bug 1215287 to do that?

Oh, sure, absolutely.

> (if you agree that's the way convolve should work)

I do agree. I think.

I need to read the rest of your comments again with a clearer head tomorrow before I can answer them.

Comment 18

[Tom Klein]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c36eeb6aa524

Comment 19

[Tom Klein]

Markus, any chance of getting to the v5 review before the next release?

As I suggested over email, if the bug 1215287 work isn't going to make it into this release, then I think it would be good to add to the v5 patch here the removal of the if-check at https://hg.mozilla.org/mozilla-central/rev/2873ffc036ef#l3.32 since it turns out that check isn't actually in line with the way convolve currently works (so it regresses convolve - see for example tests 2 and 3 in bug 1215287 comment 2).  (Or maybe *just* undo the if-check if v5 can't get reviewed before the release.)

Let me know if there's anything I can do to make the review easier - split up the patch, hop on irc to discuss it, start the discussion from scratch (it got a little unwieldy I think, going back and forth between two bugs)...

Comment 20

[Robert Longson [:longsonr]]

has been waiting for a review for some time now.

Comment 21

[Tom Klein]

Maybe C.J. will be interested in picking some of these up. :)