Optimize alpha software PictureDrawQuads.

Optimize alpha software PictureDrawQuads.

Instead of mallocing another bitmap, edit the SkPicture's draw commands
at playback time to use the PictureDrawQuad's opacity.  This
results in similar performance as if Blink had baked in the opacity in
the first place.  However, it's not correct in some cases: 
filed http://crbug.com/280374 to track.

Also turn off setFilterBitmap for software PictureDrawQuads as this is
very costly in practice.

BUG=275048
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220257

[aelias_OOO_until_Jul13, 2013-08-24 07:50:49]

Hi, I found this trick to optimize non-opaque draws without needing any invasive
Blink changes.  I went ahead and added it in the Ganesh case as well (haven't
tested that yet, but there's no reason why it shouldn't work there too).  It
turns out we don't need to worry about allocating backing bitmaps at this level
because Blink will take care of putting saveLayer calls in the SkPicture
whenever that's required.

[jamesr, 2013-08-26 18:09:08]

https://codereview.chromium.org/22867046/diff/4001/skia/ext/alpha_proxy_canvas.h
File skia/ext/alpha_proxy_canvas.h (right):

https://codereview.chromium.org/22867046/diff/4001/skia/ext/alpha_proxy_canva...
skia/ext/alpha_proxy_canvas.h:13: class SK_API AlphaProxyCanvas : public
SkProxyCanvas {
why is this in skia/ext/?  If it's logically part of skia, shouldn't it be in
the skia repository?  If it's just used by the compositor, shouldn't it be in
cc/?

[aelias_OOO_until_Jul13, 2013-08-26 18:30:34]

On 2013/08/26 18:09:08, jamesr wrote:
>
https://codereview.chromium.org/22867046/diff/4001/skia/ext/alpha_proxy_canvas.h
> File skia/ext/alpha_proxy_canvas.h (right):
> 
>
https://codereview.chromium.org/22867046/diff/4001/skia/ext/alpha_proxy_canva...
> skia/ext/alpha_proxy_canvas.h:13: class SK_API AlphaProxyCanvas : public
> SkProxyCanvas {
> why is this in skia/ext/?  If it's logically part of skia, shouldn't it be in
> the skia repository?  If it's just used by the compositor, shouldn't it be in
> cc/?

All of the other SkCanvas derivatives are in skia/ext, for example
AnalysisCanvas is also in skia/ext but only used by CC.  I don't think it should
be in Skia repository since it's for a Chrome-specific use case and we may want
to iterate on it alongside further CC changes.  I'm not sure what would be a
better place in cc/, cc/output/ or cc/base/?

[enne (OOO), 2013-08-26 19:01:31]

skia/ext seems like a reasonable place for this code to live.  It's not really a
compositor-specific piece of code; it's really an extension to Skia that just
happens to be useful for the compositor.

[enne (OOO), 2013-08-27 00:25:52]

This is the "fast but not correct" approach, right? We push more cases to blend
in not quite the right order but decrease the amount of copies we need to get
something close enough.

https://codereview.chromium.org/22867046/diff/4001/skia/ext/alpha_proxy_canva...
File skia/ext/alpha_proxy_canvas.cc (right):

https://codereview.chromium.org/22867046/diff/4001/skia/ext/alpha_proxy_canva...
skia/ext/alpha_proxy_canvas.cc:146:
alpha_paint.setXfermodeMode(SkXfermode::kSrcOver_Mode);
I am most skeptical about this particular line.  How do you support other
xfermodes with this approach?

[aelias_OOO_until_Jul13, 2013-08-27 07:02:27]

Yes, it's not correct.  I took a closer look at Blink's logic today and it's
(unfortunately?) efficient enough that we'll fall into many of the incorrect
cases in practice after all.  I tried to run the layout tests to see which ones
break but just turning on impl-side-painting still breaks half of them, so I
have no good baseline :(.

As I just described on the bug in #21, I'm thinking we'll need to do
analysis/fallback-to-tiling in the future, but for now I guess this will do.

https://codereview.chromium.org/22867046/diff/4001/skia/ext/alpha_proxy_canva...
File skia/ext/alpha_proxy_canvas.cc (right):

https://codereview.chromium.org/22867046/diff/4001/skia/ext/alpha_proxy_canva...
skia/ext/alpha_proxy_canvas.cc:146:
alpha_paint.setXfermodeMode(SkXfermode::kSrcOver_Mode);
On 2013/08/27 00:25:52, enne wrote:
> I am most skeptical about this particular line.  How do you support other
> xfermodes with this approach?

We can't.  I added a DCHECK the mode is already Src or SrcOver.

[danakj, 2013-08-27 14:45:06]

On Tue, Aug 27, 2013 at 3:02 AM, <aelias@chromium.org> wrote:

> Yes, it's not correct.  I took a closer look at Blink's logic today and
> it's
> (unfortunately?) efficient enough that we'll fall into many of the
> incorrect
> cases in practice after all.  I tried to run the layout tests to see which
> ones
> break but just turning on impl-side-painting still breaks half of them, so
> I
> have no good baseline :(.
>

Maybe we need a virtual suite that runs with impl painting so that we can
have some baselines.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2013-08-27 18:53:29]

I think you could test this with layout tests by handing this AlphaProxyCanvas
to ContentLayerUpdater::PaintContents in
BitmapContentLayerUpdater::PrepareToUpdate.

https://codereview.chromium.org/22867046/diff/13001/skia/ext/alpha_proxy_canv...
File skia/ext/alpha_proxy_canvas.cc (right):

https://codereview.chromium.org/22867046/diff/13001/skia/ext/alpha_proxy_canv...
skia/ext/alpha_proxy_canvas.cc:21: SkPaint new_paint = AlphaPaint(paint);
Shouldn't new_paint only be AlphaPaint for the top level of saveLayer calls? If
I did:

saveLayer()
saveLayer()
saveLayer()
drawRect
restoreLayer()
restoreLayer()
restoreLayer()

...ideally the alpha should only be applied once, right?

https://codereview.chromium.org/22867046/diff/13001/skia/ext/alpha_proxy_canv...
skia/ext/alpha_proxy_canvas.cc:150: SkXfermode::IsMode(paint->getXfermode(),
SkXfermode::kSrcOver_Mode));
Unless I'm missing something, I am not sure that a DCHECK is the right answer. 
Couldn't we have recorded anything from Blink in here?  A quick grep points out
that there are plenty of other modes being set on paints that could be recorded.

[aelias_OOO_until_Jul13, 2013-08-28 02:38:40]

PTAL.  I got some pushback to adding more virtuals in Skia, and learned that the
SkDrawFilter feature of skia exists to solve exactly this use case, so I
switched this patch to use that instead.  I also added a test (for the simple
one-non-opaque case that this supports correctly).

https://codereview.chromium.org/22867046/diff/13001/skia/ext/alpha_proxy_canv...
File skia/ext/alpha_proxy_canvas.cc (right):

https://codereview.chromium.org/22867046/diff/13001/skia/ext/alpha_proxy_canv...
skia/ext/alpha_proxy_canvas.cc:21: SkPaint new_paint = AlphaPaint(paint);
On 2013/08/27 18:53:30, enne wrote:
> Shouldn't new_paint only be AlphaPaint for the top level of saveLayer calls?
If
> I did:
> 
> saveLayer()
> saveLayer()
> saveLayer()
> drawRect
> restoreLayer()
> restoreLayer()
> restoreLayer()
> 
> ...ideally the alpha should only be applied once, right?

Correct, and with the SkDrawFilter approach, this can't be fixed without a new
Skia parameter.  But I've checked that classic WebView should have the exact
same bug, and given that we're already busted, I figure we should just go for
100% bugwards compatibility, rather than web-standards-compliance for now.  (The
absence of bugs might break apps that rely on the old incorrect color so it's
"safer" to be buggier.)

https://codereview.chromium.org/22867046/diff/13001/skia/ext/alpha_proxy_canv...
skia/ext/alpha_proxy_canvas.cc:150: SkXfermode::IsMode(paint->getXfermode(),
SkXfermode::kSrcOver_Mode));
On 2013/08/27 18:53:30, enne wrote:
> Unless I'm missing something, I am not sure that a DCHECK is the right answer.

> Couldn't we have recorded anything from Blink in here?  A quick grep points
out
> that there are plenty of other modes being set on paints that could be
recorded.

With further experimentation, it was unnecessary to set SrcOver_Mode after all,
so I removed the override and the DCHECK.

[joth, 2013-08-28 03:29:12]

thanks for handling this aelias.
I'm all for increasing correctness in future if it can be done without such
major perf hit, but in the meantime  matching the status quo is good for reasons
you say.

minor nit...

https://codereview.chromium.org/22867046/diff/23001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/22867046/diff/23001/cc/output/software_render...
cc/output/software_renderer.cc:314: // http://crbug.com/280374 - Applying
opacity to each individual draw call is
Is this a TODO, or an explanation of what is already done? (I think the former,
but at first it reads like the latter)

[tomhudson, 2013-08-28 14:55:18]

https://codereview.chromium.org/22867046/diff/29001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/22867046/diff/29001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1672: if (quad->opacity() != 1.0f)
Do you want to debug assert that sk_canvas_->getDrawFilter() == NULL?

https://codereview.chromium.org/22867046/diff/29001/cc/output/renderer_pixelt...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/22867046/diff/29001/cc/output/renderer_pixelt...
cc/output/renderer_pixeltest.cc:1397:
TYPED_TEST(RendererPixelTestWithSkiaGPUBackend, PictureDrawQuadOpacity) {
Why is the name "WithSkiaGPUBackend" if that's a variable?

https://codereview.chromium.org/22867046/diff/29001/cc/output/renderer_pixelt...
cc/output/renderer_pixeltest.cc:1422:
green_quad->SetNew(green_shared_state.get(),
boilerplatey, but I guess there are 6-8 instances of this call with *not quite*
the same arg patterns. :(

https://codereview.chromium.org/22867046/diff/29001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/22867046/diff/29001/cc/output/software_render...
cc/output/software_renderer.cc:318: true));
Again, consider debug assert if current filter is non-NULL?

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
File skia/ext/opacity_draw_filter.cc (right):

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
skia/ext/opacity_draw_filter.cc:17: paint->setAlpha(SkScalarRound(opacity_ *
255));
(Maybe arguing against my comment in the header)

Should we be setting alpha, or multiplying alpha?

I want to say that if we're setting you should omit the if for clarity, but
setting alpha is just heavyweight enough that it might be a performance loss, at
least if Android's ChromeView ever links with system Skia.

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
File skia/ext/opacity_draw_filter.h (right):

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
skia/ext/opacity_draw_filter.h:27: float opacity_;
Why not store SkScalarRound(opacity_*255), instead of recomputing it on every
draw?

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
skia/ext/opacity_draw_filter.h:28: float disable_image_filtering_;
bool?!

[aelias_OOO_until_Jul13, 2013-08-28 18:50:35]

https://codereview.chromium.org/22867046/diff/29001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/22867046/diff/29001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1672: if (quad->opacity() != 1.0f)
On 2013/08/28 14:55:18, tomhudson wrote:
> Do you want to debug assert that sk_canvas_->getDrawFilter() == NULL?

Done.

https://codereview.chromium.org/22867046/diff/29001/cc/output/renderer_pixelt...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/22867046/diff/29001/cc/output/renderer_pixelt...
cc/output/renderer_pixeltest.cc:1397:
TYPED_TEST(RendererPixelTestWithSkiaGPUBackend, PictureDrawQuadOpacity) {
On 2013/08/28 14:55:18, tomhudson wrote:
> Why is the name "WithSkiaGPUBackend" if that's a variable?

This exercises all of GLRenderer, SoftwareRenderer and
GLRendererWithSkiaGPUBackend.  If "WithSkiaGPUBackend" isn't specified, then
only GLRenderer and SoftwareRenderer are exercised.  It essentially affects
which GLRenderer::DrawPictureQuad variant is used.

https://codereview.chromium.org/22867046/diff/29001/cc/output/software_render...
File cc/output/software_renderer.cc (right):

https://codereview.chromium.org/22867046/diff/29001/cc/output/software_render...
cc/output/software_renderer.cc:318: true));
On 2013/08/28 14:55:18, tomhudson wrote:
> Again, consider debug assert if current filter is non-NULL?

Done.

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
File skia/ext/opacity_draw_filter.cc (right):

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
skia/ext/opacity_draw_filter.cc:17: paint->setAlpha(SkScalarRound(opacity_ *
255));
On 2013/08/28 14:55:18, tomhudson wrote:
> (Maybe arguing against my comment in the header)
> 
> Should we be setting alpha, or multiplying alpha?
> 
> I want to say that if we're setting you should omit the if for clarity, but
> setting alpha is just heavyweight enough that it might be a performance loss,
at
> least if Android's ChromeView ever links with system Skia.

There's an argument for multiplying, but for now I'm setting alpha because
that's what the classic WebView implementation did.  (You can see the code at
http://goo.gl/ZtQjjU ).

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
File skia/ext/opacity_draw_filter.h (right):

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
skia/ext/opacity_draw_filter.h:27: float opacity_;
On 2013/08/28 14:55:18, tomhudson wrote:
> Why not store SkScalarRound(opacity_*255), instead of recomputing it on every
> draw?

Done.

https://codereview.chromium.org/22867046/diff/29001/skia/ext/opacity_draw_fil...
skia/ext/opacity_draw_filter.h:28: float disable_image_filtering_;
On 2013/08/28 14:55:18, tomhudson wrote:
> bool?!

Oops, thanks for catching that.

[enne (OOO), 2013-08-28 20:08:42]

https://codereview.chromium.org/22867046/diff/35001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/22867046/diff/35001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1640: void
GLRenderer::DrawPictureQuadDirectToBackbuffer(
Can you not change this path?  I'm not sure I want to saddle Ganesh usage with
bug compatibility with WebView.

https://codereview.chromium.org/22867046/diff/35001/skia/ext/opacity_draw_fil...
File skia/ext/opacity_draw_filter.h (right):

https://codereview.chromium.org/22867046/diff/35001/skia/ext/opacity_draw_fil...
skia/ext/opacity_draw_filter.h:17: // when there are no overlapping draw calls
(as this doesn't allocate a bitmap
It's also only correct when draw calls are all opaque (as this clobbers the
alpha) and when there aren't nested SaveLayer calls (I think?)

[aelias_OOO_until_Jul13, 2013-08-28 20:22:59]

https://codereview.chromium.org/22867046/diff/35001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/22867046/diff/35001/cc/output/gl_renderer.cc#...
cc/output/gl_renderer.cc:1640: void
GLRenderer::DrawPictureQuadDirectToBackbuffer(
On 2013/08/28 20:08:42, enne wrote:
> Can you not change this path?  I'm not sure I want to saddle Ganesh usage with
> bug compatibility with WebView.

OK, reverted.  The optimization could be eventually usable by Ganesh if tiling
is carefully disabled only when the requirements hold true, but we can cross
that bridge when we get to it.

https://codereview.chromium.org/22867046/diff/35001/skia/ext/opacity_draw_fil...
File skia/ext/opacity_draw_filter.h (right):

https://codereview.chromium.org/22867046/diff/35001/skia/ext/opacity_draw_fil...
skia/ext/opacity_draw_filter.h:17: // when there are no overlapping draw calls
(as this doesn't allocate a bitmap
On 2013/08/28 20:08:42, enne wrote:
> It's also only correct when draw calls are all opaque (as this clobbers the
> alpha) and when there aren't nested SaveLayer calls (I think?)

I made the comment clearer.

[enne (OOO), 2013-08-28 20:29:47]

lgtm, thanks!

[aelias_OOO_until_Jul13, 2013-08-28 20:57:39]

OK, I just need OWNERS from Tom for the skia/ change.

[tomhudson, 2013-08-29 08:14:09]

On 2013/08/28 20:57:39, aelias wrote:
> OK, I just need OWNERS from Tom for the skia/ change.

Sorry about that. LGTM

[commit-bot: I haz the power, 2013-08-29 08:18:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/22867046/44001

[commit-bot: I haz the power, 2013-08-29 11:19:21]

Change committed as 220257