Implement a bicubic resampling image filter, with raster and GPU backends.

(Relanding r7275 with assert fix, plus fixes from r7276, r7280, r7283.)

Implement a bicubic resampling image filter, with raster and GPU backends.
In order to get this to work on the GPU side, I had to modify the width and height of the drawn texture in drawSprite() and drawDevice() to use the filtered texture's dimensions, instead of the source texture.  (This wasn't a problem before since all other image filters produce results the same dimensions as their input texture.)
For now, this implementation only does axis-aligned scaling (same as the Lanczos-3 implementation in Chrome).  It's also done for correctness and clarity, not speed, so there are lots of opportunities for speedups.

Committed: https://code.google.com/p/skia/source/detail?r=7275

Committed: https://code.google.com/p/skia/source/detail?r=7287

[Stephen White, 2013-01-08 23:20:43]

Mike, Brian:  Ping?

[reed1, 2013-01-09 13:25:36]

+humper

Is there a target use for this right now, or is it just experimental? I do think
skia needs a high-quality filter mode, but it needs to fit into our normal
pipeline, and handle all matrices.

[Stephen White, 2013-01-09 17:22:47]

On 2013/01/09 13:25:36, reed1 wrote:
> +humper
> 
> Is there a target use for this right now, or is it just experimental? I do
think
> skia needs a high-quality filter mode, but it needs to fit into our normal
> pipeline, and handle all matrices.

The use cases are 

1) To improve the quality of the GPU blur upres (both for shadows and RGBA
blurs).

2)  To potentially replace the Lanczos3 resize code in chrome/WebKit for upres. 
I could put this code in WebKit instead, but that would have the downsides of
not being SkPicture-hermetic, having to be modified on the Skia roll as the
upstream GrEffect etc API changes, and not handling 1 above.

Neither of these use cases require anything more than a 2D scale.  I could put
it in experimental, if you'd prefer.

[bsalomon, 2013-01-09 17:57:55]

If in practice we would always use Mitchell would it make sense to have the
shader bake in the coefficients rather than specifying them as uniforms?

https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
src/gpu/SkGpuDevice.cpp:1497: w = filteredTexture->width();
Does this mean that any size change always occurs on the right and bottom edges
(since we draw using l,t,w,h below)? Maybe that's ok, just checking.

[reed1, 2013-01-09 18:11:43]

If it can be used to improve the existing GPU blur code, then that sounds like a
reason to formally test it.

Is your proposal to have webkit attached this as an image filter to its
drawBitmap call (when it wants to have fancy scaling)? I had presumed our
approach was to integrate it into skia as some variant of "setBitmapFilter", and
then extend our existing sampling pipelines to get prettier/slower.

Does your code handle fractional scale factors (i.e. where the new bounds don't
lie on an integral boundary)?

[Stephen White, 2013-01-09 21:12:24]

On 2013/01/09 18:11:43, reed1 wrote:
> If it can be used to improve the existing GPU blur code, then that sounds like
a
> reason to formally test it.
> 
> Is your proposal to have webkit attached this as an image filter to its
> drawBitmap call (when it wants to have fancy scaling)? I had presumed our
> approach was to integrate it into skia as some variant of "setBitmapFilter",
and
> then extend our existing sampling pipelines to get prettier/slower.

It definitely should be folded into setBitmapFilter() in the longer term.  My
thinking was that since WebKit doesn't require the fully general solution, it
could use this image filter as a stopgap until the fully general solution was
implemented.

> Does your code handle fractional scale factors (i.e. where the new bounds
don't
> lie on an integral boundary)?

