836892 - GfxOpToSkiaOp needs updating to handle new ops from bug 809927 (to fix warnings like "HelpersSkia.h:134:10: warning: enumeration value 'OP_MULTIPLY' not handled in switch [-Wswitch]" in every file that includes HelpersSkia.h)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

Right now I get this pile of build warnings, repeated for each file that #includes HelpersSkia.h:
{
 8:23.27 In file included from /mozilla-central/gfx/2d/Scale.cpp:8:0:
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h: In function 'SkXfermode::Mode mozilla::gfx::GfxOpToSkiaOp(mozilla::gfx::CompositionOp)':
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_MULTIPLY' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_MULTIPLY' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_SCREEN' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_SCREEN' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_OVERLAY' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_OVERLAY' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_DARKEN' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_DARKEN' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_LIGHTEN' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_LIGHTEN' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_COLOR_DODGE' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_COLOR_DODGE' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_COLOR_BURN' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_COLOR_BURN' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_HARD_LIGHT' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_HARD_LIGHT' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_SOFT_LIGHT' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_SOFT_LIGHT' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_DIFFERENCE' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_DIFFERENCE' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_EXCLUSION' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_EXCLUSION' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_HUE' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_HUE' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_SATURATION' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_SATURATION' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_COLOR' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_COLOR' not handled in switch [-Wswitch]
 8:23.27 Warning: -Wswitch in /mozilla-central/gfx/2d/HelpersSkia.h: enumeration value 'OP_LUMINOSITY' not handled in switch
 8:23.27 /mozilla-central/gfx/2d/HelpersSkia.h:134:10: warning: enumeration value 'OP_LUMINOSITY' not handled in switch [-Wswitch]
}

This looks like it might be a real bug -- the called-out OP_ values were all added in bug 809927, without adding any clauses for their types to the 'switch' statement in GfxOpToSkiaOp().

Right now, these enum values will all hit the final "return SkXfermode::kSrcOver_Mode;" clause. Not sure if that's correct. (and even if it is, we should either call them out explicitly in the 'switch' statement, or just use a "default" statement, to take care of this warning).

Comment 1

[Rik Cabanier]

FYI
Skia will be updated soon to have a real multiply blend operator.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Unfortunately skia doesn't appear to support the final 4 operators, and it fails the test for COLOR_DODGE.

Not sure what we can do here, but this is going to block bug 831525

Comment 3

[Rik Cabanier]

Those operations are scheduled to be submitted to skia soon.

I have the implementation, it's just taking a bit for the skia people to integrate it.

Comment 4

[George Wright (:gw280) (needinfo me!)]

I'm planning on rebasing soon anyway. Do you have an ETA for when these will land upstream?

Comment 5

[Rik Cabanier]

No.
I will ping the skia people again.

Comment 6

[George Wright (:gw280) (needinfo me!)]

This appears to be the related bug upstream: http://code.google.com/p/skia/issues/detail?id=1037

Comment 7

[Matt Woodrow (:mattwoodrow)]

Just tried those files out locally, get a few failures and then a crash.

est_2d.composite.canvas.hard-light.html, test_2d.composite.canvas.hue.html and test_2d.composite.canvas.lighten.html all fail.

Then we crash during test_2d.composite.canvas.luminosity.html because the a value is greater than the g value during the SkPackARGB32 call in luminosity_modeproc.

Comment 8

[Rik Cabanier]

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Just tried those files out locally, get a few failures and then a crash.
> 
> est_2d.composite.canvas.hard-light.html, test_2d.composite.canvas.hue.html
> and test_2d.composite.canvas.lighten.html all fail.
> 
> Then we crash during test_2d.composite.canvas.luminosity.html because the a
> value is greater than the g value during the SkPackARGB32 call in
> luminosity_modeproc.

Skia doesn't have support for luminosity, hue, color and saturation yet so I'm unsure how you are passing it in.

I know the problem with hard-light. This should be fixed soon.
Also, the fix for multiply landed today so you should reintegrate.

Comment 9

[Matt Woodrow (:mattwoodrow)]

I replaced SkXfermode.cpp/h with the ones you provided in the skia bug report linked in comment 6.

Comment 10

[Rik Cabanier]

