Fold alpha to the draw in savelayer-draw-restore patterns with non-opaque draw

Fold alpha to the draw in savelayer-draw-restore patterns with non-opaque draw

Fold alpha of a save layer call to the subsequent draw call paint, even when
the draw call paint color is already non-opaque.

Comparing the difference of the unoptimized and the optimized call pattern
with all (layer alpha, draw alpha) combinations, this produces
off-by-one pixels ~50% of the time.

Reduces layers of current desk_carsvg.skp (recorded with cross-process
picture image filters allowed) from 122 to 115.

BUG=skia:3119
Committed: https://skia.googlesource.com/skia/+/678c1b019ac98bc7d94841132c8105a77490bc64

[Kimmo Kinnunen, 2015-01-13 08:31:27]

kkinnunen@nvidia.com changed reviewers:
+ robertphillips@google.com

[Kimmo Kinnunen, 2015-01-13 08:31:28]

Robert, you mentioned in the bug:

> 3) SaveLayer|DrawPath|Restore where both paints have an alpha
> Case 3 could be pretty easily optimized away with some care.

Can you expand on the carefulness, do you see anything missing here?

What about the pixel differences, is it a blocker? I guess it's because of
different rounding wrt real double blending and alpha folding.

[mtklein, 2015-01-13 18:48:20]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2015-01-13 18:48:20]

Thanks for doing this TODO!  I have only nits.

https://codereview.chromium.org/840483005/diff/20001/gm/recordopts.cpp
File gm/recordopts.cpp (right):

https://codereview.chromium.org/840483005/diff/20001/gm/recordopts.cpp#newcode62
gm/recordopts.cpp:62: fPicture->playback(canvas);
Seems like we might also want a reference image that's drawn directly, not
through picture?

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

https://codereview.chromium.org/840483005/diff/20001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:120: SkFloatToIntRound(SkColorGetA(drawColor) *
Let's use SkMulDiv255Round?

https://codereview.chromium.org/840483005/diff/20001/tests/RecordOptsTest.cpp
File tests/RecordOptsTest.cpp (right):

https://codereview.chromium.org/840483005/diff/20001/tests/RecordOptsTest.cpp...
tests/RecordOptsTest.cpp:128: badDrawPaint.setColor( 0x0F020202);  // Not
opaque.
Let's rename goodDrawPaint -> opaqueDrawPaint, badDrawPaint ->
transparentDrawPaint?  badDrawPaint isn't bad anymore! :)

[mtklein, 2015-01-13 18:50:58]

https://codereview.chromium.org/840483005/diff/20001/gm/recordopts.cpp
File gm/recordopts.cpp (right):

https://codereview.chromium.org/840483005/diff/20001/gm/recordopts.cpp#newcode62
gm/recordopts.cpp:62: fPicture->playback(canvas);
On 2015/01/13 18:48:20, mtklein wrote:
> Seems like we might also want a reference image that's drawn directly, not
> through picture?

Or even better, we can draw a diff between the non-picture and picture versions?

[Kimmo Kinnunen, 2015-01-14 13:07:51]

https://codereview.chromium.org/840483005/diff/20001/gm/recordopts.cpp
File gm/recordopts.cpp (right):

https://codereview.chromium.org/840483005/diff/20001/gm/recordopts.cpp#newcode62
gm/recordopts.cpp:62: fPicture->playback(canvas);
On 2015/01/13 18:50:58, mtklein wrote:
> On 2015/01/13 18:48:20, mtklein wrote:
> > Seems like we might also want a reference image that's drawn directly, not
> > through picture?
> 
> Or even better, we can draw a diff between the non-picture and picture
versions?

Done.

https://codereview.chromium.org/840483005/diff/20001/gm/recordopts.cpp#newcode62
gm/recordopts.cpp:62: fPicture->playback(canvas);
On 2015/01/13 18:48:20, mtklein wrote:
> Seems like we might also want a reference image that's drawn directly, not
> through picture?

Done.

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

https://codereview.chromium.org/840483005/diff/20001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:120: SkFloatToIntRound(SkColorGetA(drawColor) *
On 2015/01/13 18:48:20, mtklein wrote:
> Let's use SkMulDiv255Round?

Done.

https://codereview.chromium.org/840483005/diff/20001/tests/RecordOptsTest.cpp
File tests/RecordOptsTest.cpp (right):

