In order to use CSS luminance masking, we need to be able to create an instance of SkLumaXfermode w…

In order to use CSS luminance masking, we need to be able to create an instance of SkLumaXfermode which can receive
a kSrcOver mode, and applies that mode after converting the source using the luminance-to-alpha coefficients.

BUG: 289420

[Andrei Parvu, 2013-09-11 12:31:22]

I've created a patch which adds the possibility of creating an instance of
SkLumaMaskXfermode with a kSrcOver mode - we need this in order to support css
luminance masking.
Could anyone take a look, please?

[bsalomon, 2013-09-11 14:18:47]

Adding Florin

[reed1, 2013-09-11 14:30:44]

I expect we should create a different hidden subclass at Create time, so we
don't have to pass around fMode and perform the branch in the proc.

[fmalita_google_do_not_use, 2013-09-11 14:34:01]

Andrei, do you have a (work in progress) Blink patch implementing CSS luminance
masking that you can share? Or maybe you can describe how it's going to work -
I'd like to understand how this is going to be used.

Since this is used for masking, I would have thought you can perform luma
conversion during the actual masking phase (I assume there's a srcIn or dstIn
compositing step at some point), like the SVG masking code does. Is that
approach not suitable for CSS?

[Andrei Parvu, 2013-09-11 14:47:17]

On 2013/09/11 14:34:01, fmalita wrote:
> Andrei, do you have a (work in progress) Blink patch implementing CSS
luminance
> masking that you can share? Or maybe you can describe how it's going to work -
> I'd like to understand how this is going to be used.
> 
> Since this is used for masking, I would have thought you can perform luma
> conversion during the actual masking phase (I assume there's a srcIn or dstIn
> compositing step at some point), like the SVG masking code does. Is that
> approach not suitable for CSS?

Thanks for your comments. The current patch for blink can be found here:
https://codereview.chromium.org/23947005.

Florin, that approach is not exactly suitable for CSS, because in CSS we can
specify multiple masks for an element, each of them being either luminance or
alpha.
If we use the current implementation which does the luminance conversion at
dstIn compositing step, all masks will be treated the same (alpha or luminance).
That's why we decided to also add the luminance conversion at the srcOver step
(which is done for each mask in particular).

[f(malita), 2013-09-11 16:53:19]

Ah, makes sense.

Chatting with Mike, it seems a Skia color filter might be a better choice for
supporting luminance-to-alpha conversion in the long run. It would also make the
Blink implementation simpler as it's already part of GraphicsContext's state.
The problem is we can only have one active color filter and setting a sticky
luma CF may clash with color space conversion (the current GC CF owner). This is
solvable by adding some Skia color filter chaining mechanism.

But that's all down the road, and I think it's reasonable to add srcOver support
to the current xfermode implementation, to unblock your CSS masking work.

So back to the patch:

* what Mike said, to avoid branching in the hot loop
* the GL path needs to be updated to handle srcOver also
* we should add some srcOver variants to the reference test (gm/lumamode.cpp)
and possibly to bench (bench/XfermodeBench.cpp)

(in case you're not familiar with it, Skia's GM comes in handy when testing:
build the 'SampleApp' target, run 'out/Debug/SampleApp --slide GM:lumamode' to
skip to the luma GM, then you can press 'd' to switch between the
raster/picture/GL backends).

[Andrei Parvu, 2013-09-12 22:02:11]

I've added a new subclass for the srcover mode, and corrected the actual
luminance conversion and srcOver operation. Also added the srcOver mode to the
GL path and added tests in gm/lumamode.cpp and bench/XfermodeBench.cpp.

[reed1, 2013-09-13 12:44:48]

I suggest we remove all overrides etc. from this header, and *just* expose the
factory. All of the subclasses we return can be private subclasses internal to
the impl file. Basically this makes it a closed subhierarchy, which will give us
the most flexibility in optimizing the impl.

SkBlurMaskFilter, SkGradientShader (and others) already show this sort of
pattern.

