change SkDraw and all Blitters to use pixmap instead of bitmap

change SkDraw and all Blitters to use pixmap instead of bitmap

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/c31af44336f5eb4a50e83e76e51962d46c3ed458

[reed1, 2015-06-05 20:25:05]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 20:25:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148793007/1

[commit-bot: I haz the power, 2015-06-05 20:32:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 20:32:13]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-06-05 20:32:16]

The CQ bit was unchecked by reed@google.com

[reed1, 2015-06-05 21:59:02]

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

[reed1, 2015-06-05 21:59:18]

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

[reed1, 2015-06-05 21:59:24]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-06-05 21:59:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148793007/40001

[commit-bot: I haz the power, 2015-06-05 22:04:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-05 22:05:00]

Dry run: This issue passed the CQ dry run.

[jvanverth1, 2015-06-08 13:21:10]

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrAADistanceFie...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:357: SkAutoPixmapStorage dst;
It's unclear to me why this should be a SkAutoPixmapStorage, whereas the
GrSWMaskHelper continues to use an SkBitmap. I'm sure there's a reason, I just
don't understand the difference.

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:358: // reed: should these be
width/height instead of right/bottom?
I think it's safe the way it is, though admittedly confusing -- pathBounds is
created from devPathBounds.width() and devPathBounds.height(), so right and
bottom should just be the width and height. It does save on two subtracts this
way.

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:360: if (!dst.tryAlloc(bmImageInfo)) {
Change variable name to pmImageInfo?

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrSWMaskHelper....
src/gpu/GrSWMaskHelper.cpp:215: sk_bzero(fBM.getPixels(), fBM.getSafeSize());
Add a comment explaining why we're peeking at the pixels (I'm guessing it's to
set up the pixmap)?

[scroggo, 2015-06-08 15:21:39]

lgtm, modulo Jim's comments.

https://codereview.chromium.org/1148793007/diff/40001/include/core/SkDraw.h
File include/core/SkDraw.h (right):

https://codereview.chromium.org/1148793007/diff/40001/include/core/SkDraw.h#n...
include/core/SkDraw.h:143: //    const SkBitmap* fBitmap;        // required
Can we remove this?

https://codereview.chromium.org/1148793007/diff/40001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:2223: SkBitmap        bm;
It looks like bm is no longer used?

https://codereview.chromium.org/1148793007/diff/40001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:2234: if (!draw.fDst.reset(mask)) {
This is unaffected by the calls to setRect and setTranslate above, right? Should
we move it above those statements? (I doubt it makes a big difference in
practice, but it would mean we could exit earlier.)

https://codereview.chromium.org/1148793007/diff/40001/src/effects/SkLayerRast...
File src/effects/SkLayerRasterizer.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/src/effects/SkLayerRast...
src/effects/SkLayerRasterizer.cpp:130: if (!draw.fDst.reset(*mask)) {
Again, it seems like we could move this error exit case earlier.

https://codereview.chromium.org/1148793007/diff/40001/tests/DeviceLooperTest.cpp
File tests/DeviceLooperTest.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/tests/DeviceLooperTest....
tests/DeviceLooperTest.cpp:12: static void make_pm(SkAutoPixmapStorage* bm, int
w, int h) {
nit: pm or pmap?

[reed1, 2015-06-08 17:00:23]

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrAADistanceFie...
File src/gpu/GrAADistanceFieldPathRenderer.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:357: SkAutoPixmapStorage dst;
On 2015/06/08 13:21:10, jvanverth1 wrote:
> It's unclear to me why this should be a SkAutoPixmapStorage, whereas the
> GrSWMaskHelper continues to use an SkBitmap. I'm sure there's a reason, I just
> don't understand the difference.

Just me being lazy. Now helper also uses pixmaps

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:358: // reed: should these be
width/height instead of right/bottom?
On 2015/06/08 13:21:10, jvanverth1 wrote:
> I think it's safe the way it is, though admittedly confusing -- pathBounds is
> created from devPathBounds.width() and devPathBounds.height(), so right and
> bottom should just be the width and height. It does save on two subtracts this
> way.

Hmmm, I could add asserts, but its starting to feel too complicated to avoid 2
subtracts. I suggest we use width()/height() unless this is seen as
perf-important.

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrAADistanceFie...
src/gpu/GrAADistanceFieldPathRenderer.cpp:360: if (!dst.tryAlloc(bmImageInfo)) {
On 2015/06/08 13:21:10, jvanverth1 wrote:
> Change variable name to pmImageInfo?

removed local var

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrSWMaskHelper.cpp
File src/gpu/GrSWMaskHelper.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/src/gpu/GrSWMaskHelper....
src/gpu/GrSWMaskHelper.cpp:215: sk_bzero(fBM.getPixels(), fBM.getSafeSize());
On 2015/06/08 13:21:10, jvanverth1 wrote:
> Add a comment explaining why we're peeking at the pixels (I'm guessing it's to
> set up the pixmap)?

removed fBM

[reed1, 2015-06-08 17:11:01]

https://codereview.chromium.org/1148793007/diff/40001/src/core/SkDraw.cpp
File src/core/SkDraw.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:2223: SkBitmap        bm;
On 2015/06/08 15:21:38, scroggo wrote:
> It looks like bm is no longer used?

Done.

https://codereview.chromium.org/1148793007/diff/40001/src/core/SkDraw.cpp#new...
src/core/SkDraw.cpp:2234: if (!draw.fDst.reset(mask)) {
On 2015/06/08 15:21:38, scroggo wrote:
> This is unaffected by the calls to setRect and setTranslate above, right?
Should
> we move it above those statements? (I doubt it makes a big difference in
> practice, but it would mean we could exit earlier.)

Done.

https://codereview.chromium.org/1148793007/diff/40001/src/effects/SkLayerRast...
File src/effects/SkLayerRasterizer.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/src/effects/SkLayerRast...
src/effects/SkLayerRasterizer.cpp:130: if (!draw.fDst.reset(*mask)) {
On 2015/06/08 15:21:38, scroggo wrote:
> Again, it seems like we could move this error exit case earlier.

Done.

https://codereview.chromium.org/1148793007/diff/40001/tests/DeviceLooperTest.cpp
File tests/DeviceLooperTest.cpp (right):

https://codereview.chromium.org/1148793007/diff/40001/tests/DeviceLooperTest....
tests/DeviceLooperTest.cpp:12: static void make_pm(SkAutoPixmapStorage* bm, int
w, int h) {
On 2015/06/08 15:21:38, scroggo wrote:
> nit: pm or pmap?

Done.

[scroggo, 2015-06-08 17:30:48]

patch set 5 lgtm

[reed1, 2015-06-08 17:40:23]

The CQ bit was checked by reed@google.com

[reed1, 2015-06-08 17:40:24]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1148793007/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-08 17:40:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1148793007/100001

[commit-bot: I haz the power, 2015-06-08 17:47:22]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/c31af44336f5eb4a50e83e76e51962d46c3ed458

[reed2, 2015-06-09 02:57:35]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1164373003/ by reed@chromium.org.

The reason for reverting is: speculative revert to try to unblock DEPS roll.