Lazy allocate pixels in SkCanvas::readPixels

[bsalomon, 2011-10-31 20:51:44]

I'd like to remove the public readPixels from SkDevice but it is called directly
from WebKit today.

http://codereview.appspot.com/5307077/diff/2001/src/core/SkDevice.cpp
File src/core/SkDevice.cpp (right):

http://codereview.appspot.com/5307077/diff/2001/src/core/SkDevice.cpp#newcode185
src/core/SkDevice.cpp:185: if (SkBitmap::kARGB_8888_Config != subset.config()) {
This implementation could use some work, see comment inside the if block.

[bsalomon, 2011-11-02 14:27:07]

New version uses x,y,bitmap->width(),bitmap->height() to define the src
rectangle. Also, I reimplemented the old API still in use by WK on top of new
API.

[reed1, 2011-11-02 17:41:02]

http://codereview.appspot.com/5307077/diff/2001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/5307077/diff/2001/include/core/SkCanvas.h#newco...
include/core/SkCanvas.h:110: *  not overwritten.
+= If the bitmap's config is not supported (e.g. not kARGB_8888 today) then
false will be returned, and the bitmap will be unchanged.

http://codereview.appspot.com/5307077/diff/3020/include/core/SkCanvas.h
File include/core/SkCanvas.h (left):

http://codereview.appspot.com/5307077/diff/3020/include/core/SkCanvas.h#oldco...
include/core/SkCanvas.h:112: *  overwritten (if the method returns true).
Another stab at the comment. Not sure its any better, but I think this comment
is really important to get right.


/**
 *  On success (returns true), copy the canvas pixels into the bitmap.
 *  On failure, the bitmap parameter is left unchanged and false is returned.
 *
 *  If the bitmap has pixels already allocated, the canvas pixels will be
 *  written there. If not, bitmap->allocPixels() will be called automatically.
 *  The only supported config is kARGB_8888_Config, though this may be relaxed
 *  in the future.
 *
 *  The actual pixels written is the intersection of the canvas' bounds, and
 *  the rectangle formed by the bitmap's width,height and the specified x,y.
 *  If bitmap pixels extend outside of that intersection, they will not be
 *  modified.
 */

http://codereview.appspot.com/5307077/diff/3020/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

http://codereview.appspot.com/5307077/diff/3020/include/core/SkCanvas.h#newco...
include/core/SkCanvas.h:133: */
do we really need this guy?

http://codereview.appspot.com/5307077/diff/3020/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

http://codereview.appspot.com/5307077/diff/3020/src/core/SkCanvas.cpp#newcode570
src/core/SkCanvas.cpp:570: !bitmap->getTexture() &&
shouldn't this be bitmap->width() >= device->width(), instead of == ?

[bsalomon, 2011-11-02 18:01:53]

Updated comment and removed the whole-canvas version. (Bracing for comment about
"non-raster")

[bsalomon, 2011-11-02 19:27:52]

On 2011/11/02 18:01:53, bsalomon wrote:
> Updated comment and removed the whole-canvas version. (Bracing for comment
about
> "non-raster")

I ran the regular and gpu layout tests against this. Of course there were
failures, but none related to readPixels.

[bsalomon, 2011-11-02 19:35:22]

I went ahead and landed this since it was blocking me. We can make improvements
to the comments in a follow up change.

[reed1, 2011-11-02 19:43:21]

http://codereview.appspot.com/5307077/diff/14001/include/core/SkDevice.h
File include/core/SkDevice.h (right):

http://codereview.appspot.com/5307077/diff/14001/include/core/SkDevice.h#newc...
include/core/SkDevice.h:279: */
if its const, lets make the bitmap be a ref

const SkBitmap&