1203376 - lighting filter region doesn't render beyond bounding box

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Tom Klein]

Attachment: filterRegionTest.svg

This was mentioned in bug 993934.

For both diffuse and specular lighting filters (and for both software and D2D filters), the filter region seems to get clipped by x="0%", y="0%", width="100%", height="100%".

Comment 1

[Tom Klein]

Attachment: Patch v1

This makes the filter region for lighting filters behave similarly to feColorMatrix, for example, and matches the behavior of Chrome and IE.

More notes on feDiffuseLighting-1.svg changes, which expand on my bug 993934 comment 9:
On second thought, I think the new behavior I describe there (new edge highlights) is actually correct.  The filter bounds are the same as the shape bounding box in that test, and lighting affects the edge pixels of the shape (both the first interior pixel and the first exterior pixel), so there should be a 1-pixel highlight visible, which there is with the new region changes and the new feDiffuseLighting-1-ref.svg (for both software and D2D filters, but not with the old code or in Chrome or IE, even though if you expand the filter region in those cases they do indeed show highlights on those interior bounding box edge pixels).

Also, as I noted in bug 993934, D2D has an extraneous 0x010101 (should be black (and of course it was a different color for that patch)) highlight across the bottom of the interior rectangle, and now that the filter region has been expanded by this patch, it also shows the same highlight across the top of the outer rectangle - both highlights are visible on IE11 (as 0x090909s I think) if you display the testcase with an expanded filter region so that IE displays the lighting effect along the top edge.  The increase in fuzz for feDiffuseLighting-1.svg is because of that new D2D highlight across the top.

Comment 2

[Markus Stange [:mstange]]

Thank you very much for fixing this!

I haven't looked at filters in a while, and forgotten some of the things involved... Is this the same as bug 1051185? My wip patch there looks like it does a little more than your patch here (for some reason it's adding a source region property)... do you understand what I was trying to do there? Because I don't :-)

Comment 3

[Tom Klein]

(In reply to Markus Stange [:mstange] from comment #2)
> Is this the same as bug 1051185? My wip patch there looks like
> it does a little more than your patch here (for some reason it's adding a
> source region property)... do you understand what I was trying to do there?

It doesn't seem to me like the source region property is needed - it's only being used in GetInputDataSourceSurface to say that whatever input surface is passed in should be padded with transparency out to mSourceRect, but as far as I can tell, just passing EDGE_MODE_NONE (actually it's the default) instead of EDGE_MODE_DUPLICATE accomplishes the same thing (well, it pads the input surface out to whatever srcRect in DoRender is, but that's the rectangle that's going to be rendered for the lighting primitive, so it better be your mSourceRect = primitive subregion (= filter region in the default case), which is what other parts of the patch make sure it is).

Your patch did inspire a couple changes in my patch, which I'll post shortly.

Comment 4

[Tom Klein]

Attachment: Patch v2

This patch has two differences from v1:

1) I added the feConvolveMatrix !EDGE_MODE_NONE check in ResultChangeRegionForPrimitive from the patch in bug 1051185 (looks good to me).

2) I moved the lighting cases that were in PostFilterExtentsForPrimitive in v1 of this patch into ResultChangeRegionForPrimitive, which functionally gives the same behavior but only requires lighting logic in one place instead of two.

(Explanation of why behavior is the same: PostFilterExtentsForPrimitive defers to ResultChangeRegionForPrimitive in the default case, so no behavior was changed by moving logic there, and the only other function that calls ResultChangeRegionForPrimitive is ComputeResultChangeRegion, which Ands the result of ResultChangeRegionForPrimitive with the primitiveSubregion, so (because of the And) nothing changes there.)
(Aside 1: The (unpatched) lighting case in ResultChangeRegionForPrimitive in v1 of this patch was functionally dead.)
(Aside 2: ResultChangeRegionForPrimitive is confusing to me, which is why I didn't mess with it in v1... I can't tell what it's supposed to compute and what "change" means...  Luckily I don't need to know exactly for this case!))

Comment 5

[Tom Klein]

Woops - the And in ComputeResultChangeRegion at changeRegion.And(changeRegion, descr.PrimitiveSubregion()); means intersection, not union (of course...), so v2 does differ functionally from v1.  Sorry about that!

Looking more into what ComputeResultChangeRegion is meant to compute now...

Comment 6

[Robert Longson [:longsonr]]

I think it would be clearer if IsLightingFilterType became HasUnboundedOutputRegion and returned true for IsTransferFilterType(aType) || aType == FilterType::COLOR_MATRIX || IsLightingFilterType(aType)

Then IsLightingFilterType would not be required and the purpose of the code would be clearer.

Comment 7

[Markus Stange [:mstange]]