https://codereview.chromium.org/840483005/diff/20001/tests/RecordOptsTest.cpp...
tests/RecordOptsTest.cpp:128: badDrawPaint.setColor( 0x0F020202);  // Not
opaque.
On 2015/01/13 18:48:20, mtklein wrote:
> Let's rename goodDrawPaint -> opaqueDrawPaint, badDrawPaint ->
> transparentDrawPaint?  badDrawPaint isn't bad anymore! :)

Done.

[mtklein, 2015-01-14 13:28:31]

https://codereview.chromium.org/840483005/diff/60001/gm/recordopts.cpp
File gm/recordopts.cpp (right):

https://codereview.chromium.org/840483005/diff/60001/gm/recordopts.cpp#newcode21
gm/recordopts.cpp:21: // can not save layer for every call. This takes too much
time for a gm test. The drawing
... can not save the full layer ...  ?

https://codereview.chromium.org/840483005/diff/60001/gm/recordopts.cpp#newcod...
gm/recordopts.cpp:267: // execute so many save layers in time. Thus our
difference image is correct only for the
If we're only going to get a useful diff with the 8888 backend, let's simplify
this.  As cool as the HighlighSmallDifferencesXfermode is, it's a rather large
maintenance burden for such a one-off test, particularly if it doesn't give us
useful results.  Here's what I'd suggest

onDraw(canvas) {
  SkBitmap direct, optimized, diff;
  ... setup bitmaps ...

  SkCanvas dcanvas(direct), ocanvas(optimized);
  draw_all_save_layer_draw_restore_combinations(&dcanvas, false);

  SkPictureRecorder rec;
  draw_all_save_layer_draw_restore_combinations(rec.beginRecording(...), true);
  SkAutoTDelete<SkPicture> pic(rec.endRecording());
  pic->playback(&ocanvas);

  // Because the GPU backend can't handle so many save layers, we just do
everything on the CPU and draw
  // the final diff result as a bitmap.
  diff = apply_highlightsmalldifferences_xfermode_logic(direct, optimized);
  canvas->drawBitmap(diff);
}

https://codereview.chromium.org/840483005/diff/60001/src/core/SkRecordOpts.cpp
File src/core/SkRecordOpts.cpp (right):

