Add exportAllPixels() and exportRowPixels() to SkBitmap.

Add exportAllPixels() and exportRowPixels() to SkBitmap.

[epoger, 2012-11-29 10:30:06]

For now, I have written only the method declaration, to see if you're on board.

Does this seem useful, or a step in the wrong direction?

(The reason I'm looking into this is that I need to compute a checksum of an
SkBitmap's complete pixel data, with the bytes swizzled so as to be in
consistent RGBA order.  Seems like I have this need in common with the PNG
encoder.)

[epoger, 2012-11-29 10:37:32]

+schenney, who might have some thoughts as to whether this might be useful for
Chrome/WebKit code

[Stephen Chenney, 2012-11-29 13:53:37]

I looked that the WebKit code that I am familiar with (SVG filters, not canvas).
There are methods that copy one buffer to another with a color conversion that
would make use of the method you expose. There is another which does pre-mul
color validation that probably would not benefit, but which is rare.

However, we do almost all filters on hardware or inside Skia now and I do not
even think the WebKit buffer copy code is still called by Chrome.

I'm not at all against adding the methods, I just don't think they will help us
in WebKit SVG drawing.

[bsalomon, 2012-11-29 13:57:18]

https://codereview.appspot.com/6849119/diff/1/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/6849119/diff/1/include/core/SkBitmap.h#newcode290
include/core/SkBitmap.h:290: enum ExportFormat {
I think we should use SkCanvas::Config8888 here (moving or renaming it if
desired) rather than adding yet another config/format enum.

https://codereview.appspot.com/6849119/diff/1/include/core/SkBitmap.h#newcode337
include/core/SkBitmap.h:337: bool exportRowPixels(void* const dst, size_t
dstSize, ExportFormat format,
Should we make the name/signature for reading to external formats match
SkCanvas, either as it is now or deciding on something new for both? SkBitmap's
pixel-copying API already seems overly complicated now with copyTo/deepCopyTo.

I'm not a fan of making the caller do some computation just to prove that they
understand the contract of the function. We could provide a function that
computes the dstSize given an SkBitmap/external format that a client could use
to allocate their buffer if they like.

[reed1, 2012-11-29 14:22:18]

I'm pretty conservative in what I would add to a public header.

If we see a chance to share code, that's always good. Perhaps a private utils
API? 
Does this interface need to be about bitmaps, as much as just pixel-format
converters?

[epoger, 2012-11-29 17:26:06]

Thanks for the comments, all.  Here is my counter-proposal for a different
approach… what do you think?


1. In https://code.google.com/p/skia/source/browse/trunk/include/core/SkCanvas.h
, rename the SkCanvas::Config8888 enum as SkCanvas::PixelBufferFormat .  

No new values will be added yet, but it will give us the flexibility to add some
3-bytes-per-pixel formats later (for feature parity with
transform_scanline_888() in
https://code.google.com/p/skia/source/browse/trunk/src/images/transform_scanl...
).


2. Declare an SkBitmapTransformer interface (abstract base class), in
src/core/SkBitmapTransformer.h , ultimately intended to replace
src/core/SkConfig8888.h . By its location within src/, it would be private for
now; we might make it public later.  

See the code in https://codereview.appspot.com/6843123/ ('Add new
SkBitmapTransformer interface')


3. Replace src/core/SkConfig8888.cpp with an initial implementation of
SkBitmapTransformer, named SkConsistentBitmapTransformer, which uses the same
slow(ish) algorithm as SkConfig8888.cpp and supports the same set of
PixelBufferFormats.


4. Replace src/images/transform_scanline.h with a second implementation of
SkBitmapTransformer, named SkFastBitmapTransformer.  For now, this will use the
same algorithm as transform_scanline.h, but we may tweak this for performance
later.

This implementation will support a different set of PixelBufferFormats than
SkConsistentBitmapTransformer, which will become apparent to callers when they
call CreateBitmapTransformer().

https://codereview.appspot.com/6849119/diff/1/include/core/SkBitmap.h
File include/core/SkBitmap.h (right):

https://codereview.appspot.com/6849119/diff/1/include/core/SkBitmap.h#newcode149
include/core/SkBitmap.h:149: // What is extractBitmap?  I can't find a method by
that name.
I'm still looking for an answer to this question.  Anyone?

[bsalomon, 2012-11-29 17:38:18]

On 2012/11/29 17:26:06, epoger wrote:
include/core/SkBitmap.h (right):
> 
>
https://codereview.appspot.com/6849119/diff/1/include/core/SkBitmap.h#newcode149
> include/core/SkBitmap.h:149: // What is extractBitmap?  I can't find a method
by
> that name.
> I'm still looking for an answer to this question.  Anyone?

The comment should say "extractSubset".

[bsalomon, 2012-11-29 17:43:52]

On 2012/11/29 17:26:06, epoger wrote:
> Thanks for the comments, all.  Here is my counter-proposal for a different
> approach… what do you think?
> 
> 
> 1. In
https://code.google.com/p/skia/source/browse/trunk/include/core/SkCanvas.h
> , rename the SkCanvas::Config8888 enum as SkCanvas::PixelBufferFormat .  
> 
> No new values will be added yet, but it will give us the flexibility to add
some
> 3-bytes-per-pixel formats later (for feature parity with
> transform_scanline_888() in
>
https://code.google.com/p/skia/source/browse/trunk/src/images/transform_scanl...
> ).

Pixel buffer is a somewhat overloaded word (in X and GL). Maybe ExternalConfig?
Kind of matches SkBitmap::Config. Also, not sure SkCanvas rather than SkBitmap
is the right place for it. Mike may have some stronger sentiments about this
than I do.

> 
> 
> 2. Declare an SkBitmapTransformer interface (abstract base class), in
> src/core/SkBitmapTransformer.h , ultimately intended to replace
> src/core/SkConfig8888.h . By its location within src/, it would be private for
> now; we might make it public later.  
> 
> See the code in https://codereview.appspot.com/6843123/ ('Add new
> SkBitmapTransformer interface')
> 
> 
> 3. Replace src/core/SkConfig8888.cpp with an initial implementation of
> SkBitmapTransformer, named SkConsistentBitmapTransformer, which uses the same
> slow(ish) algorithm as SkConfig8888.cpp and supports the same set of
> PixelBufferFormats.
> 
> 
> 4. Replace src/images/transform_scanline.h with a second implementation of
> SkBitmapTransformer, named SkFastBitmapTransformer.  For now, this will use
the
> same algorithm as transform_scanline.h, but we may tweak this for performance
> later.
> 
> This implementation will support a different set of PixelBufferFormats than
> SkConsistentBitmapTransformer, which will become apparent to callers when they
> call CreateBitmapTransformer().
> 
> https://codereview.appspot.com/6849119/diff/1/include/core/SkBitmap.h
> File include/core/SkBitmap.h (right):

I'm wondering if we really need the complexity of a base class. Maybe a flag
indicating "fast vs roundtrip preserving" (or something like that)?

[epoger, 2012-11-29 18:12:31]

> Pixel buffer is a somewhat overloaded word (in X and GL). Maybe
ExternalConfig?
> Kind of matches SkBitmap::Config. Also, not sure SkCanvas rather than SkBitmap
> is the right place for it. Mike may have some stronger sentiments about this
> than I do.

Whatever you guys like is fine with me.

> > This implementation will support a different set of PixelBufferFormats than
> > SkConsistentBitmapTransformer, which will become apparent to callers when
they
> > call CreateBitmapTransformer().
> 
> I'm wondering if we really need the complexity of a base class. Maybe a flag
> indicating "fast vs roundtrip preserving" (or something like that)?

Are you concerned about performance issues due to inheritance or mental
complexity?

I don't know about performance implications, but from an OO standpoint it seems
like a good fit to me (there are multiple implementations of the same interface,
with different qualities).

[bsalomon, 2012-11-29 19:24:23]

On 2012/11/29 18:12:31, epoger wrote:
> > Pixel buffer is a somewhat overloaded word (in X and GL). Maybe
> ExternalConfig?
> > Kind of matches SkBitmap::Config. Also, not sure SkCanvas rather than
SkBitmap
> > is the right place for it. Mike may have some stronger sentiments about this
> > than I do.
> 
> Whatever you guys like is fine with me.
> 
> > > This implementation will support a different set of PixelBufferFormats
than
> > > SkConsistentBitmapTransformer, which will become apparent to callers when
> they
> > > call CreateBitmapTransformer().
> > 
> > I'm wondering if we really need the complexity of a base class. Maybe a flag
> > indicating "fast vs roundtrip preserving" (or something like that)?
> 
> Are you concerned about performance issues due to inheritance or mental
> complexity?
> 
> I don't know about performance implications, but from an OO standpoint it
seems
> like a good fit to me (there are multiple implementations of the same
interface,
> with different qualities).

I was thinking about mental complexity. There are two types of conversions
today: swizzling and premul. Sometimes both are performed in one call. The
difference between fast/consistent only applies to premul conversions. We could
find that for future external formats fast/consistent doesn't apply or could
have implementations that vary in other ways. For example it won't apply at all
to RGB-only configs.

[epoger, 2012-11-29 19:34:46]

On 2012/11/29 19:24:23, bsalomon wrote:
> On 2012/11/29 18:12:31, epoger wrote:
> > > Pixel buffer is a somewhat overloaded word (in X and GL). Maybe
> > ExternalConfig?
> > > Kind of matches SkBitmap::Config. Also, not sure SkCanvas rather than
> SkBitmap
> > > is the right place for it. Mike may have some stronger sentiments about
this
> > > than I do.
> > 
> > Whatever you guys like is fine with me.
> > 
> > > > This implementation will support a different set of PixelBufferFormats
> than
> > > > SkConsistentBitmapTransformer, which will become apparent to callers
when
> > they
> > > > call CreateBitmapTransformer().
> > > 
> > > I'm wondering if we really need the complexity of a base class. Maybe a
flag
> > > indicating "fast vs roundtrip preserving" (or something like that)?
> > 
> > Are you concerned about performance issues due to inheritance or mental
> > complexity?
> > 
> > I don't know about performance implications, but from an OO standpoint it
> seems
> > like a good fit to me (there are multiple implementations of the same
> interface,
> > with different qualities).
> 
> I was thinking about mental complexity. There are two types of conversions
> today: swizzling and premul. Sometimes both are performed in one call. The
> difference between fast/consistent only applies to premul conversions. We
could
> find that for future external formats fast/consistent doesn't apply or could
> have implementations that vary in other ways. For example it won't apply at
all
> to RGB-only configs.

In my opinion, the fact that implementations may diverge in different ways over
time is actually an argument in FAVOR of a stable interface class, with an
arbitrary number of implementations.

But I'm not a nut about it, and I will be happy either way.  Mike, I await your
"tiebreaker" thoughts, and then I will get to work on those steps 1-4 I listed
previously.

[epoger, 2012-12-12 17:25:06]

Closing this one in favor of https://codereview.appspot.com/6920050/ ('Create
SkBitmapChecksummer and associated SkBitmapTransformer'), just committed as
https://code.google.com/p/skia/source/detail?r=6759 .