(In reply to Tom Klein from comment #3)
> (well, it pads
> the input surface out to whatever srcRect in DoRender is, but that's the
> rectangle that's going to be rendered for the lighting primitive, so it
> better be your mSourceRect = primitive subregion (= filter region in the
> default case), which is what other parts of the patch make sure it is).

Hmm, is that even true if the input primitive and the lighting primitive have different primitive subregions? I think my patch was trying to use the primitive subregion of the input primitive, not the one of the lighting primitive. Would that be more correct? Does it even make a difference? I think when I wrote that patch I was concerned about the position of edge highlights. But if the input is padded with transparency anyway, does the size of the input even have any influence on the edge highlight positions? Actually, can there even be edge highlights, given that the lighting filter tries to avoid them by using REPEAT for the edge mode? Oh maybe that's what it is about - we need to pad with transparency inside the input primitive subregion, and then the lighting filter chooses to REPEAT around that surface?
Sorry for this avalanche of questions; I'm trying to wrap my head around this again...

Comment 8

[Tom Klein]

Attachment: primitiveRegionsTest.svg

Okay, I think I see what you're saying.  I guess I had a different take - my thinking was that I thought the filter convention was that whenever you're at a step in the filter process where a region of a surface is undefined, it should be made transparent black, which is what the patches here do.  That definitely can introduce highlights when the input surface is smaller than the lighting primitive region, but to me those seem expected (you're outputing a small surface into a sea of transparent black, and then lighting that result).

I'm attaching a couple testcases (are these the kind of situations you had in mind?) and I'll attach some screenshots of what the various patches and browsers do.

Comment 9

[Tom Klein]

Attachment: primitiveRegionsTest screenshots.png

It looks like Chrome does the same as this patch (but has some region issues), and it looks like IE doesn't light anything beyond the bounds of the input image (I scaled some of those images, so I'm not sure if the first IE region doesn't line up because of my scaling or because it's outputing an unexpected region).

Comment 10

[Tom Klein]

Attachment: invalidationTests.html

I don't know which edge mode behavior you'll want to go with, but I'm thinking now that the unpatched behavior of ResultChangeRegionForPrimitive (which is also what the v1 patch here uses) is actually correct (though I still feel confused...).

ComputeResultChangeRegion gets called from the layers code to find the effects region that needs to get invalidated when a given pre-effects region is invalid:
https://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=76076d8dad02#4500
I guess the question is what made it invalid in this case - if it's just a repaint issue then I would think ResultChangeRegionForPrimitive in the lighting case should just return its pre-filter region (adjusted for kernelUnitLength around the edges), which is what the current (unpatched) code does.  (But if that's all that invalidation here means then I don't understand why feGaussianBlur, for example, is returning the pre-filter invalidation region inflated by the standard deviations...)  Unfortunately I haven't been able to come up with a test yet to exercise the return value from ResultChangeRegionForPrimitive - so far all of the attached tests render fine even when I return a bogus 5x5 region whose topLeft is the topLeft of the pre-filter invalidation region.  I don't know the layers code very well, so maybe this is obvious to somebody else.  Any ideas?

Also, Robert, your suggestion looks good to me, thanks - I'll make the change if we wind up using one of these patches.

Comment 11

[Markus Stange [:mstange]]

Attachment: invalidation testcase (click to trigger animation)

ResultChangeRegionForPrimitive is called for changes *inside* the filtered element. If you trigger changes to the filtered element itself, those usually result in invalidations from the outside as well, hiding any bugs in the code that handles inside invalidations. So your testcase needs to have the filter applied on some kind of container element, such as <g>, or any HTML element.

Comment 12

[Markus Stange [:mstange]]

Attachment: screen recording of testcase with bug 1051185's patch

This video should explain why I needed the invalidation changes with the edge behavior from bug 1051185: a change that affects the edge pixels of the filter input can affect output pixels all the way to the outside of the filter result - it's no longer limited to just the kernelUnitLength radius around it.

Does this make sense or should I explain more?

Comment 13

[Markus Stange [:mstange]]

(In reply to Tom Klein from comment #10)
> I don't know which edge mode behavior you'll want to go with

Since there doesn't seem to be any cross-browser consensus on this, let's go with whatever is simplest. That would be your patch in this bug then, I think.

Comment 14

[Tom Klein]

Attachment: Patch v3

Thanks a bunch for the explanations and the testcase, that makes sense now!  I added a little of the explanation to the description of ComputeResultChangeRegion; let me know if I didn't get it quite right.

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8670077 [details] [diff] [review]
Patch v3

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

Thanks!

Comment 17

[Tom Klein]

Attachment: Patch v4

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

The new version of feDiffuseLighting-1.svg failed on Win8 x64 - it had 217 differing pixels instead of the 216 fuzz allowed in the v3 patch, and the "new" differing pixel is in a not unexpected edge highlight area, so increasing the fuzz to 217 and checking in.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2873ffc036ef

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2873ffc036ef