https://codereview.chromium.org/24078006/diff/9001/include/effects/SkLumaXfer...
File include/effects/SkLumaXfermode.h (right):

https://codereview.chromium.org/24078006/diff/9001/include/effects/SkLumaXfer...
include/effects/SkLumaXfermode.h:19: *  SrcIn/DstIn xfer:
need to update/generalize the dox

https://codereview.chromium.org/24078006/diff/9001/include/effects/SkLumaXfer...
include/effects/SkLumaXfermode.h:29: *  Only kSrcIn_Mode and kDstIn_Mode are
supported - for everything else,
update dox

https://codereview.chromium.org/24078006/diff/9001/include/effects/SkLumaXfer...
include/effects/SkLumaXfermode.h:60: class SkLumaMaskXfermodeSrcOver : public
SkLumaMaskXfermode {
Lets not make any of the subclasses public. Can always just rely on the factory.

[Andrei Parvu, 2013-09-13 14:01:59]

Moved the srcOver class to the implementation file and updated the dox.

[reed1, 2013-09-13 15:22:59]

api lgtm

https://codereview.chromium.org/24078006/diff/17001/src/effects/SkLumaXfermod...
File src/effects/SkLumaXfermode.cpp (right):

https://codereview.chromium.org/24078006/diff/17001/src/effects/SkLumaXfermod...
src/effects/SkLumaXfermode.cpp:77: return lumaProc(*a, *b);
nit: if this is going to be a method, and not a function-ptr, it needs
this->lumaProc()

[commit-bot: I haz the power, 2013-09-13 18:21:30]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/andrei.prv@gmail.com/24078006/25001

[commit-bot: I haz the power, 2013-09-13 18:26:21]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests,
BuildTools
http://173.255.115.253:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

[Andrei Parvu, 2013-09-14 06:02:04]

On 2013/09/13 18:26:21, I haz the power (commit-bot) wrote:
> Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
> step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests,
> BuildTools
>
http://173.255.115.253:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...

Does anyone know why the apply patch step fails in the build trybots?

[f(malita), 2013-09-16 13:09:18]

On 2013/09/14 06:02:04, Andrei Parvu wrote:
> On 2013/09/13 18:26:21, I haz the power (commit-bot) wrote:
> > Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
> > step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests,
> > BuildTools
> >
>
http://173.255.115.253:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...
> 
> Does anyone know why the apply patch step fails in the build trybots?

I think the bots expect paths to have a phony prefix which gets stripped at
apply time: patch -p1 ... Looking at the raw patch, you're missing that:

--- gm/lumamode.cpp	(revision 11192)
+++ gm/lumamode.cpp	(working copy)

vs. some other random Skia CL:

--- a/gm/gradients.cpp
+++ b/gm/gradients.cpp

How did you upload the patch? git cl upload should do the right thing. I haven't
used straight svn so not sure whether you need to do anything special in that
case.

[Andrei Parvu, 2013-09-16 13:22:22]

On 2013/09/16 13:09:18, Florin Malita wrote:
> On 2013/09/14 06:02:04, Andrei Parvu wrote:
> > On 2013/09/13 18:26:21, I haz the power (commit-bot) wrote:
> > > Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
> > > step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests,
> > > BuildTools
> > >
> >
>
http://173.255.115.253:10117/buildstatus?builder=Build-Ubuntu12-GCC-x86_64-Re...
> > 
> > Does anyone know why the apply patch step fails in the build trybots?
> 
> I think the bots expect paths to have a phony prefix which gets stripped at
> apply time: patch -p1 ... Looking at the raw patch, you're missing that:
> 
> --- gm/lumamode.cpp	(revision 11192)
> +++ gm/lumamode.cpp	(working copy)
> 
> vs. some other random Skia CL:
> 
> --- a/gm/gradients.cpp
> +++ b/gm/gradients.cpp
> 
> How did you upload the patch? git cl upload should do the right thing. I
haven't
> used straight svn so not sure whether you need to do anything special in that
> case.

I don't have commit rights to Skia, so I don't think I can do git cl upload - I
think someone must commit my patch.

[f(malita), 2013-09-16 13:32:34]

On 2013/09/16 13:22:22, Andrei Parvu wrote:
> I don't have commit rights to Skia, so I don't think I can do git cl upload -
I
> think someone must commit my patch.

Most of the steps in
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
(up to the point or actually committing) don't require committer rights. I think
you should be able to upload the patch using git cl, can you give it a try?

[Andrei Parvu, 2013-09-16 13:41:25]

On 2013/09/16 13:32:34, Florin Malita wrote:
> On 2013/09/16 13:22:22, Andrei Parvu wrote:
> > I don't have commit rights to Skia, so I don't think I can do git cl upload
-
> I
> > think someone must commit my patch.
> 
> Most of the steps in
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
> (up to the point or actually committing) don't require committer rights. I
think
> you should be able to upload the patch using git cl, can you give it a try?

I uploaded the patches with gcl upload. Running git cl upload gives the
following error:

"Got error status from ['svn', 'diff',
'd99d102dd27d7acca1bbc16884a1a5719fef2568', 'HEAD']:


