lighting image filters

Raster implementation of diffuse and specular lighting filters.  Externally, the caller instantiates a light (distant, point or spot), and an SkDiffuseLightingFilter or SkSpecularLightingImageFilter with that light.  A Sobel edge detection filter is applied to the alpha of the incoming bitmap, and the result is used as a height map for lighting calculations.

[reed1, 2012-06-20 12:12:25]

Can we reformulate this as a set of factory functions, rather than exposing so
much of the hierarchy and implementation? GradientShaders and BlurMaskFilters do
this, allowing us to specialize as we need underneath the hood.

Can we use a struct+enum for the light variants? That would seem to match the
use if the virtual enum/cast, and will make serialization easier.

[Stephen White, 2012-06-20 14:03:44]

On 2012/06/20 12:12:25, reed1 wrote:
> Can we reformulate this as a set of factory functions, rather than exposing so
> much of the hierarchy and implementation? GradientShaders and BlurMaskFilters
do
> this, allowing us to specialize as we need underneath the hood.

Good idea.  This would let me hide the light classes inside the .cpp.  Were you
thinking something like this:

class SkDiffuseLightingImageFilter ... {
static SkImageFilter* CreatePointLit(const SkPoint3 location&, const SkColor&
lightColor, float surfaceScale, float kd);
static SkImageFilter* CreateDistantLit(float azimuth, float elevation,  const
SkColor& lightColor, float surfaceScale, float kd);
static SkImageFilter* CreateSpotLit(const SkPoint3& location, const SkPoint3&
target, float specularExponent, float cutoffAngle, const SkColor& lightColor,
float surfaceScale, float kd);

and similarly for SkSpecularLightingImageFilter?

> Can we use a struct+enum for the light variants? That would seem to match the
> use if the virtual enum/cast, and will make serialization easier.

You mean one struct for all the light types?  Hmmm.  Right now, the
surfaceToLight() and lightColorScale() functions are part of the template
contract, so we'd have to find another way to template on the different light
types.

I think serialization is doable with the current implementation.  Let me give it
a shot.

[reed1, 2012-06-20 14:24:21]

Just trying to minimize our public base-class exposure. A virtual light
hierarchy can be made to work, but it might be overkill, just a hunch. Do we
need to support arbitrary future light types at this stage for SVG
compatibility?

[Stephen White, 2012-06-20 16:17:18]

create factory functions, remove SkLights from header, implement serialization

[reed1, 2012-06-20 16:45:43]

1. Deliberately using float instead of SkScalar in SkPoint3?
2. Can all of the factories just be part of a single class (basically treated as
a namespace)?

e.g.

class SkLightingImageFilter {
public:
    static SkImageFilter* CreateSpot...(...);
    static SkImageFilter* CreateDirectional...(...);
    ...
};

Then there's no need for *any* actual visible implementation details. Makes for
a header which is just a bunch of factories (and documentation).

[Stephen White, 2012-06-20 18:35:26]

On 2012/06/20 16:45:43, reed1 wrote:
> 1. Deliberately using float instead of SkScalar in SkPoint3?

Use of SkScalar tends to spread like kudzu, and I'd either end up having to do
all the math using the SkScalar macros, or just do more scalar-to-float
conversions partway down.  IMHO, if someone actually cares about fixed-point
performance on this filter, we can address it in a followup.

> 2. Can all of the factories just be part of a single class (basically treated
as
> a namespace)?
> 
> e.g.
> 
> class SkLightingImageFilter {
> public:
>     static SkImageFilter* CreateSpot...(...);
>     static SkImageFilter* CreateDirectional...(...);
>     ...
> };
> 
> Then there's no need for *any* actual visible implementation details. Makes
for
> a header which is just a bunch of factories (and documentation).

OK, I've met you part way:  all the factories are in SkLightingImageFilter, and
all the subclasses are in the .cpp.  There are still some member variables in
there, which seems to be true for all the other image filters as well.

[reed1, 2012-06-20 20:31:18]

1. Until we kill SkScalar-as-fixed globally, lets not introduce a root-level
class like SkPoint3 that doesn't use it. Not sure your reluctance to a
multiplication macro, but if you insist, lets move the class to be an inner
class inside SkLightingImageFilter.

2.  I don't see any value in exposing protected methods and fields publicly for
a class that we explicitly don't want clients to subclass. SkGradientShader
doesn't, though we do in other cases (which I hope to fix). However, I can
always clean it up later in another CL :)

[reed1, 2012-06-20 20:37:22]

http://codereview.appspot.com/6302101/diff/10002/src/effects/SkLightingImageF...
File src/effects/SkLightingImageFilter.cpp (right):

http://codereview.appspot.com/6302101/diff/10002/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:14: const float oneThird = 1.0f / 3.0f;
nit: gOneThird naming convention

