Fix some extract subset bugs.

Fix some extract subset bugs.

In SkBitmap::extractSubset, perform a deepCopy, if the pixelRef supports it.

Fixes a bug in the 'extractbitmap' gm, which attempts to draw a subset of a texture backed bitmap (if the canvas is really an SkGpuCanvas).

Also fix some bugs that happen when there is a pixel offset. These fixes get bypassed by the deepCopy, but a user can still set a pixel offset manually.

When copying GPU backed bitmap with a pixel offset, copy the offset.

If the new config is the same as the old, copy fRowBytes as well.

Add a function to SkBitmap.cpp (getUpperLeftFromOffset) to find the x,y coordinate to use when copying to a new config.

Fix a bug where readPixels copied to the correct desired config and we were setting the generation ID to match even though the desired config was not the same as the original config (caught by my new tests!).

Add some tests to verify the correct behavior.

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

[bsalomon, 2012-11-13 15:44:36]

Would it make sense to have pixel ref track the x/y either instead of or in
addition to the offset?

Does a double-extract work?

https://codereview.appspot.com/6839043/diff/4001/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

https://codereview.appspot.com/6839043/diff/4001/src/core/SkBitmap.cpp#newcod...
src/core/SkBitmap.cpp:882: switch (bm.getConfig()) {
Is there no BPP helper that we can call here?

[Leon, 2012-11-13 16:49:01]

On 2012/11/13 15:44:36, bsalomon wrote:
> Would it make sense to have pixel ref track the x/y either instead of or in
> addition to the offset?

That is certainly doable, although I'm not sure yet what is the best
combination. Storing both could lead to the two conflicting, and we may have to
change the API. Currently we have a function that allows the caller to set the
SkPixelRef and an offset. If we change this (not sure whether it's safe to
change) we could instead offer

setPixelRef(SkPixelRef, int x, int y)

Then we would do the calculation for fPixelRefOffset inside setPixelRef (unless
we want to calculate it each time we need it). If we leave the old API, then we
would still need getUpperLeftFromOffset for callers who specify a pixel offset.
Ultimately we only need it when calling copyTo on a GPU-backed bitmap (which is
actually not the preferred way of copying), so I think it would make the most
sense to calculate it on demand.

> 
> Does a double-extract work?

Contrary to what I said before, I think it does. extractSubset computes the
offset of the (x,y), and then adds that to fPixelRefOffset. I have not tested it
though.

> 
> https://codereview.appspot.com/6839043/diff/4001/src/core/SkBitmap.cpp
> File src/core/SkBitmap.cpp (right):
> 
>
https://codereview.appspot.com/6839043/diff/4001/src/core/SkBitmap.cpp#newcod...
> src/core/SkBitmap.cpp:882: switch (bm.getConfig()) {
> Is there no BPP helper that we can call here?

*x >> (fBytesPerPixel >> 1);

which would do the right thing for 565, 4444, 8888, A8 and Index8. No_Config and
A8_Config would no longer return false, but we could handle that differently.

__________________________________________________________________________________________________________________________________

Brian also suggested that we change extractSubset to perform a deepCopy
initially, fixing this problem another way.

[Leon, 2012-12-06 21:56:30]

PTAL.

I have changed it so the deepCopy is done in extractSubset, although the fact
that anyone can call setPixelRef with an offset makes me think the other changes
should still be taken. Let me know if you disagree though.

[bsalomon, 2012-12-06 22:16:17]

I think we have some deep problems with SkBitmap's interface WRT texture backed
bitmaps. That said, this seems to make things better.

https://codereview.appspot.com/6839043/diff/11001/include/gpu/SkGrPixelRef.h
File include/gpu/SkGrPixelRef.h (right):

https://codereview.appspot.com/6839043/diff/11001/include/gpu/SkGrPixelRef.h#...
include/gpu/SkGrPixelRef.h:62: const SkIRect* subset = NULL) SK_OVERRIDE;
prolly shouldn't respecify default param in subclass.

https://codereview.appspot.com/6839043/diff/11001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6839043/diff/11001/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:1449: SK_Scalar1 * dst->width(),
I think we should either:

a) Assert that (topLeft.XY + dst.WH) is <= src.WH.

-or-

b) clip rect's bottom/right to (src.WH - topLeft.XY)


Either way we should document in the header what whether TL.XY + dst.WH > src.WH
is legal and if so what the behavior is.

I'd vote for clipping.

[Leon, 2012-12-06 23:12:25]

https://codereview.appspot.com/6839043/diff/11001/include/gpu/SkGrPixelRef.h
File include/gpu/SkGrPixelRef.h (right):

https://codereview.appspot.com/6839043/diff/11001/include/gpu/SkGrPixelRef.h#...
include/gpu/SkGrPixelRef.h:62: const SkIRect* subset = NULL) SK_OVERRIDE;
On 2012/12/06 22:16:18, bsalomon wrote:
> prolly shouldn't respecify default param in subclass.

Done.

https://codereview.appspot.com/6839043/diff/11001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6839043/diff/11001/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:1449: SK_Scalar1 * dst->width(),
On 2012/12/06 22:16:18, bsalomon wrote:
> I think we should either:
> 
> a) Assert that (topLeft.XY + dst.WH) is <= src.WH.
> 
> -or-
> 
> b) clip rect's bottom/right to (src.WH - topLeft.XY)
> 
> 
> Either way we should document in the header what whether TL.XY + dst.WH >
src.WH
> is legal and if so what the behavior is.
> 
> I'd vote for clipping.

