Fix end-of-pattern matching for Skia recording optimization.

Fix end-of-pattern matching for Skia recording optimization.

The recorder optimizer's pattern matcher was accepting command sequences
when it shouldn't have.

In the submitted case, and the pattern matcher was looking for:
	saveLayer, drawBitmap, restore
and in the rendering for the submitted case, the sequence of commands
were:
	saveLayer, drawBitmap, drawBitmap, restore

This sequence was improperly accepted by the matcher, and the optimizer
reduced the sequence to:
	drawBitmap, drawBitmap
where the opacity from the saveLayer paint argument was applied
to the first drawBitmap only.

The user-visible effect in Chrome was a flashing effect on an image
caused by incorrect (too-high) opacity.

The patch adds a Skia test to check for pixel colour values in
a similarly structured recording.  All other Skia tests pass.
Blink layout tests also pass with this change.

BUG=chromium:344987
Committed: https://skia.googlesource.com/skia/+/3f22e8c44a16d93377c0a3881f936e692b5b3320

[mtklein, 2014-07-30 18:47:26]

This looks right to me.  Some nits on the test only.  Thank you for fixing this.

Don't pay much attention to that Windows8 failure.  Seems you hit the one flaky
bot lottery.  That particular cluster of failures means this is likely some
bigger problem, not you. http://code.google.com/p/skia/issues/detail?id=2782

I've started a couple other trybots and tested this locally.  I think it's good
to land.

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1647: SkPicture* picture = recorder.endRecording();
As written this leaks.  Best to clean it up!

SkAutoTUnref<SkPicture> picture(recorder.endRecording());

(Don't worry about SkCanvas* canvas = ... above.  The recorder will track that
lifetime.)

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1663: REPORTER_ASSERT(reporter,
replayCanvas.readPixels(onePixelII,&blueDot,4,30,30));
This is all fine, but I think you can replace everything after
replayCanvas.flush() with

REPORTER_ASSERT(reporter, bm_replay.getColor(30, 30) == 0xFF000080);
REPORTER_ASSERT(reporter, bm_replay.getColor(55, 55) == 0xFF800000);

[robertphillips, 2014-07-30 19:03:50]

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1630: SkImageInfo onePixelII =
SkImageInfo::Make(1,1,kBGRA_8888_SkColorType,kPremul_SkAlphaType);
blueBM? - Skia doesn't use '_' in variable names.

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1632:
make_bm(&bm_blue,100,100,SkColorSetARGB(255,0,0,255),true);
redBM?

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1634:
make_bm(&bm_red,100,100,SkColorSetARGB(255,255,0,0),true);
semiTransparent?

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1650: // and check a couple of its pixels.
replayBM?

[dneto, 2014-07-30 19:26:21]

Thanks for the feedback.  I'm new to Skia.
I've just uploaded a patch addressing your comments, and which fixes lint
problems.

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp
File tests/PictureTest.cpp (right):

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1630: SkImageInfo onePixelII =
SkImageInfo::Make(1,1,kBGRA_8888_SkColorType,kPremul_SkAlphaType);
On 2014/07/30 19:03:49, robertphillips wrote:
> blueBM? - Skia doesn't use '_' in variable names.

Done.

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1632:
make_bm(&bm_blue,100,100,SkColorSetARGB(255,0,0,255),true);
On 2014/07/30 19:03:49, robertphillips wrote:
> redBM?

Done.

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1634:
make_bm(&bm_red,100,100,SkColorSetARGB(255,255,0,0),true);
On 2014/07/30 19:03:50, robertphillips wrote:
> semiTransparent?

Done.

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1647: SkPicture* picture = recorder.endRecording();
On 2014/07/30 18:47:26, mtklein wrote:
> As written this leaks.  Best to clean it up!
> 
> SkAutoTUnref<SkPicture> picture(recorder.endRecording());
> 
> (Don't worry about SkCanvas* canvas = ... above.  The recorder will track that
> lifetime.)

Done.

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1650: // and check a couple of its pixels.
On 2014/07/30 19:03:50, robertphillips wrote:
> replayBM?

Done.

https://codereview.chromium.org/430503004/diff/40001/tests/PictureTest.cpp#ne...
tests/PictureTest.cpp:1663: REPORTER_ASSERT(reporter,
replayCanvas.readPixels(onePixelII,&blueDot,4,30,30));
On 2014/07/30 18:47:26, mtklein wrote:
> This is all fine, but I think you can replace everything after
> replayCanvas.flush() with
> 
> REPORTER_ASSERT(reporter, bm_replay.getColor(30, 30) == 0xFF000080);
> REPORTER_ASSERT(reporter, bm_replay.getColor(55, 55) == 0xFF800000);

Done.

[mtklein, 2014-07-30 19:30:06]

lgtm

[mtklein, 2014-07-30 22:30:47]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-07-30 22:31:00]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/dneto@chromium.org/430503004/60001

[commit-bot: I haz the power, 2014-07-30 22:42:27]

Change committed as 3f22e8c44a16d93377c0a3881f936e692b5b3320