Blend image filter

Implements the non-Porter-Duff compositing modes required for SVG's feBlend element.  This filter has two inputs, since normal blending can't be used.  The GPU side uses two filter stages to accomplish this:  one to sample the background, and one to sample the foreground and blend it.

[bsalomon, 2012-08-22 12:44:29]

Overall it looks good. Can you explain why there is a foreground passed to the
constructor and a src bitmap. Why both? Are they used in different scenarios?

Also, should this be added to the SkGlobalInitialization_*.cpp files?

http://codereview.appspot.com/6463081/diff/1/src/effects/SkBlendImageFilter.cpp
File src/effects/SkBlendImageFilter.cpp (right):

http://codereview.appspot.com/6463081/diff/1/src/effects/SkBlendImageFilter.c...
src/effects/SkBlendImageFilter.cpp:168: // FIXME:  This should be refactored
with SkSingleImageFilter's version.
What does this comment mean?

[Stephen White, 2012-08-22 15:30:33]

On 2012/08/22 12:44:29, bsalomon wrote:
> Overall it looks good. Can you explain why there is a foreground passed to the
> constructor and a src bitmap. Why both? Are they used in different scenarios?

You mean the src bitmap passed to onFilterImage?  That's the "root" src image of
the filter network.  The foreground input, by contrast, is this filter's input,
and is an SkImageFilter.  If it's NULL, the src bitmap is used instead.  If it's
non-NULL, it's recursively evaluated.  This is the same as how the single-input
image filters work:  the "src" bitmap is the implicit "root" of the filter tree,
and only used if the input is NULL.

> Also, should this be added to the SkGlobalInitialization_*.cpp files?

Thanks, yes it should.  Done.
>
http://codereview.appspot.com/6463081/diff/1/src/effects/SkBlendImageFilter.cpp
> File src/effects/SkBlendImageFilter.cpp (right):
> 
>
http://codereview.appspot.com/6463081/diff/1/src/effects/SkBlendImageFilter.c...
> src/effects/SkBlendImageFilter.cpp:168: // FIXME:  This should be refactored
> with SkSingleImageFilter's version.
> What does this comment mean?

I basically copy-and-pasted SkSingleInputImageFilter::getInputResultAsTexture().
 :(  (and the comment got the name wrong, fixed).  I'd love to move it to the
common base class (SkImageFilter), but I can't, since SkImageFilter.cpp lives in
src/core, which can't depend on src/gpu.

This comment is a reminder to refactor it somewhere else, maybe a utility class.

[bsalomon, 2012-08-22 16:16:43]

LGTM.

On 2012/08/22 15:30:33, Stephen White wrote:
> On 2012/08/22 12:44:29, bsalomon wrote:
> > Overall it looks good. Can you explain why there is a foreground passed to
the
> > constructor and a src bitmap. Why both? Are they used in different
scenarios?
> 
> You mean the src bitmap passed to onFilterImage?  That's the "root" src image
of
> the filter network.  The foreground input, by contrast, is this filter's
input,
> and is an SkImageFilter.  If it's NULL, the src bitmap is used instead.  If
it's
> non-NULL, it's recursively evaluated.  This is the same as how the
single-input
> image filters work:  the "src" bitmap is the implicit "root" of the filter
tree,
> and only used if the input is NULL.

Oh right, that makes sense.

> 
> > Also, should this be added to the SkGlobalInitialization_*.cpp files?
> 
> Thanks, yes it should.  Done.
> >
>
http://codereview.appspot.com/6463081/diff/1/src/effects/SkBlendImageFilter.cpp
> > File src/effects/SkBlendImageFilter.cpp (right):
> > 
> >
>
http://codereview.appspot.com/6463081/diff/1/src/effects/SkBlendImageFilter.c...
> > src/effects/SkBlendImageFilter.cpp:168: // FIXME:  This should be refactored
> > with SkSingleImageFilter's version.
> > What does this comment mean?
> 
> I basically copy-and-pasted
SkSingleInputImageFilter::getInputResultAsTexture().
>  :(  (and the comment got the name wrong, fixed).  I'd love to move it to the
> common base class (SkImageFilter), but I can't, since SkImageFilter.cpp lives
in
> src/core, which can't depend on src/gpu.
> 
> This comment is a reminder to refactor it somewhere else, maybe a utility
class.

Got it... we can't use that class because it is only works with a single source?

[Stephen White, 2012-08-22 16:22:20]

On 2012/08/22 16:16:43, bsalomon wrote:
> LGTM.
> 
> On 2012/08/22 15:30:33, Stephen White wrote:
> > On 2012/08/22 12:44:29, bsalomon wrote:
> > > Overall it looks good. Can you explain why there is a foreground passed to
> the
> > > constructor and a src bitmap. Why both? Are they used in different
> scenarios?
> > 
> > You mean the src bitmap passed to onFilterImage?  That's the "root" src
image
> of
> > the filter network.  The foreground input, by contrast, is this filter's
> input,
> > and is an SkImageFilter.  If it's NULL, the src bitmap is used instead.  If
> it's
> > non-NULL, it's recursively evaluated.  This is the same as how the
> single-input
> > image filters work:  the "src" bitmap is the implicit "root" of the filter
> tree,
> > and only used if the input is NULL.
> 
> Oh right, that makes sense.
> 
> > 
> > > Also, should this be added to the SkGlobalInitialization_*.cpp files?
> > 
> > Thanks, yes it should.  Done.
> > >
> >
>
http://codereview.appspot.com/6463081/diff/1/src/effects/SkBlendImageFilter.cpp
> > > File src/effects/SkBlendImageFilter.cpp (right):
> > > 
> > >
> >
>
http://codereview.appspot.com/6463081/diff/1/src/effects/SkBlendImageFilter.c...
> > > src/effects/SkBlendImageFilter.cpp:168: // FIXME:  This should be
refactored
> > > with SkSingleImageFilter's version.
> > > What does this comment mean?
> > 
> > I basically copy-and-pasted
> SkSingleInputImageFilter::getInputResultAsTexture().
> >  :(  (and the comment got the name wrong, fixed).  I'd love to move it to
the
> > common base class (SkImageFilter), but I can't, since SkImageFilter.cpp
lives
> in
> > src/core, which can't depend on src/gpu.
> > 
> > This comment is a reminder to refactor it somewhere else, maybe a utility
> class.
> 
> Got it... we can't use that class because it is only works with a single
source?

Exactly.

[bsalomon, 2012-08-22 17:53:38]

https://codereview.appspot.com/6463081/diff/14002/src/ports/SkGlobalInitializ...
File src/ports/SkGlobalInitialization_default.cpp (right):

https://codereview.appspot.com/6463081/diff/14002/src/ports/SkGlobalInitializ...
src/ports/SkGlobalInitialization_default.cpp:49: #include
"SkMagnifierImageFilter.h"
Missing the new header (causing the android build bot failures)

[Stephen White, 2012-08-22 18:35:04]

On 2012/08/22 17:53:38, bsalomon wrote:
>
https://codereview.appspot.com/6463081/diff/14002/src/ports/SkGlobalInitializ...
> File src/ports/SkGlobalInitialization_default.cpp (right):
> 
>
https://codereview.appspot.com/6463081/diff/14002/src/ports/SkGlobalInitializ...
> src/ports/SkGlobalInitialization_default.cpp:49: #include
> "SkMagnifierImageFilter.h"
> Missing the new header (causing the android build bot failures)

Doh!  Fixed in r5236.

[Stephen White, 2012-08-22 21:30:38]

Was landed as r5231; closing.