Good call. I was thinking about an assert, and forgot to include it, but I am
fine with a clip as well.

Done

[bsalomon, 2012-12-07 14:35:47]

https://codereview.appspot.com/6839043/diff/14002/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.appspot.com/6839043/diff/14002/include/gpu/GrContext.h#new...
include/gpu/GrContext.h:595: * Copies all texels from one texture to another.
This should be updated... it doesn't copy all the pixels. We can say it copies a
rect of pixels from src to dst. The size of dst is the size of the rectangle
copied and topLeft is the position of the rect in src. The rect is clipped to
src's bounds.

https://codereview.appspot.com/6839043/diff/14002/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6839043/diff/14002/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:1443: sampleM.setIDiv(src->width(), src->height());
I think this would clip and catch a few more bad cases:

SkIRect srcRect = SkIRect::MakeWH(dst->width(), dst-height());
if (NULL != topLeft) {
    srcRect.offset(*topLeft);
}
SkIRect srcBounds = SkIRect::MakeWH(src->width(), src->height());
if (!srcRect.intersect(srcBounds)) {
    return;
}
sampleM.preTranslate(SkIntToScalar(srcRect.fLeft), SkIntToScalar(srcRect.fTop));
drawState->createTextureEffect(0, src, sampleM);
SkRect dstRect = SkRect::MakeWH(SkIntToScalar(srcRect->width(),
SkIntToScalar(dstRect->height());
fGpu->drawSimpleRect(dstRect, NULL);

[Leon, 2012-12-07 16:28:18]

https://codereview.appspot.com/6839043/diff/14002/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.appspot.com/6839043/diff/14002/include/gpu/GrContext.h#new...
include/gpu/GrContext.h:595: * Copies all texels from one texture to another.
On 2012/12/07 14:35:47, bsalomon wrote:
> This should be updated... it doesn't copy all the pixels. We can say it copies
a
> rect of pixels from src to dst. The size of dst is the size of the rectangle
> copied and topLeft is the position of the rect in src. The rect is clipped to
> src's bounds.

Done.

https://codereview.appspot.com/6839043/diff/14002/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.appspot.com/6839043/diff/14002/src/gpu/GrContext.cpp#newco...
src/gpu/GrContext.cpp:1443: sampleM.setIDiv(src->width(), src->height());
On 2012/12/07 14:35:47, bsalomon wrote:
> I think this would clip and catch a few more bad cases:
> 
> SkIRect srcRect = SkIRect::MakeWH(dst->width(), dst-height());
> if (NULL != topLeft) {
>     srcRect.offset(*topLeft);
> }
> SkIRect srcBounds = SkIRect::MakeWH(src->width(), src->height());
> if (!srcRect.intersect(srcBounds)) {
>     return;
> }
> sampleM.preTranslate(SkIntToScalar(srcRect.fLeft),
SkIntToScalar(srcRect.fTop));
> drawState->createTextureEffect(0, src, sampleM);
> SkRect dstRect = SkRect::MakeWH(SkIntToScalar(srcRect->width(),
> SkIntToScalar(dstRect->height());
> fGpu->drawSimpleRect(dstRect, NULL);

Done.

[bsalomon, 2012-12-07 16:29:25]

On 2012/12/07 16:28:18, scroggo-work wrote:
> https://codereview.appspot.com/6839043/diff/14002/include/gpu/GrContext.h
> File include/gpu/GrContext.h (right):
> 
>
https://codereview.appspot.com/6839043/diff/14002/include/gpu/GrContext.h#new...
> include/gpu/GrContext.h:595: * Copies all texels from one texture to another.
> On 2012/12/07 14:35:47, bsalomon wrote:
> > This should be updated... it doesn't copy all the pixels. We can say it
copies
> a
> > rect of pixels from src to dst. The size of dst is the size of the rectangle
> > copied and topLeft is the position of the rect in src. The rect is clipped
to
> > src's bounds.
> 
> Done.
> 
> https://codereview.appspot.com/6839043/diff/14002/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.appspot.com/6839043/diff/14002/src/gpu/GrContext.cpp#newco...
> src/gpu/GrContext.cpp:1443: sampleM.setIDiv(src->width(), src->height());
> On 2012/12/07 14:35:47, bsalomon wrote:
> > I think this would clip and catch a few more bad cases:
> > 
> > SkIRect srcRect = SkIRect::MakeWH(dst->width(), dst-height());
> > if (NULL != topLeft) {
> >     srcRect.offset(*topLeft);
> > }
> > SkIRect srcBounds = SkIRect::MakeWH(src->width(), src->height());
> > if (!srcRect.intersect(srcBounds)) {
> >     return;
> > }
> > sampleM.preTranslate(SkIntToScalar(srcRect.fLeft),
> SkIntToScalar(srcRect.fTop));
> > drawState->createTextureEffect(0, src, sampleM);
> > SkRect dstRect = SkRect::MakeWH(SkIntToScalar(srcRect->width(),
> > SkIntToScalar(dstRect->height());
> > fGpu->drawSimpleRect(dstRect, NULL);
> 
> Done.

If it all works then LGTM.