Yes it does, but the image it returns will have some clamping in the edge pixels
where there is only partial coverage (I think the skia bilinear does this too,
but it's hard to tell).  I need to look into what WebKit does with those edge
pixels; they should probably be blended by rect coverage, but I'm not sure if
they are currently.

[Stephen White, 2013-01-09 21:18:18]

On 2013/01/09 17:57:55, bsalomon wrote:
> If in practice we would always use Mitchell would it make sense to have the
> shader bake in the coefficients rather than specifying them as uniforms?

We could look at that.  There are a couple of other interesting options for
bicubic filters, so I'd prefer to keep it generic for now.

https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
> src/gpu/SkGpuDevice.cpp:1497: w = filteredTexture->width();
> Does this mean that any size change always occurs on the right and bottom
edges
> (since we draw using l,t,w,h below)? Maybe that's ok, just checking.

No, you're right; there might be a subpixel offset required.  I'll have a look
at the WebKit side to be sure.  I'll probably address that in a subsequent
patch, however.

[bsalomon, 2013-01-09 21:44:09]

https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
> > src/gpu/SkGpuDevice.cpp:1497: w = filteredTexture->width();
> > Does this mean that any size change always occurs on the right and bottom
> edges
> > (since we draw using l,t,w,h below)? Maybe that's ok, just checking.
> 
> No, you're right; there might be a subpixel offset required.  I'll have a look
> at the WebKit side to be sure.  I'll probably address that in a subsequent
> patch, however.

It sounds like onFilterImageGpu()'s signature needs to change to allow a return
offset like filterImage() does.

Minor aside: onFilterImageGpu should really be renamed to filterImageGpu(). We
generally use "on" for protected/private virtuals that implement public
functions:

class Base {
public:
   void foo() {
      check_some_stuff();
      if (!this->onFoo()) {
          handle_some_errors();
      }
   }
private:
   virtual bool onFoo() = 0;
}

[Stephen White, 2013-01-10 17:26:53]

On 2013/01/09 21:44:09, bsalomon wrote:
>
https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
> > > src/gpu/SkGpuDevice.cpp:1497: w = filteredTexture->width();
> > > Does this mean that any size change always occurs on the right and bottom
> > edges
> > > (since we draw using l,t,w,h below)? Maybe that's ok, just checking.
> > 
> > No, you're right; there might be a subpixel offset required.  I'll have a
look
> > at the WebKit side to be sure.  I'll probably address that in a subsequent
> > patch, however.
> 
> It sounds like onFilterImageGpu()'s signature needs to change to allow a
return
> offset like filterImage() does.

Looking into the Chrome side a bit more, it seems that the existing Lanczos code
only handles integer dest rects.  The GPU blur code won't need them either.  So
subpixel offsets are not required for the bicubic resize, and we can always
assume 0, 0 offset.

You're right that we should sync up the onFilterImage() and onFilterImageGPU()
API's, though.  I'd actually prefer that the offset be returned in a separate
call (getOffset()?), since very few of the existing filters use it, and in those
cases it can be determined from immutable values (i.e., from values passed to
the constructor).  it seems a shame to pollute the API with a call that could be
handled with a virtual on only those filters that need it.  Mike, what do you
think?

> Minor aside: onFilterImageGpu should really be renamed to filterImageGpu(). We
> generally use "on" for protected/private virtuals that implement public
> functions:

Good point.  I'll do that in a separate patch.

[reed1, 2013-01-10 18:50:45]

On 2013/01/10 17:26:53, Stephen White wrote:
> On 2013/01/09 21:44:09, bsalomon wrote:
> >
>
https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
> > > > src/gpu/SkGpuDevice.cpp:1497: w = filteredTexture->width();
> > > > Does this mean that any size change always occurs on the right and
bottom
> > > edges
> > > > (since we draw using l,t,w,h below)? Maybe that's ok, just checking.
> > > 
> > > No, you're right; there might be a subpixel offset required.  I'll have a
> look
> > > at the WebKit side to be sure.  I'll probably address that in a subsequent
> > > patch, however.
> > 
> > It sounds like onFilterImageGpu()'s signature needs to change to allow a
> return
> > offset like filterImage() does.
> 
> Looking into the Chrome side a bit more, it seems that the existing Lanczos
code
> only handles integer dest rects.  The GPU blur code won't need them either. 
So
> subpixel offsets are not required for the bicubic resize, and we can always
> assume 0, 0 offset.
> 
> You're right that we should sync up the onFilterImage() and onFilterImageGPU()
> API's, though.  I'd actually prefer that the offset be returned in a separate
> call (getOffset()?), since very few of the existing filters use it, and in
those
> cases it can be determined from immutable values (i.e., from values passed to
> the constructor).  it seems a shame to pollute the API with a call that could
be
> handled with a virtual on only those filters that need it.  Mike, what do you
> think?

I interpreted Brian's comment to suggest that onFilterImageGpu just have an
additional parameter (offset or rect), not an additional virtual.

> 
> > Minor aside: onFilterImageGpu should really be renamed to filterImageGpu().
We
> > generally use "on" for protected/private virtuals that implement public
> > functions:
> 
> Good point.  I'll do that in a separate patch.

[Stephen White, 2013-01-10 19:25:12]

On 2013/01/10 18:50:45, reed1 wrote:
> On 2013/01/10 17:26:53, Stephen White wrote:
> > On 2013/01/09 21:44:09, bsalomon wrote:
> > >
> >
>
https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp#newc...
> > > > > src/gpu/SkGpuDevice.cpp:1497: w = filteredTexture->width();
> > > > > Does this mean that any size change always occurs on the right and
> bottom
> > > > edges
> > > > > (since we draw using l,t,w,h below)? Maybe that's ok, just checking.
> > > > 
> > > > No, you're right; there might be a subpixel offset required.  I'll have
a
> > look
> > > > at the WebKit side to be sure.  I'll probably address that in a
subsequent
> > > > patch, however.
> > > 
> > > It sounds like onFilterImageGpu()'s signature needs to change to allow a
> > return
> > > offset like filterImage() does.
> > 
> > Looking into the Chrome side a bit more, it seems that the existing Lanczos
> code
> > only handles integer dest rects.  The GPU blur code won't need them either. 
> So
> > subpixel offsets are not required for the bicubic resize, and we can always
> > assume 0, 0 offset.
> > 
> > You're right that we should sync up the onFilterImage() and
onFilterImageGPU()
> > API's, though.  I'd actually prefer that the offset be returned in a
separate
> > call (getOffset()?), since very few of the existing filters use it, and in
> those
> > cases it can be determined from immutable values (i.e., from values passed
to
> > the constructor).  it seems a shame to pollute the API with a call that
could
> be
> > handled with a virtual on only those filters that need it.  Mike, what do
you
> > think?
> 
> I interpreted Brian's comment to suggest that onFilterImageGpu just have an
> additional parameter (offset or rect), not an additional virtual.

Yes, that's what Brian suggested.  My point was that we could remove the param
from both APIs and all the call sites by adding a virtual instead.  That way,
only those filters which care about offsets have to handle them.  The WebKit
implementation seems to be able to get by without an offset outparam.

Your "offset or rect" comment brings up a good point, though:  a rect would be
better than an offset for the cases where texture width and height don't match
bitmap width and height (e.g., NPOT textures), or if a filter wants to return a
subset of an existing bitmap.

> 
> > 
> > > Minor aside: onFilterImageGpu should really be renamed to
filterImageGpu().
> We
> > > generally use "on" for protected/private virtuals that implement public
> > > functions:
> > 
> > Good point.  I'll do that in a separate patch.

[reed1, 2013-01-10 19:34:11]

Can't we already efficiently return a subset of an existing bitmap, via
extract-subset?

I don't understand the texture-vs-bitmap width/height statement. Is that
already something known to Brian?

I don't favor the virtual, since it can require a getter and setter if the
filters are chained together. I think it is clearest to be seen as an
in/out parameter.


On Thu, Jan 10, 2013 at 2:25 PM, <senorblanco@chromium.org> wrote:

> On 2013/01/10 18:50:45, reed1 wrote:
>
>> On 2013/01/10 17:26:53, Stephen White wrote:
>> > On 2013/01/09 21:44:09, bsalomon wrote:
>> > >
>> >
>>
>
> https://codereview.appspot.**com/7033049/diff/2001/src/gpu/**
>
SkGpuDevice.cpp#newcode1497<https://codereview.appspot.com/7033049/diff/2001/src/gpu/SkGpuDevice.cpp#newcode1497>
>
>> > > > > src/gpu/SkGpuDevice.cpp:1497: w = filteredTexture->width();
>> > > > > Does this mean that any size change always occurs on the right
>>
> and
>
>> bottom
>> > > > edges
>> > > > > (since we draw using l,t,w,h below)? Maybe that's ok, just
>>
> checking.
>
>> > > >
>> > > > No, you're right; there might be a subpixel offset required.
>>
> I'll have a
>
>> > look
>> > > > at the WebKit side to be sure.  I'll probably address that in a
>>
> subsequent
>
>> > > > patch, however.
>> > >
>> > > It sounds like onFilterImageGpu()'s signature needs to change to
>>
> allow a
>
>> > return
>> > > offset like filterImage() does.
>> >
>> > Looking into the Chrome side a bit more, it seems that the existing
>>
> Lanczos
>
>> code
>> > only handles integer dest rects.  The GPU blur code won't need them
>>
> either.
>
>> So
>> > subpixel offsets are not required for the bicubic resize, and we can
>>
> always
>
>> > assume 0, 0 offset.
>> >
>> > You're right that we should sync up the onFilterImage() and
>>
> onFilterImageGPU()
>
>> > API's, though.  I'd actually prefer that the offset be returned in a
>>
> separate
>
>> > call (getOffset()?), since very few of the existing filters use it,
>>
> and in
>
>> those
>> > cases it can be determined from immutable values (i.e., from values
>>
> passed to
>
>> > the constructor).  it seems a shame to pollute the API with a call
>>
> that could
>
>> be
>> > handled with a virtual on only those filters that need it.  Mike,
>>
> what do you
>
>> > think?
>>
>
>  I interpreted Brian's comment to suggest that onFilterImageGpu just
>>
> have an
>
>> additional parameter (offset or rect), not an additional virtual.
>>
>
> Yes, that's what Brian suggested.  My point was that we could remove the
> param from both APIs and all the call sites by adding a virtual instead.
>  That way, only those filters which care about offsets have to handle
> them.  The WebKit implementation seems to be able to get by without an
> offset outparam.
>
> Your "offset or rect" comment brings up a good point, though:  a rect
> would be better than an offset for the cases where texture width and
> height don't match bitmap width and height (e.g., NPOT textures), or if
> a filter wants to return a subset of an existing bitmap.
>
>
>
>  >
>> > > Minor aside: onFilterImageGpu should really be renamed to
>>
> filterImageGpu().
>
>> We
>> > > generally use "on" for protected/private virtuals that implement
>>
> public
>
>> > > functions:
>> >
>> > Good point.  I'll do that in a separate patch.
>>
>
>
>
>
https://codereview.appspot.**com/7033049/<https://codereview.appspot.com/7033...
>

[bsalomon, 2013-01-10 19:41:57]

On 2013/01/10 19:34:11, reed1 wrote:
> Can't we already efficiently return a subset of an existing bitmap, via
> extract-subset?
> 
> I don't understand the texture-vs-bitmap width/height statement. Is that
> already something known to Brian?
> 

Texture's don't have an extract subset equivalent. One (magnificent) day someone
will tackle this generally so that texture-backed SkBitmap's can have a proper
extract subset implementation. There is some now quite dated info about this
here: http://code.google.com/p/skia/wiki/GrTextureViewProposal.

I don't think NPOT is an issue unless we ever need to tile or mip-map the
result. ES2 is our minimum and it supports NPOT with no mips and clamp
wrap-mode.

[Stephen White, 2013-01-10 23:23:34]

On 2013/01/10 19:41:57, bsalomon wrote:
> On 2013/01/10 19:34:11, reed1 wrote:
> > Can't we already efficiently return a subset of an existing bitmap, via
> > extract-subset?
> > 
> > I don't understand the texture-vs-bitmap width/height statement. Is that
> > already something known to Brian?
> > 
> 
> Texture's don't have an extract subset equivalent. One (magnificent) day
someone
> will tackle this generally so that texture-backed SkBitmap's can have a proper
> extract subset implementation. There is some now quite dated info about this
> here: http://code.google.com/p/skia/wiki/GrTextureViewProposal.
> 
> I don't think NPOT is an issue unless we ever need to tile or mip-map the
> result. ES2 is our minimum and it supports NPOT with no mips and clamp
> wrap-mode.

OK, that's cool, so we can always rely on texture size == bitmap size for now. 
I suppose if we ever start using approx. match scratch textures, though, we'll
have trouble.

[Stephen White, 2013-01-11 15:42:21]

I've logged https://code.google.com/p/skia/issues/detail?id=1055 for the API
differences between filterImageGPU() and onFilterImage().

Since this filter doesn't need either subpixel or pixel-based offsets, I think
we can leave those questions aside for now.

Are there any other issues you guys would like to see addressed before this
lands?

[bsalomon, 2013-01-11 16:04:35]

On 2013/01/11 15:42:21, Stephen White wrote:
> I've logged https://code.google.com/p/skia/issues/detail?id=1055 for the API
> differences between filterImageGPU() and onFilterImage().
> 
> Since this filter doesn't need either subpixel or pixel-based offsets, I think
> we can leave those questions aside for now.
> 
> Are there any other issues you guys would like to see addressed before this
> lands?

lgtm