Fix recording of saveLayout with unusual Xfermodes.

Fix recording of saveLayout with unusual Xfermodes.

This is the root cause of a Chrome rendering bug when it tiles
layers with masks.

BUG=skia:1291, chromium:401593
Committed: https://skia.googlesource.com/skia/+/327f905d2cb0d37c302d651d8f2b17ea56368467

[dneto, 2014-09-12 18:22:12]

dneto@chromium.org changed reviewers:
+ mtklein@google.com, reed@google.com

[dneto, 2014-09-12 18:23:03]

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
File src/core/SkBBoxHierarchyRecord.cpp (right):

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
src/core/SkBBoxHierarchyRecord.cpp:41: if (!needToHandleBBox && paint) {
The obvious thing would be to commonize this Xfermode checking logic in one
place.  But I believe we're removing this version of the recording logic, so I
didn't want to overgeneralize at this moment.
This deprecated codepath is still used by Chromium.

The exact list of unsafe xfermodes was determined by the test I added.

https://codereview.chromium.org/568073004/diff/1/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/568073004/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:237: SkXfermode* xfermode = paint->getXfermode();
This codepath is not the default for Chromium, but it is the default for Skia
when its own tree.

[mtklein, 2014-09-12 19:20:08]

Most of my comments are nitty.  The code generally looks good to me.

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
File src/core/SkBBoxHierarchyRecord.cpp (right):

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
src/core/SkBBoxHierarchyRecord.cpp:41: if (!needToHandleBBox && paint) {
On 2014/09/12 18:23:03, dneto wrote:
> The obvious thing would be to commonize this Xfermode checking logic in one
> place.  But I believe we're removing this version of the recording logic, so I
> didn't want to overgeneralize at this moment.
> This deprecated codepath is still used by Chromium.
> 
> The exact list of unsafe xfermodes was determined by the test I added.

SGTM.  In fact, at ToT you should now see the new code path active.

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
src/core/SkBBoxHierarchyRecord.cpp:42: // Unusual Xfermodes require us to
process a saved layer
Can you copy (or move) this comment over to SkRecordDraw.cpp?  It'd be a shame
to lose it when we kill this code off.

https://codereview.chromium.org/568073004/diff/1/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/568073004/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:234: if (paint->getImageFilter() ||
paint->getColorFilter())
Please add {}

https://codereview.chromium.org/568073004/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:237: SkXfermode* xfermode = paint->getXfermode();
On 2014/09/12 18:23:03, dneto wrote:
> This codepath is not the default for Chromium, but it is the default for Skia
> when its own tree.

Sync up!  :P

https://codereview.chromium.org/568073004/diff/1/tests/RecordingXfermodeTest.cpp
File tests/RecordingXfermodeTest.cpp (right):

https://codereview.chromium.org/568073004/diff/1/tests/RecordingXfermodeTest....
tests/RecordingXfermodeTest.cpp:33: class ExampleDrawer : public Drawer {
Generally I'm keen on interfaces like this, but perhaps we can drop Drawer and
rename ExampleDrawer to Drawer?  There don't seem to be any others.

https://codereview.chromium.org/568073004/diff/1/tests/RecordingXfermodeTest....
tests/RecordingXfermodeTest.cpp:36: :
imageInfo_(SkImageInfo::MakeN32Premul(200,100))
Generally, Skia style wants a big "foo_ -> fFoo" and a 2-space -> 4-space find
and replace here.

[Justin Novosad, 2014-09-12 20:17:05]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2014-09-12 20:17:06]

You should add a test that exercises this fix. There is already a gm test that
covers all the xfer modes. You could create a new flavor that uses saveLayers.

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
File src/core/SkBBoxHierarchyRecord.cpp (right):

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
src/core/SkBBoxHierarchyRecord.cpp:52: case SkXfermode::kClear_Mode:
I feel this logic belongs in a more central location because it is susceptible
of being reused.  Actually, you even duplicated it inside this CL. I would even
suggest exposing it in the public API. Perhaps something like bool
SkXfermode::transparentSourceAffectsDestination().

[mtklein, 2014-09-12 20:23:07]

junov, does the already-existing new test not test this to your satisfaction?

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
File src/core/SkBBoxHierarchyRecord.cpp (right):

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
src/core/SkBBoxHierarchyRecord.cpp:52: case SkXfermode::kClear_Mode:
On 2014/09/12 20:17:06, junov wrote:
> I feel this logic belongs in a more central location because it is susceptible
> of being reused.  Actually, you even duplicated it inside this CL. I would
even
> suggest exposing it in the public API. Perhaps something like bool
> SkXfermode::transparentSourceAffectsDestination().

Let's hold off on that for another CL.  This duplication is ephemeral.  I don't
want to slow down this bug fix with the bikeshedding that comes along with
making things public API.

[mtklein, 2014-09-12 20:38:27]

https://codereview.chromium.org/568073004/diff/20001/tests/RecordingXfermodeT...
File tests/RecordingXfermodeTest.cpp (right):

https://codereview.chromium.org/568073004/diff/20001/tests/RecordingXfermodeT...
tests/RecordingXfermodeTest.cpp:132: SkCanvas* canvas = recorder.beginRecording(
fWidth, fHeight, &factory);
If you'd like to test new vs. old, the better thing to do is test
SkPictureRecorder::beginRecording() against
SkPictureRecorder::DEPRECATED_beginRecording().  We're not getting rid of
SkPictureRecorder, rather we're switching what backend it uses.

EXPERIMENTAL::SkRecording, however, is already dead code walking.  You can force
the same effect via SkPictureRecorder::EXPERIMENTAL_beginRecording(), though the
default in Skia and Chromium (if it hasn't been rolled back) is beginRecording()
== EXPERIMENTAL_beginRecording().

[dneto, 2014-09-12 20:40:00]

On 2014/09/12 20:17:06, junov wrote:
> You should add a test that exercises this fix. There is already a gm test that
> covers all the xfer modes. You could create a new flavor that uses saveLayers.
> 
>
https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
> File src/core/SkBBoxHierarchyRecord.cpp (right):
> 
>
https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
> src/core/SkBBoxHierarchyRecord.cpp:52: case SkXfermode::kClear_Mode:
> I feel this logic belongs in a more central location because it is susceptible
> of being reused.  Actually, you even duplicated it inside this CL. I would
even
> suggest exposing it in the public API. Perhaps something like bool
> SkXfermode::transparentSourceAffectsDestination().

Ask mtklein says, the dm test I added already exercises the functionality.
In fact it checks both the deprecated and the new cases, and it's how
I determined the exact set of modes that needed special casing.

[dneto, 2014-09-12 20:41:42]

I've fixed the nitty gritty issues: formatting, bracing, naming of member
variables.
I've also copied the substantive comment to the fix in the new flow.

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
File src/core/SkBBoxHierarchyRecord.cpp (right):

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
src/core/SkBBoxHierarchyRecord.cpp:42: // Unusual Xfermodes require us to
process a saved layer
On 2014/09/12 19:20:08, mtklein wrote:
> Can you copy (or move) this comment over to SkRecordDraw.cpp?  It'd be a shame
> to lose it when we kill this code off.

Done.

https://codereview.chromium.org/568073004/diff/1/src/core/SkBBoxHierarchyReco...
src/core/SkBBoxHierarchyRecord.cpp:52: case SkXfermode::kClear_Mode:
On 2014/09/12 20:23:07, mtklein wrote:
> On 2014/09/12 20:17:06, junov wrote:
> > I feel this logic belongs in a more central location because it is
susceptible
> > of being reused.  Actually, you even duplicated it inside this CL. I would
> even
> > suggest exposing it in the public API. Perhaps something like bool
> > SkXfermode::transparentSourceAffectsDestination().
> 
> Let's hold off on that for another CL.  This duplication is ephemeral.  I
don't
> want to slow down this bug fix with the bikeshedding that comes along with
> making things public API.

Acknowledged.

https://codereview.chromium.org/568073004/diff/1/src/core/SkRecordDraw.cpp
File src/core/SkRecordDraw.cpp (right):

https://codereview.chromium.org/568073004/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:234: if (paint->getImageFilter() ||
paint->getColorFilter())
On 2014/09/12 19:20:08, mtklein wrote:
> Please add {} 

Done.

https://codereview.chromium.org/568073004/diff/1/src/core/SkRecordDraw.cpp#ne...
src/core/SkRecordDraw.cpp:237: SkXfermode* xfermode = paint->getXfermode();
On 2014/09/12 19:20:08, mtklein wrote:
> On 2014/09/12 18:23:03, dneto wrote:
> > This codepath is not the default for Chromium, but it is the default for
Skia
> > when its own tree.
> 
> Sync up!  :P

Acknowledged.
Yeah, that's outta my control.  I tested with both paths, which was fun. :-)

https://codereview.chromium.org/568073004/diff/1/tests/RecordingXfermodeTest.cpp
File tests/RecordingXfermodeTest.cpp (right):

https://codereview.chromium.org/568073004/diff/1/tests/RecordingXfermodeTest....
tests/RecordingXfermodeTest.cpp:33: class ExampleDrawer : public Drawer {
On 2014/09/12 19:20:08, mtklein wrote:
> Generally I'm keen on interfaces like this, but perhaps we can drop Drawer and
> rename ExampleDrawer to Drawer?  There don't seem to be any others.

Done.

https://codereview.chromium.org/568073004/diff/1/tests/RecordingXfermodeTest....
tests/RecordingXfermodeTest.cpp:36: :
imageInfo_(SkImageInfo::MakeN32Premul(200,100))
On 2014/09/12 19:20:08, mtklein wrote:
> Generally, Skia style wants a big "foo_ -> fFoo" and a 2-space -> 4-space find
> and replace here.

Done.

[mtklein, 2014-09-12 20:57:17]

thanks, sorry for doing this review in waves.  close now

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

https://codereview.chromium.org/568073004/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:250: case SkXfermode::kClear_Mode:
Might note something like,

// For each of these transfer modes, if the source alpha is zero (our
transparent black), the resulting blended alpha is not necessarily equal to the
original destination alpha.

https://codereview.chromium.org/568073004/diff/20001/tests/RecordingXfermodeT...
File tests/RecordingXfermodeTest.cpp (right):

https://codereview.chromium.org/568073004/diff/20001/tests/RecordingXfermodeT...
tests/RecordingXfermodeTest.cpp:70: void writeToFile(std::string fname,
SkPicture* picture) {
remove before submitting?

https://codereview.chromium.org/568073004/diff/20001/tests/RecordingXfermodeT...
tests/RecordingXfermodeTest.cpp:82: const SkRect& intoClip,
some of the indents seem to have gone crazy here.  side effect of s/  /    /g?

https://codereview.chromium.org/568073004/diff/20001/tests/RecordingXfermodeT...
tests/RecordingXfermodeTest.cpp:243: REPORTER_ASSERT_MESSAGE( reporter,
0==numErrors, errors.c_str() );
might just move this line up into the #else?

[Justin Novosad, 2014-09-12 21:02:40]

Sorry, I missed the test first time I looked. (How did that happen?)

[dneto, 2014-09-15 15:48:49]

Thanks for the feedback.  
I've addressed Mike's comment #7 and changed the test to explicitly call both
backends, and fixed the associated comments.

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

https://codereview.chromium.org/568073004/diff/20001/src/core/SkRecordDraw.cp...
src/core/SkRecordDraw.cpp:250: case SkXfermode::kClear_Mode:
On 2014/09/12 20:57:16, mtklein wrote:
> Might note something like,
> 
> // For each of these transfer modes, if the source alpha is zero (our
> transparent black), the resulting blended alpha is not necessarily equal to
the
> original destination alpha.

Done.

[mtklein, 2014-09-15 16:14:00]

The CQ bit was checked by mtklein@google.com

[mtklein, 2014-09-15 16:14:01]

lgtm

[commit-bot: I haz the power, 2014-09-15 16:14:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568073004/40001

[mtklein, 2014-09-15 16:23:10]

On 2014/09/15 16:14:17, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patchset/568073004/40001

Looks like MSVC is feeling a little skittish about some implicit int -> float
conversions:

[16:17:10.923000]
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(51)
: warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible loss
of data
[16:17:10.923000]
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(122)
: warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible loss
of data
[16:17:10.923000]
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(123)
: warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible loss
of data
[16:17:10.923000]
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(161)
: warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible loss
of data
[16:17:10.923000]
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(162)
: warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible loss
of data

Would you mind wrapping those in SkIntToScalar()?

[commit-bot: I haz the power, 2014-09-15 16:29:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 16:29:30]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[dneto, 2014-09-15 17:33:03]

On 2014/09/15 16:23:10, mtklein wrote:
> On 2014/09/15 16:14:17, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patchset/568073004/40001
> 
> Looks like MSVC is feeling a little skittish about some implicit int -> float
> conversions:
> 
> [16:17:10.923000]
>
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(51)
> : warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible
loss
> of data
> [16:17:10.923000]
>
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(122)
> : warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible
loss
> of data
> [16:17:10.923000]
>
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(123)
> : warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible
loss
> of data
> [16:17:10.923000]
>
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(161)
> : warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible
loss
> of data
> [16:17:10.923000]
>
c:\0\build-win-vs2013-x86-debug-trybot\build\skia\tests\recordingxfermodetest.cpp(162)
> : warning C4244: 'argument' : conversion from 'int' to 'SkScalar', possible
loss
> of data
> 
> Would you mind wrapping those in SkIntToScalar()?

Will do.

[dneto, 2014-09-15 17:43:11]

Now the test uses SkIntToScalar. This should satisfy the MSVC compiler.  This
version passes the Skia "dm" tests again.

[mtklein, 2014-09-15 17:44:41]

The CQ bit was checked by mtklein@google.com

[mtklein, 2014-09-15 17:44:42]

lgtm

[commit-bot: I haz the power, 2014-09-15 17:44:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/568073004/60001

[commit-bot: I haz the power, 2014-09-15 17:53:21]

Committed patchset #4 (id:60001) as 327f905d2cb0d37c302d651d8f2b17ea56368467