https://codereview.chromium.org/840483005/diff/60001/src/core/SkRecordOpts.cp...
src/core/SkRecordOpts.cpp:119: drawPaint->setColor(SkColorSetA(drawColor,
Just FYI, SkPaint has a setAlpha() helper.

[Kimmo Kinnunen, 2015-01-14 14:07:20]

https://codereview.chromium.org/840483005/diff/60001/gm/recordopts.cpp
File gm/recordopts.cpp (right):

https://codereview.chromium.org/840483005/diff/60001/gm/recordopts.cpp#newcod...
gm/recordopts.cpp:267: // execute so many save layers in time. Thus our
difference image is correct only for the
On 2015/01/14 13:28:30, mtklein wrote:
> If we're only going to get a useful diff with the 8888 backend, let's simplify
> this.

Right. Well, with my gpu and the current code it gives the same result.
The original idea was that the file wouldn't be for just this one test, rather
also the future tests. Thus the diff might be useful in scenarios where gpu
would be slow (-->reason to optimize) but the testcase not so heavy (-->direct
xfermode useful).

But I can also spend time some more time rewriting this again to be simpler, of
course..

[mtklein, 2015-01-14 14:53:10]

LGTM

Greg mentioned to me just now he was rejiggering how GPU transfermodes work, so
we might want to get his eyes on this too.

https://codereview.chromium.org/840483005/diff/60001/gm/recordopts.cpp
File gm/recordopts.cpp (right):

https://codereview.chromium.org/840483005/diff/60001/gm/recordopts.cpp#newcod...
gm/recordopts.cpp:267: // execute so many save layers in time. Thus our
difference image is correct only for the
Hmmm.  I think I misunderstood the downside of the setup.  This seems fine. 
Let's land this, see how close the 8888 backend and all the other GPUs are, and
then decide to either baseline this as-is or rewrite it to force 8888.

[egdaniel, 2015-01-14 15:00:37]

egdaniel@google.com changed reviewers:
+ egdaniel@google.com

[egdaniel, 2015-01-14 15:00:38]

The main question I have here on the gpu side is will this Xfer effect be
blending with Dst or will it actually be using a background texture to blend
with? If it is the former then you should make an GrXferProcessor instead of a
GrFragmentProcessor. If the later then the current version is fine. Or if it
could be either you will want both.

The reason for this is I am in the process of landing changes that will use GrXP
if and only if there is no background texture and GrFP otherwise.

[mtklein, 2015-01-14 15:12:52]

Hmm.  Let me un-lgtm this.  Why don't we just readPixels() back after we draw,
do the diff, then overdraw the diff back?  That way the diffing code will be the
same across all platforms and configs, and we don't have to hassle with a new
xfermode.

[mtklein, 2015-01-14 15:13:05]

On 2015/01/14 15:12:52, mtklein wrote:
> Hmm.  Let me un-lgtm this.  Why don't we just readPixels() back after we draw,
> do the diff, then overdraw the diff back?  That way the diffing code will be
the
> same across all platforms and configs, and we don't have to hassle with a new
> xfermode.

Err, not lgtm?

[egdaniel, 2015-01-14 15:37:28]

On 2015/01/14 15:13:05, mtklein wrote:
> On 2015/01/14 15:12:52, mtklein wrote:
> > Hmm.  Let me un-lgtm this.  Why don't we just readPixels() back after we
draw,
> > do the diff, then overdraw the diff back?  That way the diffing code will be
> the
> > same across all platforms and configs, and we don't have to hassle with a
new
> > xfermode.
> 
> Err, not lgtm?

I second this idea. This way whatever diff visualizations we see across configs
comes from the code we are actually testing and not the diffing tool being used.

[Kimmo Kinnunen, 2015-01-15 08:38:04]

On 2015/01/14 15:12:52, mtklein wrote:
> Hmm.  Let me un-lgtm this.  Why don't we just readPixels() back after we draw,
> do the diff, then overdraw the diff back? 
Done.

[mtklein, 2015-01-15 16:08:27]

Let's land this today without yet landing a new GM.  It's starting to bother me
that we're piping through pictures and doing diffing inside a GM, when that's
something DM/skiagold ought to be handling.  And the diff we're making now is
admittedly a bit weird... it's the optimized picture into the real canvas,
versus a reference software canvas.

I've been playing around with this and noticed a few things:
  - I think we can just remove the check for NULL bounds in the optimization,
and then in the future we can always feed the saveLayer bounds.
  - Even with bounds in the saveLayer, the GPU backends take forever to run this
GM.
I guess the reference software canvas is a concession to how slow it is to draw
the full drawing pattern into a GPU canvas?

Anyway, for now, I think we can just track this change's affect with the
existing GMs and SKPs.  If this really does affect desk_carsvg.skp, we'll see
the effect there.

[Kimmo Kinnunen, 2015-01-16 09:02:44]

On 2015/01/15 16:08:27, mtklein wrote:
> Let's land this today without yet landing a new GM.

Done.

>  It's starting to bother me
> that we're piping through pictures and doing diffing inside a GM, when that's
> something DM/skiagold ought to be handling.  And the diff we're making now is
> admittedly a bit weird... it's the optimized picture into the real canvas,
> versus a reference software canvas.

Right.. Bother.. the rationale given above sounds a bit like the rationale I had
with the first version of the test. Like, doing the simplest thing that's useful
and leaving the diffing to the Skia infrastructure.. before I spent a few
minutes on few days to hone the test further...
 
> I've been playing around with this and noticed a few things:
>   - I think we can just remove the check for NULL bounds in the optimization,
> and then in the future we can always feed the saveLayer bounds.
>   - Even with bounds in the saveLayer, the GPU backends take forever to run
this
> GM.
> I guess the reference software canvas is a concession to how slow it is to
draw
> the full drawing pattern into a GPU canvas?

Yes, provided I understood what you mean. Using software canvas to create the
reference picture is an admission of defeat in trying to use the unoptimized
call sequence with the GPU. The GPU backend can takes too much time drawing full
save layers, 1x1 bounded save layers or 1x1 clips constraining the save layers.
The SW backend takes too much time in drawing full save layers. These were
partly noted as comments in the testcase. The need for the software canvas to
create the reference picture can be seen as a supporting indicator for need of
such a fix -- though the testcase is contrived.

[mtklein, 2015-01-16 12:54:52]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-01-16 12:54:52]

lgtm

[commit-bot: I haz the power, 2015-01-16 12:55:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/840483005/100001

[commit-bot: I haz the power, 2015-01-16 13:04:39]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/678c1b019ac98bc7d94841132c8105a77490bc64