Bug 89116 – Specular light filters produce dark results

• Component: SVG
• Status: RESOLVED FIXED

[Florin Malita]

Created attachment 147628 [details]
White light filters should not produce grey rectangles around the target

This is most obvious if the light filter overlay is not clipped (see attached test).

FELighting is applied in unmultiplied space, but the alpha channel is set for premultiplied RGBA. Then when the image is converted to premultiplied for compositing, the RGB values are scaled down resulting in a darker image.

[Florin Malita]

A straightforward fix is to update FELighting to produce premultiplied results (it already does, but stores it in the unmultiplied buffer). Not only does this take care of the darker result, but it also eliminates subsequent unmultiplied->premultiplied conversions needed by feComposite.

Patch coming up.

[Florin Malita]

Created attachment 147656 [details]
Patch

[Dirk Schulze]

Comment on attachment 147656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147656&action=review

r=me

> Source/WebCore/ChangeLog:14
> +        for premultiplied values (which causes a dakening of the result upon the

s/dakening/darkening/

[Zoltan Herczeg]

Not sure I totally understand the concept here.

Try to gather what I know:
- This only affects specular lighting, since it is always 1 for diffuse
- The heightmap is based on the alpha channel.
  -> it doesn't matter if the input is pre or unmultiplied
  (perhaps the filter should be enough clever to recognise this)
- The spec says only: Sa = max(Sr, Sg, Sb)

Making the output brighter is a nice idea, but is it really what the spec wants? Unfortunately the SVG spec for filters does not explain a lot of things. Sometimes difficult to know what the authors want...

[Florin Malita]

(In reply to comment #4)
> Not sure I totally understand the concept here.
> 
> Try to gather what I know:
> - This only affects specular lighting, since it is always 1 for diffuse
Right.

> - The heightmap is based on the alpha channel.
>   -> it doesn't matter if the input is pre or unmultiplied
>   (perhaps the filter should be enough clever to recognise this)
Yes (for RGB calculations), but see below.

> - The spec says only: Sa = max(Sr, Sg, Sb)
This is where is matters - this is supposed to be done in premultiplied space: "Unless otherwise stated, all image filters operate on premultiplied RGBA samples."

> Making the output brighter is a nice idea
It's not just brighter, it's really keeping the color correct.

Take a pure green light for example: #00ff00. Then at some point assume we calculate a specular RGB component of (0,128,0). The spec says the alpha should also be 128, so (r,g,b,a) = (0, 128, 0, 128). If you treat this as unmultiplied, then for compositing purposes your light is no longer pure green (#00ff00) but some darker tone (#004000) -> hence the grey artifact in the example. If OTOH (0, 128, 0, 128) is premultiplied, then you're still applying the original color with 50% alpha as intended.

[Stephen White]

(In reply to comment #4)
> Not sure I totally understand the concept here.
> 
> Try to gather what I know:
> - This only affects specular lighting, since it is always 1 for diffuse
> - The heightmap is based on the alpha channel.
>   -> it doesn't matter if the input is pre or unmultiplied
>   (perhaps the filter should be enough clever to recognise this)

As you say, this shouldn't matter either way for correctness, since the filter only takes the alpha.  It might be a little faster this way, however:  if the previous filter outputs a premultiplied result (the more common case), this will save having to do a useless unmultiply step.  Of course, ideally we should just grab the alpha without touching RGB, but that could be a further optimization.

> - The spec says only: Sa = max(Sr, Sg, Sb)
> 
> Making the output brighter is a nice idea, but is it really what the spec wants? Unfortunately the SVG spec for filters does not explain a lot of things. Sometimes difficult to know what the authors want...

This part is for correctness.  My reading that Florin's change is correct is from here:  "Unless otherwise stated, all image filters operate on premultiplied RGBA samples. Filters which work more naturally on non-premultiplied data (feColorMatrix and feComponentTransfer) will temporarily undo and redo premultiplication as specified."

By requesting an unmultiplied result buffer, the way the code was before, the next filter will cause the premultiply to happen again,
leaving us with  Sr = Sr * Sa, Sg = Sg * sA, Sb * Sa (where Sa was the previously-computed max of the unmultiplied colours).  If that was what was desired, presumably that would have been explicitly stated in the feSpecularFilter description.  Since it's not listed as an exception to the "all filters produced premultiplied alpha" rule, I think it's safe to say the original (Sr, Sg, Sb) as described in the feSpecularFilter docs are assumed to be already premultiplied.

[Florin Malita]

Created attachment 147808 [details]
Patch for landing

[Zoltan Herczeg]

Thnaks for the explanation.

[WebKit Review Bot]

Comment on attachment 147808 [details]
Patch for landing

Clearing flags on attachment: 147808

Committed r120467: <http://trac.webkit.org/changeset/120467>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.