Let blitters be notified when they're done being used

Let blitters be notified when they're done being used

Committed: https://skia.googlesource.com/skia/+/75f88512a1b74ebb1bbb4f0046e43f1a5a234320

[reed1, 2014-07-18 19:59:46]

Seems hard to guarantee that we always call this (and don't ever call it twice).
Is there a more narrow proposal to address your need? (what is your need?)

[krajcevski, 2014-07-18 21:32:47]

On 2014/07/18 19:59:46, reed1 wrote:
> Seems hard to guarantee that we always call this (and don't ever call it
twice).
> Is there a more narrow proposal to address your need? (what is your need?)

The SkScan::AntiFillPath function finishes executing before the blitter has a
chance to call its destructor. This causes the problem where the SuperBlitter
frees the buffered scan lines before the underlying blitter (my compression
blitter) has a chance to use them. This happens whenever the blit region is not
a multiple of the requested number of scan lines and, in my case, two scanlines
get blit at the end of the scan conversion but not enough to trigger a full
flush (which happens every four).

[krajcevski, 2014-07-18 21:34:01]

On 2014/07/18 21:32:47, krajcevski wrote:
> On 2014/07/18 19:59:46, reed1 wrote:
> > Seems hard to guarantee that we always call this (and don't ever call it
> twice).
> > Is there a more narrow proposal to address your need? (what is your need?)
> 
> The SkScan::AntiFillPath function finishes executing before the blitter has a
> chance to call its destructor. This causes the problem where the SuperBlitter
> frees the buffered scan lines before the underlying blitter (my compression
> blitter) has a chance to use them. This happens whenever the blit region is
not
> a multiple of the requested number of scan lines and, in my case, two
scanlines
> get blit at the end of the scan conversion but not enough to trigger a full
> flush (which happens every four).

I think that the blitter that needs to override this function should be
resilient to having it called twice, but the guarantee that it's always called
is probably somewhat hard to enforce.

[reed1, 2014-07-21 13:15:01]

I'm fine with the idea, just want to be sure we don't make our code more
brittle.

Can you add a debugging check in the base-class to ensure that we always call
it?

[reed1, 2014-07-21 13:16:10]

On 2014/07/18 21:32:47, krajcevski wrote:
> On 2014/07/18 19:59:46, reed1 wrote:
> > Seems hard to guarantee that we always call this (and don't ever call it
> twice).
> > Is there a more narrow proposal to address your need? (what is your need?)
> 
> The SkScan::AntiFillPath function finishes executing before the blitter has a
> chance to call its destructor. This causes the problem where the SuperBlitter
> frees the buffered scan lines before the underlying blitter (my compression
> blitter) has a chance to use them. This happens whenever the blit region is
not
> a multiple of the requested number of scan lines and, in my case, two
scanlines
> get blit at the end of the scan conversion but not enough to trigger a full
> flush (which happens every four).

Crazy idea -- what if we ask the blitter to perform the allocation (and own the
memory)? That way it could handle the 4x multiplier, and could always safely
cleanup from its destructor?

[krajcevski, 2014-07-21 14:46:22]

Sure, did you mean something like this?

[reed1, 2014-07-21 14:51:13]

lgtm -- robert, does this look good?

https://codereview.chromium.org/399593007/diff/20001/src/core/SkBlitter.cpp
File src/core/SkBlitter.cpp (right):

https://codereview.chromium.org/399593007/diff/20001/src/core/SkBlitter.cpp#n...
src/core/SkBlitter.cpp:202: if (NULL != fBlitMemory) {
Should this happen (we get called twice)?

[krajcevski, 2014-07-21 15:04:41]

https://codereview.chromium.org/399593007/diff/20001/src/core/SkBlitter.cpp
File src/core/SkBlitter.cpp (right):

https://codereview.chromium.org/399593007/diff/20001/src/core/SkBlitter.cpp#n...
src/core/SkBlitter.cpp:202: if (NULL != fBlitMemory) {
On 2014/07/21 14:51:13, reed1 wrote:
> Should this happen (we get called twice)?

Added an assert to catch if it is, but we shouldn't fail (crash) if we can avoid
it.

[robertphillips, 2014-07-21 15:14:59]

https://codereview.chromium.org/399593007/diff/40001/src/core/SkBlitter.h
File src/core/SkBlitter.h (right):

https://codereview.chromium.org/399593007/diff/40001/src/core/SkBlitter.h#new...
src/core/SkBlitter.h:111: 
What do you think about using SkAutoMalloc for this?

[krajcevski, 2014-07-21 15:47:41]

https://codereview.chromium.org/399593007/diff/40001/src/core/SkBlitter.h
File src/core/SkBlitter.h (right):

https://codereview.chromium.org/399593007/diff/40001/src/core/SkBlitter.h#new...
src/core/SkBlitter.h:111: 
On 2014/07/21 15:14:59, robertphillips wrote:
> What do you think about using SkAutoMalloc for this?

Done.

[robertphillips, 2014-07-21 16:07:19]

lgtm

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

https://codereview.chromium.org/399593007/diff/60001/src/core/SkBlitter.h#new...
src/core/SkBlitter.h:27: public:
I don't think you need the 'fBlitMemory(0)' anymore.

[krajcevski, 2014-07-21 16:42:09]

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

https://codereview.chromium.org/399593007/diff/60001/src/core/SkBlitter.h#new...
src/core/SkBlitter.h:27: public:
On 2014/07/21 16:07:19, robertphillips wrote:
> I don't think you need the 'fBlitMemory(0)' anymore.

Done.

I stopped reading at the explicit declaration on the SkAutoMalloc constructor.
:P

[krajcevski, 2014-07-21 16:42:12]

The CQ bit was checked by krajcevski@google.com

[commit-bot: I haz the power, 2014-07-21 16:42:52]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/krajcevski@google.com/399593007/80001

[commit-bot: I haz the power, 2014-07-21 16:54:27]

Change committed as 75f88512a1b74ebb1bbb4f0046e43f1a5a234320