start to remove lockPixels from bitmapshader

start to remove lockPixels from bitmapshader

BUG=
R=scroggo@google.com

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

[reed1, 2013-09-09 13:53:21]

This is my place-holder CL for trying to wean Skia off of lockPixels, and to use
the scaledbitmapcache instead. So far, it has inspired 3 simpler CLs (mostly
cleanups), and those have already landed.

[scroggo, 2013-09-10 18:08:31]

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcShade...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcShade...
src/core/SkBitmapProcShader.cpp:83: static bool validForDrawing(const SkBitmap&
bm) {
Style nit: valid_for_drawing

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcShade...
src/core/SkBitmapProcShader.cpp:92: bm.lockPixels();
AutoLockPixels?

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcShade...
src/core/SkBitmapProcShader.cpp:105: if (!fRawBitmap.getTexture() &&
!validForDrawing(fRawBitmap)) {
Should valid_for_drawing check bitmap.getTexture()? It seems like a logical
piece of being valid for drawing.

If we want to take it a bit further, should valid_for_drawing be folded into
SkBitmap::readyToDraw?

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
File src/core/SkBitmapProcState.cpp (left):

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:179: *  If we get here, the caller has requested
either Med or High filter-level
Do these comments no longer apply?

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:258: fFilterLevel = SkPaint::kLow_FilterLevel;
This appears to be a change in behavior: if !mip || scaleSqd >= SK_Scalar1, we
no longer set fFilterLevel to SkPaint::kLow_FilterLevel. Is this intended?

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:252: static bool decodeInto(const SkBitmap& src,
int pow2, SkBitmap* dst) {
Style nit: decode_into

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:253: // new way to decodeInto...
Should there be some more explicit comments, eg FIXME, in progress etc?

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:270: bool SkBitmapProcState::lockBaseBitmap() {
Will this function become more general to lock other versions besides the base
bitmap?

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState.h
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.h:164: bool possiblyScaleImage();
Can you add documentation for what the return value means? If it's "whether the
image was scaled", maybe the name of the function should just be "scaleImage".

[reed1, 2013-09-10 20:28:11]

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcShade...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcShade...
src/core/SkBitmapProcShader.cpp:83: static bool validForDrawing(const SkBitmap&
bm) {
On 2013/09/10 18:08:31, scroggo wrote:
> Style nit: valid_for_drawing

Done.

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcShade...
src/core/SkBitmapProcShader.cpp:92: bm.lockPixels();
On 2013/09/10 18:08:31, scroggo wrote:
> AutoLockPixels?

Done.

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcShade...
src/core/SkBitmapProcShader.cpp:105: if (!fRawBitmap.getTexture() &&
!validForDrawing(fRawBitmap)) {
On 2013/09/10 18:08:31, scroggo wrote:
> Should valid_for_drawing check bitmap.getTexture()? It seems like a logical
> piece of being valid for drawing.

Yes, though I don't see now why we could work with a texture?

> 
> If we want to take it a bit further, should valid_for_drawing be folded into
> SkBitmap::readyToDraw?

Ready to draw today presumes that we *have* called lockPixels (in that it looks
get getPixels()). I am explicitly trying to *not* call lockPixels at this point,
since I don't know the CTM. Perhaps if this no-need-to-lock idea propagates more
widely, we can fold it into that method.

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:252: static bool decodeInto(const SkBitmap& src,
int pow2, SkBitmap* dst) {
On 2013/09/10 18:08:31, scroggo wrote:
> Style nit: decode_into

Done.

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:253: // new way to decodeInto...
On 2013/09/10 18:08:31, scroggo wrote:
> Should there be some more explicit comments, eg FIXME, in progress etc?

Done.

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.cpp:270: bool SkBitmapProcState::lockBaseBitmap() {
On 2013/09/10 18:08:31, scroggo wrote:
> Will this function become more general to lock other versions besides the base
> bitmap?

I think not. I think that functionality belongs in possiblyScaleBitmap, where we
worry about mipmaps etc.

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState.h
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/23591030/diff/7001/src/core/SkBitmapProcState...
src/core/SkBitmapProcState.h:164: bool possiblyScaleImage();
On 2013/09/10 18:08:31, scroggo wrote:
> Can you add documentation for what the return value means? If it's "whether
the
> image was scaled", maybe the name of the function should just be "scaleImage".

Done.

[scroggo, 2013-09-10 21:51:41]

lgtm

https://codereview.chromium.org/23591030/diff/17001/src/core/SkBitmapProcStat...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/23591030/diff/17001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:259: *  If decodeInto() fails, the possibly we
just have an old subclass that
If decodeInto() fails, it is possible* we just have an old subclass that

https://codereview.chromium.org/23591030/diff/17001/src/lazy/SkLazyPixelRef.cpp
File src/lazy/SkLazyPixelRef.cpp (right):

https://codereview.chromium.org/23591030/diff/17001/src/lazy/SkLazyPixelRef.c...
src/lazy/SkLazyPixelRef.cpp:151: static bool initFromInfo(SkBitmap* bm, const
SkImage::Info& info, size_t rowBytes) {
init_from_info*

[reed1, 2013-09-11 14:16:36]

https://codereview.chromium.org/23591030/diff/17001/src/core/SkBitmapProcStat...
File src/core/SkBitmapProcState.cpp (right):

https://codereview.chromium.org/23591030/diff/17001/src/core/SkBitmapProcStat...
src/core/SkBitmapProcState.cpp:259: *  If decodeInto() fails, the possibly we
just have an old subclass that
On 2013/09/10 21:51:41, scroggo wrote:
> If decodeInto() fails, it is possible* we just have an old subclass that

Done.

https://codereview.chromium.org/23591030/diff/17001/src/lazy/SkLazyPixelRef.cpp
File src/lazy/SkLazyPixelRef.cpp (right):

https://codereview.chromium.org/23591030/diff/17001/src/lazy/SkLazyPixelRef.c...
src/lazy/SkLazyPixelRef.cpp:151: static bool initFromInfo(SkBitmap* bm, const
SkImage::Info& info, size_t rowBytes) {
On 2013/09/10 21:51:41, scroggo wrote:
> init_from_info*

Done.

[reed1, 2013-09-11 14:17:31]

Before I try to land, I want to address at least the TODO related to recognizing
the "fast-path" for SkMallocPixelRef, where we *never* need to cache the
original bits.

[reed1, 2013-09-11 16:50:01]

latest should handle the fast-path. ptal

[scroggo, 2013-09-11 20:18:55]

lgtm at patchset 7

[reed1, 2013-09-13 16:05:05]

Committed patchset #8 manually as r11258 (presubmit successful).