(In reply to Matt Woodrow (:mattwoodrow) from comment #9)
> I replaced SkXfermode.cpp/h with the ones you provided in the skia bug
> report linked in comment 6.

ah. That one had a couple of issue. Change the following functions:
 static inline int hardlight_byte(int sc, int dc, int sa, int da) {
      if(!sa || !da)
          return sc * da;
      float Sc = (float)sc/sa;
      float Dc = (float)dc/da;
      if(Sc <= 0.5)
          Sc *= 2 * Dc;
      else
          Sc = -1 + 2 * Sc + 2 * Dc - 2 * Sc * Dc;
  
     return Sc * sa * da;
 }

and:

 static inline BlendColor SetLum(BlendColor C, float l) {
   float d = l - Lum(C);
    C.r +=  d;
    C.g +=  d;
    C.b +=  d;
  
    return clipColor(C); // <---
 }

These fixes will also make it into skia soon

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Import the new skia xfermodes

I really want to get skia enabled on windows xp this week, so I've imported the required changes into our tree.

I think we should take this locally until we can get an updated version of skia that includes it.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Add new skia blending modes to GfxOpToSkiaOp

We pass all tests with these patches :)

Comment 13

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 710398 [details] [diff] [review]
Import the new skia xfermodes

Review of attachment 710398 [details] [diff] [review]:
-----------------------------------------------------------------

In general I like it.

::: gfx/skia/include/core/SkXfermode.h
@@ +36,2 @@
>      virtual void xferA8(SkAlpha dst[], const SkPMColor src[], int count,
> +                        const SkAlpha aa[]) const;

All these const changes aren't technically part of this patch. They're artefacts from https://codereview.appspot.com/6941065. Do we really want them? It seems harmless to me, though.

@@ +67,5 @@
>          dst         zero            one
>          srcover     one             isa
>          dstover     ida             one
>       */
> +    virtual bool asCoeff(Coeff* src, Coeff* dst) const;

Same as above

@@ +73,5 @@
>      /**
>       *  The same as calling xfermode->asCoeff(..), except that this also checks
>       *  if the xfermode is NULL, and if so, treats its as kSrcOver_Mode.
>       */
> +    static bool AsCoeff(const SkXfermode*, Coeff* src, Coeff* dst);

Same

@@ +118,5 @@
>          kExclusion_Mode,
> +		kHue_Mode,
> +		kSaturation_Mode,
> +		kColor_Mode,
> +	    kLuminosity_Mode,

Indentation

@@ +128,5 @@
>       *  If the xfermode is one of the modes in the Mode enum, then asMode()
>       *  returns true and sets (if not null) mode accordingly. Otherwise it
>       *  returns false and ignores the mode parameter.
>       */
> +    virtual bool asMode(Mode* mode) const;

Same as above

@@ +134,5 @@
>      /**
>       *  The same as calling xfermode->asMode(mode), except that this also checks
>       *  if the xfermode is NULL, and if so, treats its as kSrcOver_Mode.
>       */
> +    static bool AsMode(const SkXfermode*, Mode* mode);

Same

@@ +146,5 @@
>       *                         SkXfermode::kDstOver_Mode)) {
>       *      ...
>       *  }
>       */
> +    static bool IsMode(const SkXfermode* xfer, Mode mode);

Same

@@ +173,5 @@
>       */
>      static bool ModeAsCoeff(Mode mode, Coeff* src, Coeff* dst);
>  
>      // DEPRECATED: call AsMode(...)
> +    static bool IsMode(const SkXfermode* xfer, Mode* mode) {

Same

@@ +189,5 @@
>  
>          This method will not be called directly by the client, so it need not
>          be implemented if your subclass has overridden xfer32/xfer16/xferA8
>      */
> +    virtual SkPMColor xferColor(SkPMColor src, SkPMColor dst) const;

Same

@@ +220,2 @@
>      virtual void xferA8(SkAlpha dst[], const SkPMColor src[], int count,
> +                        const SkAlpha aa[]) const SK_OVERRIDE;

Same

::: gfx/skia/src/core/SkXfermode.cpp
@@ -283,3 @@
>  }
>  static SkPMColor colordodge_modeproc(SkPMColor src, SkPMColor dst) {
> -    // added to avoid div-by-zero in colordodge_byte

Might be worth moving this comment to its new location in colordodge_byte()

@@ -318,3 @@
>  }
>  static SkPMColor colorburn_modeproc(SkPMColor src, SkPMColor dst) {
> -    // added to avoid div-by-zero in colorburn_byte

Again here for colorburn_byte()

@@ +377,5 @@
>      int da = SkGetPackedA32(dst);
>      int a = srcover_byte(sa, da);
> +    int r = blendfunc_byte(SkGetPackedR32(src), SkGetPackedR32(dst), sa, da, exclusion_byte);
> +    int g = blendfunc_byte(SkGetPackedG32(src), SkGetPackedG32(dst), sa, da, exclusion_byte);
> +    int b = blendfunc_byte(SkGetPackedB32(src), SkGetPackedB32(dst), sa, da, difference_byte);

s/difference/exclusion/?

Comment 14

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/135831bd2127
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbedfb9d4604

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/135831bd2127
https://hg.mozilla.org/mozilla-central/rev/bbedfb9d4604