Got exception while uploading -- saving description to
/Users/parvu/.git_cl_description_backup"

[bsalomon, 2013-09-16 13:41:27]

sorry for the late review. I just looked at the GPU code and lgtm with one tiny
nit.

https://codereview.chromium.org/24078006/diff/36001/src/effects/SkLumaXfermod...
File src/effects/SkLumaXfermode.cpp (right):

https://codereview.chromium.org/24078006/diff/36001/src/effects/SkLumaXfermod...
src/effects/SkLumaXfermode.cpp:211: builder->fsCodeAppendf("\t\tvec4 newB =
%s;\t\tif (newB.a > 0.0) { newB *= luma / newB.a; }\n\t\tnewB.a = luma;\n",
opB);
nit probably want a \n after the vec4 newB = ...; line

[f(malita), 2013-09-16 13:44:40]

On 2013/09/16 13:41:25, Andrei Parvu wrote:
> On 2013/09/16 13:32:34, Florin Malita wrote:
> > On 2013/09/16 13:22:22, Andrei Parvu wrote:
> > > I don't have commit rights to Skia, so I don't think I can do git cl
upload
> -
> > I
> > > think someone must commit my patch.
> > 
> > Most of the steps in
> >
>
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
> > (up to the point or actually committing) don't require committer rights. I
> think
> > you should be able to upload the patch using git cl, can you give it a try?
> 
> I uploaded the patches with gcl upload. Running git cl upload gives the
> following error:
> 
> "Got error status from ['svn', 'diff',
> 'd99d102dd27d7acca1bbc16884a1a5719fef2568', 'HEAD']:
> 
> 
> Got exception while uploading -- saving description to
> /Users/parvu/.git_cl_description_backup"

Ah, maybe git cl doesn't play well with a gcl change?! I'll take care of it, let
me check a thing with Mike first.

[Andrei Parvu, 2013-09-16 13:58:01]

On 2013/09/16 13:41:27, bsalomon wrote:
> sorry for the late review. I just looked at the GPU code and lgtm with one
tiny
> nit.
> 
>
https://codereview.chromium.org/24078006/diff/36001/src/effects/SkLumaXfermod...
> File src/effects/SkLumaXfermode.cpp (right):
> 
>
https://codereview.chromium.org/24078006/diff/36001/src/effects/SkLumaXfermod...
> src/effects/SkLumaXfermode.cpp:211: builder->fsCodeAppendf("\t\tvec4 newB =
> %s;\t\tif (newB.a > 0.0) { newB *= luma / newB.a; }\n\t\tnewB.a = luma;\n",
> opB);
> nit probably want a \n after the vec4 newB = ...; line

Thanks for the review. Added the nit.

[f(malita), 2013-09-16 15:20:23]

Patch imported: https://codereview.chromium.org/23710053/

I've also added SkLumaMaskXfermodeSrcOver deserialization procs to support
picture playback for the new mode.