Extend pixelref to receive the CTM, and to return a (new) width/height

Extend pixelref to receive the CTM, and to return a (new) width/height

[reed1, 2012-12-10 20:03:44]

prototype for extended api on pixelref. Lots more changes needed to our
subclasses (and call sites), but wanted to float the API past you right away.

[Alpha, 2012-12-10 20:10:16]

Question:

How to handle a case where a pixelref is used for different scale? Will Skia
lock and unlock for a specific scale? Will there be nested locks with different
scales?

[reed1, 2012-12-10 20:12:37]

The matrix parameter is how I tell the pixelRef what scale I plan to use. The
pixelRef can use that to decide to return a smaller (or larger (gulp)) image in
Info.

[Alpha, 2012-12-10 20:16:44]

My question was, will OnLockPixels be called consecutively with different
matrixes? e.g.

OnLockPixels(.., matA);
OnLockPixels(.., matB);
OnUnlockPixels();
OnUnlockPixels();

or

OnLockPixels(.., matA);
OnUnlockPixels();
OnLockPixels(.., matB);
OnUnlockPixels();

[reed1, 2012-12-10 20:40:30]

Ah!

Good question. It sure seems that we'd want a different protocol if we needed to
support nested locks in this new crazy world

lock(matrixA)
lock(matrixB)
unlock
unlock

We'd want to (at least) pass something to the unlocker to know which one we
mean... Yuck. Maybe this is the wrong way to support custom decoding.

Alpha, what are your plans/hopes around this API, and in what timeframe?

[Alpha, 2012-12-10 20:59:40]

I think it's important for us to have some kind of API so we can experiment
strategies.

This case is probably rare, I think it's okay we proceed with Skia to
choose an optimal scale. Which means the sequence will be:

lock(matA)
  -> OnLockPixels(matA)
lock(matB)
  ignore and skia perform scale
unlock
unlock



2012/12/10 <reed@google.com>

> Ah!
>
> Good question. It sure seems that we'd want a different protocol if we
> needed to support nested locks in this new crazy world
>
> lock(matrixA)
> lock(matrixB)
> unlock
> unlock
>
> We'd want to (at least) pass something to the unlocker to know which one
> we mean... Yuck. Maybe this is the wrong way to support custom decoding.
>
> Alpha, what are your plans/hopes around this API, and in what timeframe?
>
>
https://codereview.appspot.**com/6904072/<https://codereview.appspot.com/6904...
>

[bsalomon, 2012-12-11 16:32:03]

For the GPU it'd be nice if the pixel itself could determine the row bytes
rather than the bitmap telling the pixel ref what row bytes to use.

https://codereview.appspot.com/6904072/diff/14001/include/core/SkMallocPixelR...
File include/core/SkMallocPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/14001/include/core/SkMallocPixelR...
include/core/SkMallocPixelRef.h:24: SkMallocPixelRef(void* addr, const Info&,
bool weShouldFreePixels = true);
Hmm... "we" is somewhat ambiguous... how about pixelRefShouldFreePixels or
pixelRefOwnsPixels.

https://codereview.appspot.com/6904072/diff/14001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/14001/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:57: int                 fWidth;
You mean the pixel ref will know the dimensions!? Blasphemy.... sweet sweet
blasphemy. +100

[Leon, 2012-12-11 17:08:02]

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

https://codereview.appspot.com/6904072/diff/12001/include/core/SkBitmap.h#new...
include/core/SkBitmap.h:674: friend class SkPixelRef;
I do not see SkPixelRef taking advantage of this.

https://codereview.appspot.com/6904072/diff/12001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/12001/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:60: SkColorTable*       fColorTable;
Should we add a comment that this is just a pointer, and the subclass refs as
necessary?

https://codereview.appspot.com/6904072/diff/12001/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:116: *  Call to balanace a previous call to
lockPixels(). Returns the pixels
Returns void.

https://codereview.appspot.com/6904072/diff/12001/src/core/SkMallocPixelRef.cpp
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.appspot.com/6904072/diff/12001/src/core/SkMallocPixelRef.c...
src/core/SkMallocPixelRef.cpp:51: fInfo.flatten(buffer);
Won't this require bumping the PICTURE_VERSION?

