refactor bitmapshader to use a controller

refactor bitmapshader to use a controller

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/64045423ddb0159cf857d55e25bad11026355902

[reed1, 2015-06-01 21:01:01]

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

[reed1, 2015-06-01 21:01:01]

#1 needs to change to allow the controller to return more state (e.g. a ref'd
mipmap tree) which can be released when the shader is done with it.

Perhaps we turn the controller into a factory, and the object it returns is
stateful (and contains the pixels), allow the shader to hold onto that instance
until it is done.

class PixmapProvider {
    const SkPixmap& pixmap();
};

PixmapProvider* controller->request(bitmap, matrix, filter);
...
delete provider;

For performance, we can extend the controller to provide some storage, so that
small providers need not actually call new/delete (as we do for
Shader::Context).

[scroggo, 2015-06-02 13:24:54]

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController...
File src/core/SkBitmapController.cpp (right):

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController...
src/core/SkBitmapController.cpp:177: fScaledBitmap.installPixels(info,
level.fPixels, level.fRowBytes);
Should we check the return value? (Even just to assert?)

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController...
src/core/SkBitmapController.cpp:192: if (!state.processHQRequest() &&
!state.processHQRequest()) {
Two questions:
- if inQuality is not kHigh, won't the (first) call to processHQRequest assert?
- is the second call supposed to be processMediumRequest?

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController.h
File src/core/SkBitmapController.h (right):

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController...
src/core/SkBitmapController.h:22: SkBitmap* outBM, SkMatrix* outInv,
SkFilterQuality* outQuality);
Maybe add a comment - it looks like all of these must be non-NULL?

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

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapProcState....
src/core/SkBitmapProcState.cpp:42: static bool valid_for_drawing(const SkBitmap&
bm) {
Can we share this code? (My first thought when I saw the first one was that it's
got to be something we can use in more than one place.)

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

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapProcState....
src/core/SkBitmapProcState.h:136: bool lockBaseBitmap();
The implementation appears to be gone. Remove this too?

[reed1, 2015-06-02 14:07:46]

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController...
File src/core/SkBitmapController.cpp (right):

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController...
src/core/SkBitmapController.cpp:177: fScaledBitmap.installPixels(info,
level.fPixels, level.fRowBytes);
On 2015/06/02 13:24:53, scroggo wrote:
> Should we check the return value? (Even just to assert?)

Done.

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController...
src/core/SkBitmapController.cpp:192: if (!state.processHQRequest() &&
!state.processHQRequest()) {
On 2015/06/02 13:24:54, scroggo wrote:
> Two questions:
> - if inQuality is not kHigh, won't the (first) call to processHQRequest
assert?
> - is the second call supposed to be processMediumRequest?

Done.

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController.h
File src/core/SkBitmapController.h (right):

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapController...
src/core/SkBitmapController.h:22: SkBitmap* outBM, SkMatrix* outInv,
SkFilterQuality* outQuality);
On 2015/06/02 13:24:54, scroggo wrote:
> Maybe add a comment - it looks like all of these must be non-NULL?

New version is simpler/clearer.

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

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapProcState....
src/core/SkBitmapProcState.cpp:42: static bool valid_for_drawing(const SkBitmap&
bm) {
On 2015/06/02 13:24:54, scroggo wrote:
> Can we share this code? (My first thought when I saw the first one was that
it's
> got to be something we can use in more than one place.)

Acknowledged.

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

https://codereview.chromium.org/1153123003/diff/1/src/core/SkBitmapProcState....
src/core/SkBitmapProcState.h:136: bool lockBaseBitmap();
On 2015/06/02 13:24:54, scroggo wrote:
> The implementation appears to be gone. Remove this too?

Done.

[reed1, 2015-06-02 14:07:52]

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

[commit-bot: I haz the power, 2015-06-02 14:07:58]

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

[commit-bot: I haz the power, 2015-06-02 14:09:02]

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

[commit-bot: I haz the power, 2015-06-02 14:09:03]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[reed1, 2015-06-02 20:26:59]

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

[commit-bot: I haz the power, 2015-06-02 20:27:17]

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

[commit-bot: I haz the power, 2015-06-02 20:32:29]

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

[commit-bot: I haz the power, 2015-06-02 20:32:30]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-06-02 21:08:14]

ptal

[scroggo, 2015-06-02 21:17:34]

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
File src/core/SkBitmapController.cpp (right):

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
src/core/SkBitmapController.cpp:211: SkDebugf("");
Did you mean to leave this in?

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
File src/core/SkBitmapController.h (right):

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
src/core/SkBitmapController.h:20: class SkBitmapController {
I tend to default to making classes noncopyable. I think the default is fine
(although unnecessary and likely wrong) to copy, but maybe if it held a cache
that would be a problem?

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
src/core/SkBitmapController.h:22: class State {
Should this be noncopyable?

The only harm/weirdness I can imagine in copying is that presumably
fLockedBitmap should actually be locked, but if you copied a State the copy's
fLockedBitmap would not be (although its SkPixelRef would be).

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
src/core/SkBitmapController.h:41: State* requestBitmap(const SkBitmap&, const
SkMatrix& inverse, SkFilterQuality,
This will matter more once it's public:

Add a comment here that State needs to be SkInPlace->destroy()ed unless the
SkInPlace is NULL, in which case it can be deleted (/SkDELETEd?)

Maybe that's obvious, though, if you know what an SkInPlace is.

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkInPlace.h
File src/core/SkInPlace.h (right):

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkInPlace.h#ne...
src/core/SkInPlace.h:13: class SkInPlace {
Can we merge this with SkSmallAllocator? It looks like SkSmallAllocator sets a
hard limit on the number of objects it will create, so that we could limit the
space we reserve for pointers. It looks like this class only allows 1 object, so
maybe it is a special case of SkSmallAllocator?

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkInPlace.h#ne...
src/core/SkInPlace.h:20: return new (fStorage) T;
SkNEW_PLACEMENT?

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkInPlace.h#ne...
src/core/SkInPlace.h:29: return new (fStorage) T(a1);
SkNEW_PLACEMENT_ARGS?

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkInPlace.h#ne...
src/core/SkInPlace.h:44: template <typename T, typename A1, typename A2,
typename A3> T* create(const A1& a1, const A2& a2, const A3& a3) {
nit: line length

[reed2, 2015-06-03 02:33:34]

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

[reed2, 2015-06-03 02:33:35]

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkInPlace.h
File src/core/SkInPlace.h (right):

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkInPlace.h#ne...
src/core/SkInPlace.h:13: class SkInPlace {
On 2015/06/02 21:17:34, scroggo wrote:
> Can we merge this with SkSmallAllocator? It looks like SkSmallAllocator sets a
> hard limit on the number of objects it will create, so that we could limit the
> space we reserve for pointers. It looks like this class only allows 1 object,
so
> maybe it is a special case of SkSmallAllocator?

SmallAllocator is templated on size, meaning the public signature would have to
know how big the preallocated storage was, which seems odd. Also SA just fails
if the request is too big, whereas InPlace succeeds (by calling new) which seems
correct given that I don't know the size of the client's sublass.

[reed1, 2015-06-04 13:17:03]

removed SkInPlace and just using storage/size since it will (eventually) be a
public api.

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
File src/core/SkBitmapController.cpp (right):

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
src/core/SkBitmapController.cpp:211: SkDebugf("");
On 2015/06/02 21:17:33, scroggo wrote:
> Did you mean to leave this in?

Done.

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
File src/core/SkBitmapController.h (right):

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
src/core/SkBitmapController.h:20: class SkBitmapController {
On 2015/06/02 21:17:33, scroggo wrote:
> I tend to default to making classes noncopyable. I think the default is fine
> (although unnecessary and likely wrong) to copy, but maybe if it held a cache
> that would be a problem?

Done.

https://codereview.chromium.org/1153123003/diff/60001/src/core/SkBitmapContro...
src/core/SkBitmapController.h:22: class State {
On 2015/06/02 21:17:34, scroggo wrote:
> Should this be noncopyable?
> 
> The only harm/weirdness I can imagine in copying is that presumably
> fLockedBitmap should actually be locked, but if you copied a State the copy's
> fLockedBitmap would not be (although its SkPixelRef would be).

Done.

[reed1, 2015-06-04 13:18:08]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-06-04 13:18:18]

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

[commit-bot: I haz the power, 2015-06-04 13:18:21]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-06-04 19:18 UTC

[scroggo, 2015-06-04 13:29:41]

On 2015/06/04 13:17:03, reed1 wrote:
> removed SkInPlace and just using storage/size since it will (eventually) be a
> public api.

I actually like SkInPlace - just looking for ways to share code. Either way,
LGTM

[commit-bot: I haz the power, 2015-06-04 13:31:35]

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

[robertphillips, 2015-06-04 15:27:27]

posthumous lgtm + a question

https://codereview.chromium.org/1153123003/diff/100001/src/core/SkBitmapProcS...
File src/core/SkBitmapProcState.h (right):

https://codereview.chromium.org/1153123003/diff/100001/src/core/SkBitmapProcS...
src/core/SkBitmapProcState.h:131: enum {
Is there any static assert we could write that would cache a gradual change ?