Make filters use SkImage instead of SkBitmap

Make filters use SkImage instead of SkBitmap

Make filters use SkImage instead of SkBitmap. This way rendering
of a device can be captured to a read-only snapshot. This
replaces the usage of SkBaseDevice::accessBitmap.

The SkBaseDevice::accessBitmap suffers from the problem that the
returned bitmap refers to the backing store of the device. Thus
if the device is further drawn to, the results are expected
to be reflected to the reference returned by accessBitmap.

This work is work towards making it possible to have render targets that
are not neccessarily textures. Rather, when the render target rendering
is needed, a snapshot should be taken.

BUG=skia:3388

[Kimmo Kinnunen, 2015-02-12 13:55:56]

kkinnunen@nvidia.com changed reviewers:
+ bsalomon@google.com

[Kimmo Kinnunen, 2015-02-12 13:55:57]

Unfortunately didn't manage to get a running version (dm compiles).

Did you mean something like this, in order to avoid SkBaseDevice::accessBitmap()

[reed1, 2015-02-12 14:21:51]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2015-02-12 14:21:53]

I like the experiment.

As you find misc. clean-ups (e.g. adding const in SkImage.h) consider submitting
those as separate CLs... will reduce the size of this biggie (which may take a
while to complete).

https://codereview.chromium.org/920513003/diff/20001/include/core/SkImage.h
File include/core/SkImage.h (right):

https://codereview.chromium.org/920513003/diff/20001/include/core/SkImage.h#n...
include/core/SkImage.h:77: GrTexture* getTexture() const;
Lets land this specific change ASAP.

https://codereview.chromium.org/920513003/diff/20001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/920513003/diff/20001/include/core/SkImageFilt...
include/core/SkImageFilter.h:61: virtual bool get(const Key& key,
SkAutoTUnref<SkImage>& result, SkIPoint* offset) const = 0;
I understand the idea, but this parameter passing style is not our convention.
Can this function just return SkImage* or NULL?

https://codereview.chromium.org/920513003/diff/20001/include/core/SkImageFilt...
include/core/SkImageFilter.h:62: virtual void set(const Key& key, SkImage&
result, const SkIPoint& offset) = 0;
Skia convention is to pass this by (const) pointer, not ref.

[bsalomon, 2015-02-13 14:24:35]

Kimmo, you're a brave soul! :)

Moving image filters towards surface and image is absolutely the right way to
go.

This patch inspired internal talk about IFs fit into the pipeline and how we
might improve them overall.

https://codereview.chromium.org/920513003/diff/20001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/920513003/diff/20001/include/core/SkDevice.h#...
include/core/SkDevice.h:382: // TODO: remove when GrRenderTarget is-a SkSurface.
Do you mean to make GrRenderTarget be a subclass of SkSurface?

[Kimmo Kinnunen, 2015-02-16 14:56:38]

On 2015/02/13 14:24:35, bsalomon wrote:
> Kimmo, you're a brave soul! :)
> 
> Moving image filters towards surface and image is absolutely the right way to
> go.
> 
> This patch inspired internal talk about IFs fit into the pipeline and how we
> might improve them overall.

Sure. Unfortunately it requires even more refactoring.
Either SkImages need to be constructable from some sort of SkBaseDevice state,
or all SkBaseDevices need to be backed by a SkSurface.

For the latter:
I'm kind of working on this, but I'm not sure if that's acceptable (it also
affects all the places).
It may doable at least by a having two kinds of devices: ones that come from
SkSurfaces, and ones that do not.

Eg. "normal" case would be that surface owns device (current surface
constructors).
    "deprecated" case would be that device owns the surface (eg. skia refs the
device, but the device can be snapshotted through the surface that the device
owns.)

The problem with this is that it goes against the current pattern where canvas
is created of a (bitmap) device. E.g the "deprecated" case is the default at the
moment.

The code could be something like following:
https://codereview.chromium.org/912243004


Maybe the other option could be to have all devices own their surfaces.

> include/core/SkDevice.h:382: // TODO: remove when GrRenderTarget is-a
SkSurface.
> Do you mean to make GrRenderTarget be a subclass of SkSurface?