https://codereview.appspot.com/6904072/diff/12001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.appspot.com/6904072/diff/12001/src/core/SkPixelRef.cpp#new...
src/core/SkPixelRef.cpp:134: sk_bzero(&fLockedInfo, sizeof(fLockedInfo));
fLockedInfo.setZero();

https://codereview.appspot.com/6904072/diff/12001/src/core/SkPixelRef.cpp#new...
src/core/SkPixelRef.cpp:196: buffer.writeBool(fColorTable != NULL);
Won't writeFlattenable handle a NULL object?

https://codereview.appspot.com/6904072/diff/12001/src/ports/SkImageRef_ashmem.h
File src/ports/SkImageRef_ashmem.h (right):

https://codereview.appspot.com/6904072/diff/12001/src/ports/SkImageRef_ashmem...
src/ports/SkImageRef_ashmem.h:35: virtual void* onLockPixels(Info*, const
SkMatrix*) SK_OVERRIDE;
Need to change this in SkImageRef_ashmem.cpp as well.

https://codereview.appspot.com/6904072/diff/14001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/14001/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:226: virtual void* onLockPixels(Info* info, const
SkMatrix* ctm) = 0;
Maybe there should be a comment here that info will never be NULL? I noticed
that the current subclasses always assume that it is non-NULL, but the only way
to know that is by looking at the implementation of lockPixels.

[DerekS, 2012-12-11 18:13:28]

https://codereview.appspot.com/6904072/diff/14001/src/ports/SkImageRef_ashmem.h
File src/ports/SkImageRef_ashmem.h (right):

https://codereview.appspot.com/6904072/diff/14001/src/ports/SkImageRef_ashmem...
src/ports/SkImageRef_ashmem.h:35: virtual void* onLockPixels(Info*, const
SkMatrix*) SK_OVERRIDE;
You need to update the cpp file or this will fail on android builds

[reed1, 2012-12-11 18:21:23]

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

https://codereview.appspot.com/6904072/diff/12001/include/core/SkBitmap.h#new...
include/core/SkBitmap.h:674: friend class SkPixelRef;
On 2012/12/11 17:08:02, scroggo-work wrote:
> I do not see SkPixelRef taking advantage of this.

Add comment: so pixelref can call ComputeSafeSize64()

https://codereview.appspot.com/6904072/diff/12001/src/core/SkMallocPixelRef.cpp
File src/core/SkMallocPixelRef.cpp (right):

https://codereview.appspot.com/6904072/diff/12001/src/core/SkMallocPixelRef.c...
src/core/SkMallocPixelRef.cpp:51: fInfo.flatten(buffer);
On 2012/12/11 17:08:02, scroggo-work wrote:
> Won't this require bumping the PICTURE_VERSION?

Why yes, yes it will. (unless you see a way around that).

https://codereview.appspot.com/6904072/diff/12001/src/core/SkPixelRef.cpp
File src/core/SkPixelRef.cpp (right):

https://codereview.appspot.com/6904072/diff/12001/src/core/SkPixelRef.cpp#new...
src/core/SkPixelRef.cpp:196: buffer.writeBool(fColorTable != NULL);
On 2012/12/11 17:08:02, scroggo-work wrote:
> Won't writeFlattenable handle a NULL object?

Hmmm, probably yes. This is not new logic, but I think you're right, and since
I'm bumping the version anyway, I will try that change.

https://codereview.appspot.com/6904072/diff/12001/src/ports/SkImageRef_ashmem.h
File src/ports/SkImageRef_ashmem.h (right):

https://codereview.appspot.com/6904072/diff/12001/src/ports/SkImageRef_ashmem...
src/ports/SkImageRef_ashmem.h:35: virtual void* onLockPixels(Info*, const
SkMatrix*) SK_OVERRIDE;
On 2012/12/11 17:08:02, scroggo-work wrote:
> Need to change this in SkImageRef_ashmem.cpp as well.

Done. Sure wish I knew how to test that...