http://codereview.appspot.com/6302101/diff/10002/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:17: inline void shiftMatrix(int m[9])
nit: open { on the same line as the function

http://codereview.appspot.com/6302101/diff/10002/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:32: float lum = fKD *
normal.dot(surfaceTolight);
nit: if blocks always have { }

[Stephen White, 2012-06-20 20:52:10]

On 2012/06/20 20:31:18, reed1 wrote:
> 1. Until we kill SkScalar-as-fixed globally, lets not introduce a root-level
> class like SkPoint3 that doesn't use it. Not sure your reluctance to a
> multiplication macro, but if you insist, lets move the class to be an inner
> class inside SkLightingImageFilter.

That'll make all the callsites really ugly.  Since you've given me the choice of
two ugly stepsisters, I'll choose the SkScalar option.  :)

> 2.  I don't see any value in exposing protected methods and fields publicly
for
> a class that we explicitly don't want clients to subclass. SkGradientShader
> doesn't, though we do in other cases (which I hope to fix). However, I can
> always clean it up later in another CL :)

SGTM

[Stephen White, 2012-06-20 23:21:32]

http://codereview.appspot.com/6302101/diff/10002/src/effects/SkLightingImageF...
File src/effects/SkLightingImageFilter.cpp (right):

http://codereview.appspot.com/6302101/diff/10002/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:14: const float oneThird = 1.0f / 3.0f;
On 2012/06/20 20:37:22, reed1 wrote:
> nit: gOneThird naming convention

Done.

http://codereview.appspot.com/6302101/diff/10002/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:17: inline void shiftMatrix(int m[9])
On 2012/06/20 20:37:22, reed1 wrote:
> nit: open { on the same line as the function

Done.

http://codereview.appspot.com/6302101/diff/10002/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:32: float lum = fKD *
normal.dot(surfaceTolight);
On 2012/06/20 20:37:22, reed1 wrote:
> nit: if blocks always have { }

Done (code is gone; replaced w/SkScalarClampMax).

[TomH, 2012-06-21 15:21:20]

Mike tackled all the tough stuff.

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
File src/effects/SkLightingImageFilter.cpp (right):

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:22: inline void shiftMatrix(int m[9]) {
Nit: This name doesn't really tell me anything about *why* you'd do this, and
neither do any of the call sites. (Yeah, I figured it out eventually, but is
there a better name?)

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:36: SkScalar lum = SkScalarMul(fKD,
normal.dot(surfaceTolight));
Nit: is lum luminance?

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:69: inline SkScalar compute(int a, int b,
int c, int d, int e, int f, SkScalar scale) {
Nit: What are you computing?

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:164: for (++y; y < src.height() - 1; ++y)
{
These three code blocks are almost identical - would generalizing and moving to
an inlined function lose too much performance from all the conditionals?

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:344: fCosOuterConeAngle =
cosf(SkDegreesToRadians(cutoffAngle));
Do we need to clean up the floating point assumption more places? SkScalarCos()
here, frex.

(Even if we do get rid of fixed, we may instead allow double, so going through
SkScalarFoo() keeps the right precision)

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:530: switch (light()->type()) {
It'd be nice if there were some way to replace the switches with template magic,
but I don't see any...

[Stephen White, 2012-06-22 18:13:40]

Thanks for the review.

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
File src/effects/SkLightingImageFilter.cpp (right):

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:22: inline void shiftMatrix(int m[9]) {
On 2012/06/21 15:21:20, TomH wrote:
> Nit: This name doesn't really tell me anything about *why* you'd do this, and
> neither do any of the call sites. (Yeah, I figured it out eventually, but is
> there a better name?)

I can't think of a good one.  Renamed to shiftMatrixLeft() and added a comment.

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:36: SkScalar lum = SkScalarMul(fKD,
normal.dot(surfaceTolight));
On 2012/06/21 15:21:20, TomH wrote:
> Nit: is lum luminance?

Approximatgely, but not really.  Changed it to colorScale, to avoid confusion.

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:69: inline SkScalar compute(int a, int b,
int c, int d, int e, int f, SkScalar scale) {
On 2012/06/21 15:21:20, TomH wrote:
> Nit: What are you computing?

How about sobel()?  (ulterior motive:  this also gets me under the 80-char limit
at the call sites).

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:164: for (++y; y < src.height() - 1; ++y)
{
On 2012/06/21 15:21:20, TomH wrote:
> These three code blocks are almost identical - would generalizing and moving
to
> an inlined function lose too much performance from all the conditionals?

Yeah, it's the inner loops I'm most worried about:  3 reads in the interior
version vs. on the top and bottom borders.  Still, I agree that it's too
verbose.  I'll try to clean it up in a followup.

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:344: fCosOuterConeAngle =
cosf(SkDegreesToRadians(cutoffAngle));
On 2012/06/21 15:21:20, TomH wrote:
> Do we need to clean up the floating point assumption more places?
SkScalarCos()
> here, frex.

Good catch.  Fixed.  Let me know if you see any more -- I'd like to keep it
SkScalar-clean, now that I've done the grunt-work.
> 
> (Even if we do get rid of fixed, we may instead allow double, so going through
> SkScalarFoo() keeps the right precision)

http://codereview.appspot.com/6302101/diff/16001/src/effects/SkLightingImageF...
src/effects/SkLightingImageFilter.cpp:530: switch (light()->type()) {
On 2012/06/21 15:21:20, TomH wrote:
> It'd be nice if there were some way to replace the switches with template
magic,
> but I don't see any...

There is a way to do it using a linked list of templates ("next" is a template
argument), and iterating over it with tail-recursion, but I'm not a big fan of
template metaprogramming to be honest.

[TomH, 2012-06-22 20:31:50]

LGTM

[Stephen White, 2012-06-22 21:01:50]

Landed as r4314; closing.