Yeah, I kind of envisioned that it could be one possible end scenario (how many
surface classes does a library need?). However, that particular call-site is
most likely not the elegant way to associate the surface and the device. Thus
the remark is better to maybe left out of the discussions at the moment, maybe
it's not central to the problem I'm trying to fix in the end (which is still the
render targets for MSAA..) . IMO better way to do the association is in the
patch above (https://codereview.chromium.org/912243004)

[Kimmo Kinnunen, 2015-02-17 14:11:24]

On 2015/02/16 14:56:38, Kimmo Kinnunen wrote:
> On 2015/02/13 14:24:35, bsalomon wrote:
> > Kimmo, you're a brave soul! :)
> > 
> > Moving image filters towards surface and image is absolutely the right way
to
> > go.
> > 
> > This patch inspired internal talk about IFs fit into the pipeline and how we
> > might improve them overall.
> 
> Sure. Unfortunately it requires even more refactoring.
> Either SkImages need to be constructable from some sort of SkBaseDevice state,
> or all SkBaseDevices need to be backed by a SkSurface.

I think the latter is quite messy. On top of the problems mentioned in the above
message, there seems to be the problem that it would necessarily cause ref loop.
The loop comes from the fact that if device relies on the surface to do the
snapshot, then the surface must live at least as long as the device. 

The other option, eg. making SkImage* SkBaseDevice::newSnapshotImage() and then
calling that in place of SkBaseDevice::accessBitmap() seems to be more viable?

The approach could maybe be built as the logical continuation of following
patch:
https://codereview.chromium.org/926843003

[Kimmo Kinnunen, 2015-02-18 15:14:13]

On 2015/02/17 14:11:24, Kimmo Kinnunen wrote:
> The other option, eg. making SkImage* SkBaseDevice::newSnapshotImage() and
then
> calling that in place of SkBaseDevice::accessBitmap() seems to be more viable?

This is now done in:
https://chromiumcodereview.appspot.com/933043006/ 

(few test failures left, but the idea should be maybe identifiable)

[reed2, 2015-02-24 23:18:13]

reed@chromium.org changed reviewers:
+ reed@chromium.org

[reed2, 2015-02-24 23:18:14]

I really like the effect of this CL. What is its status?

One thing I like is it removes the bulk of the callers of
SkDevice::accessBitmap... which I'd like to remove (or at least make private).

[Kimmo Kinnunen, 2015-02-25 13:45:07]

On 2015/02/24 23:18:14, reed2 wrote:
> I really like the effect of this CL. What is its status?

This works now.

[reed1, 2015-02-25 16:36:21]

I see that one somewhat orthogonal change is adding kImage to DrawFilter. I can
do that now, separately, since it makes sense regardless of anything else, it it
would reduce the size (a little) of this CL.

https://codereview.chromium.org/920513003/diff/120001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/920513003/diff/120001/include/core/SkCanvas.h...
include/core/SkCanvas.h:1231: virtual void onDrawSprite(const SkImage&, int
left, int top, const SkPaint*);
same nit about shadowing virtuals w/ the same name. onDrawImageAsSprite?

https://codereview.chromium.org/920513003/diff/120001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/920513003/diff/120001/include/core/SkDevice.h...
include/core/SkDevice.h:210: virtual void drawSprite(const SkDraw&, const
SkImage& bitmap,
naming nit: two virtuals w/ the same name are tricky (shadowing, etc.). Skia
tries to never do this. Why not drawImageAsSprite() so we still capture the
semantic that the CTM is to be ignored.

[Kimmo Kinnunen, 2015-02-26 12:24:01]

On 2015/02/25 16:36:21, reed1 wrote:
> I see that one somewhat orthogonal change is adding kImage to DrawFilter. I
can
> do that now, separately, since it makes sense regardless of anything else, it
it
> would reduce the size (a little) of this CL.
> 
> https://codereview.chromium.org/920513003/diff/120001/include/core/SkCanvas.h
> File include/core/SkCanvas.h (right):
> 
>
https://codereview.chromium.org/920513003/diff/120001/include/core/SkCanvas.h...
> include/core/SkCanvas.h:1231: virtual void onDrawSprite(const SkImage&, int
> left, int top, const SkPaint*);
> same nit about shadowing virtuals w/ the same name. onDrawImageAsSprite?
> 
> https://codereview.chromium.org/920513003/diff/120001/include/core/SkDevice.h
> File include/core/SkDevice.h (right):
> 
>
https://codereview.chromium.org/920513003/diff/120001/include/core/SkDevice.h...
> include/core/SkDevice.h:210: virtual void drawSprite(const SkDraw&, const
> SkImage& bitmap,
> naming nit: two virtuals w/ the same name are tricky (shadowing, etc.). Skia
> tries to never do this. Why not drawImageAsSprite() so we still capture the
> semantic that the CTM is to be ignored.
> I see that one somewhat orthogonal change is adding kImage to DrawFilter. I
can
> do that now, separately, since it makes sense regardless of anything else, it
it
> would reduce the size (a little) of this CL.

Ok. I separated that to be able to compile. We can use yours. Mine is at:
https://chromiumcodereview.appspot.com/960783003/



>
https://codereview.chromium.org/920513003/diff/120001/include/core/SkDevice.h...
> include/core/SkDevice.h:210: virtual void drawSprite(const SkDraw&, const
> SkImage& bitmap,
> naming nit: two virtuals w/ the same name are tricky (shadowing, etc.). Skia
> tries to never do this. Why not drawImageAsSprite() so we still capture the
> semantic that the CTM is to be ignored.
 
Done. 
BTW, it's not the ctm that's important, it's the semantic of filters being
applied..