https://codereview.appspot.com/6904072/diff/14001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/14001/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:226: virtual void* onLockPixels(Info* info, const
SkMatrix* ctm) = 0;
On 2012/12/11 17:08:02, scroggo-work wrote:
> Maybe there should be a comment here that info will never be NULL? I noticed
> that the current subclasses always assume that it is non-NULL, but the only
way
> to know that is by looking at the implementation of lockPixels.

Done.

[Leon, 2012-12-11 18:44:20]

Overall lgtm

https://codereview.appspot.com/6904072/diff/10002/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/10002/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:227: *  The caller is always lockPixels(), and it is
guaranteed to always pass
The caller is always called by*

https://codereview.appspot.com/6904072/diff/10002/src/ports/SkImageRef_ashmem...
File src/ports/SkImageRef_ashmem.cpp (right):

https://codereview.appspot.com/6904072/diff/10002/src/ports/SkImageRef_ashmem...
src/ports/SkImageRef_ashmem.cpp:161: void* SkImageRef_ashmem::onLockPixels(Info*
info, const SkMatrix* ctm) {
Looks like this will build now, but we won't handle the ColorTable properly.

Not sure it's worth fixing right now, since no one is using it, but should we at
least make a comment? Or perhaps we should stop building it again?

[Leon, 2012-12-11 19:01:48]

https://codereview.appspot.com/6904072/diff/10002/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/10002/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:227: *  The caller is always lockPixels(), and it is
guaranteed to always pass
On 2012/12/11 18:44:20, scroggo-work wrote:
> The caller is always called by*

D'oh - my comment makes little sense...

I think I was thinking

"Always called by lockPixels()"

But it looks fine as is.

https://codereview.appspot.com/6904072/diff/10002/src/ports/SkImageRef_ashmem...
File src/ports/SkImageRef_ashmem.cpp (right):

https://codereview.appspot.com/6904072/diff/10002/src/ports/SkImageRef_ashmem...
src/ports/SkImageRef_ashmem.cpp:161: void* SkImageRef_ashmem::onLockPixels(Info*
info, const SkMatrix* ctm) {
On 2012/12/11 18:44:20, scroggo-work wrote:
> Looks like this will build now, but we won't handle the ColorTable properly.
> 
> Not sure it's worth fixing right now, since no one is using it, but should we
at
> least make a comment? Or perhaps we should stop building it again?

Nvm, this looks good.

[Alpha, 2012-12-11 19:56:55]

+nduca, enne who are interested in this

[Alpha, 2012-12-11 21:28:44]

On 2012/12/11 19:56:55, Alpha wrote:
> +nduca, enne who are interested in this

lgtm by the way.

[reed1, 2012-12-12 13:22:37]

I have begun to try to actually call this new entry-point, and am running into
problems. Skia itself calls it early on, before it has a matrix, and then calls
lock again (presumably w/ a matrix). This will effectively kill the 2nd call,
since the pixelref will have cached the non-matrix version... I continue to
contemplate.

[reed1, 2012-12-14 21:19:36]

patchset #8 -- rebaselined

Next step is to actually call the api, and correctly handle the (possibly
different) width/height that is returned.

[nduca, 2012-12-19 04:53:59]

https://codereview.appspot.com/6904072/diff/24001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/24001/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:92: *  was returned on the first call, which may or
may not be optimal for the
Just to ask a dumb question, which layer causes this restriction? Is this a
skia-side restriction, a webkit-side restriction, a limitation of the interface?
Or maybe its a "this is hard enough as it is, lets make some progress"? Nbd,
just trying to figure things out...

[nduca, 2012-12-19 05:13:09]

https://codereview.appspot.com/6904072/diff/24001/include/core/SkPixelRef.h
File include/core/SkPixelRef.h (right):

https://codereview.appspot.com/6904072/diff/24001/include/core/SkPixelRef.h#n...
include/core/SkPixelRef.h:97: void* lockPixels(Info* info, const SkMatrix* ctm =
NULL);
also, should this be another structure like
LockInfo {
SkMatrix ctm
}

That way if we decide in the future we want to pass down other state to it, say
the quality settings or filters etc [pulling something out of my ass], we could
without all the rolling issues that come from adding an arg?