Closed Bug 836892 Opened 7 years ago Closed 7 years ago

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)

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dholbert, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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).
Summary: GfxOpToSkiaOp needs updating to handle new ops from bug 809927 (to fix warnings like "elpersSkia.h:134:10: warning: enumeration value 'OP_MULTIPLY' not handled in switch [-Wswitch]" in every file that includes HelpersSkia.h) → 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)
FYI
Skia will be updated soon to have a real multiply blend operator.
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
Blocks: 831525
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.
I'm planning on rebasing soon anyway. Do you have an ETA for when these will land upstream?
No.
I will ping the skia people again.
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.
(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.
I replaced SkXfermode.cpp/h with the ones you provided in the skia bug report linked in comment 6.
(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
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.
Attachment #710398 - Flags: review?(gwright)
We pass all tests with these patches :)
Attachment #710399 - Flags: review?(gwright)
Attachment #710399 - Flags: review?(gwright) → review+
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/?
Attachment #710398 - Flags: review?(gwright) → review+
Blocks: 839269
https://hg.mozilla.org/mozilla-central/rev/135831bd2127
https://hg.mozilla.org/mozilla-central/rev/bbedfb9d4604
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.