making SkGpuDevice::drawBitmap work with GPU-accelerated blur

[junov1, 2011-07-25 21:37:23]

PTAL

http://codereview.appspot.com/4803052/diff/1/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

http://codereview.appspot.com/4803052/diff/1/src/core/SkBitmap.cpp#newcode427
src/core/SkBitmap.cpp:427: return fPixelRef && !isVolatile() ?
fPixelRef->getTexture() : NULL;
Before you ask: this is necessary because the fPixelRef might be shared with
another bitmap which is not volatile. Therefore, fPixelRef->getTexture() may
return non-NULL even when the bitmap is volatile.

[reed1, 2011-07-26 14:10:26]

I need to understand the related bug around subrects and writePixels before
going further on this.

http://codereview.appspot.com/4803052/diff/1/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

http://codereview.appspot.com/4803052/diff/1/src/core/SkBitmap.cpp#newcode427
src/core/SkBitmap.cpp:427: return fPixelRef && !isVolatile() ?
fPixelRef->getTexture() : NULL;
Still don't understand. isVolatile is a hint from the user that the bitmap
should not be cached, since it may not be drawn multiple times. If the caller of
getTexture() cares about that hint, they can check it themselves. I don't see
why we should hide the texture from the caller in this API.

On 2011/07/25 21:37:23, junov1 wrote:
> Before you ask: this is necessary because the fPixelRef might be shared with
> another bitmap which is not volatile. Therefore, fPixelRef->getTexture() may
> return non-NULL even when the bitmap is volatile.

[junov, 2011-07-26 14:54:45]

http://codereview.appspot.com/4803052/diff/1/src/core/SkBitmap.cpp
File src/core/SkBitmap.cpp (right):

http://codereview.appspot.com/4803052/diff/1/src/core/SkBitmap.cpp#newcode427
src/core/SkBitmap.cpp:427: return fPixelRef && !isVolatile() ?
fPixelRef->getTexture() : NULL;
On 2011/07/26 14:10:26, reed1 wrote:
> If the caller of
> getTexture() cares about that hint, they can check it themselves. 

The caller is SkAutoCachedTexture::set, which is barely a better place for the
check.  The fundamental problem here is that SkBitmap::getTexture() assumes that
fPixelRef->getTexture() returns a texture that was spawned from this bitmap,
which is not necessarily the case.  Checking isVolatile() works for now, but it
is not a clean solution, I am first to admit.  The proper long term solution is
to fix issue 332.  If this change goes in, I will add a comment to issue 332
that this check on isVolatile should be removed.  For now, this check will not
hurt existing caching performance because fPixelRef->getTexture is supposed to
return NULL for volatile textures anyways, it is just in the rare case of a
falsely shared texture reference that this condition will kick in.

[junov1, 2011-07-27 17:39:31]

Ping! New patch

[bsalomon, 2011-07-27 18:07:33]

On 2011/07/27 17:39:31, junov1 wrote:
> Ping! New patch

It LGTM but I'll defer to Mike who knows this code better. If I uncomment that
line I'll see issue 332 on the domain sample?

[reed1, 2011-07-27 18:23:46]

waiting on a repro case to see the bug.

[junov, 2011-07-27 18:33:14]

The repro case for 332?  It is like Brian wrote.  Just comment-out the
setIsVolatile line in SkGpuDevice::drawBitmap, then the Texture Domain
sample will fail to render correctly.

Once 332 is fixed, we can remove that line from the code.


On Wed, Jul 27, 2011 at 2:23 PM,  <reed@google.com> wrote:
> waiting on a repro case to see the bug.
>
> http://codereview.appspot.com/4803052/
>

[reed1, 2011-07-28 13:34:46]

Lets not checking anything related to volatile or the workaround, now that we're
on a better track in 332 of the real bug and possible ways to fix it.

[junov1, 2011-07-28 17:40:17]

On 2011/07/28 13:34:46, reed1 wrote:
> Lets not checking anything related to volatile or the workaround, now that
we're
> on a better track in 332 of the real bug and possible ways to fix it.

Done. Just to be clear: the latest patch (3) does not address 332 at all.  It
fixes the case where the IgnoreTranform flag is off.  Also SampleTextureDomain
was modified to demonstrate the bug with IgnoreTransform and at the same time it
demonstrate a different manifestation of issue 332, through the use of a
MaskFilter

[reed1, 2011-07-28 17:46:29]

This is simpler thanks.

Are we saying that drawPath doesn't support maskfilters, but drawRect does? That
sounds like a bug worth fixing.

http://codereview.appspot.com/4803052/diff/8002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (left):

http://codereview.appspot.com/4803052/diff/8002/src/gpu/SkGpuDevice.cpp#oldco...
src/gpu/SkGpuDevice.cpp:1266: }
Tiny optimization:

SkMatrix drawMatrix;
drawMatrix.setConcat(*draw.fMatrix, m);

[junov1, 2011-07-28 18:03:38]

On 2011/07/28 17:46:29, reed1 wrote:
> Are we saying that drawPath doesn't support maskfilters, but drawRect does?
That
> sounds like a bug worth fixing.

Not at all. drawRect ends up using drawPath if there is a mask filter, so
drawPAth works fine.  The problem with the previous state of the code was that
the transform was being applied directly to the primitive rather than using the
global transform matrix.  That was preventing the transform from affecting the
blur radius when the ignore transform flags was turned off.

By using the global transform, there is no reason to fork between the drawRect
and drawPath methods.

[reed1, 2011-07-28 18:05:37]

On 2011/07/28 18:03:38, junov1 wrote:
> On 2011/07/28 17:46:29, reed1 wrote:
> > Are we saying that drawPath doesn't support maskfilters, but drawRect does?
> That
> > sounds like a bug worth fixing.
> 
> Not at all. drawRect ends up using drawPath if there is a mask filter, so
> drawPAth works fine.  The problem with the previous state of the code was that
> the transform was being applied directly to the primitive rather than using
the
> global transform matrix.  That was preventing the transform from affecting the
> blur radius when the ignore transform flags was turned off.
> 
> By using the global transform, there is no reason to fork between the drawRect
> and drawPath methods.

OK, then perhaps we can make the comment in the code clearer. That was why I
asked the question.

[junov1, 2011-07-28 19:45:43]

Done.

[reed1, 2011-07-28 19:47:53]

LGTM