Closed
Bug 836892
Opened 12 years ago
Closed 12 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)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: dholbert, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
patch
|
gw280
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gw280
:
review+
|
Details | Diff | Splinter Review |
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).
Reporter | ||
Updated•12 years ago
|
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)
Comment 1•12 years ago
|
||
FYI
Skia will be updated soon to have a real multiply blend operator.
Comment 2•12 years ago
|
||
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
Comment 3•12 years ago
|
||
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•12 years ago
|
||
I'm planning on rebasing soon anyway. Do you have an ETA for when these will land upstream?
Comment 5•12 years ago
|
||
No.
I will ping the skia people again.
Comment 6•12 years ago
|
||
This appears to be the related bug upstream: http://code.google.com/p/skia/issues/detail?id=1037
Comment 7•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
I replaced SkXfermode.cpp/h with the ones you provided in the skia bug report linked in comment 6.
Comment 10•12 years ago
|
||
(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•12 years ago
|
||
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)
Comment 12•12 years ago
|
||
We pass all tests with these patches :)
Attachment #710399 -
Flags: review?(gwright)
Updated•12 years ago
|
Attachment #710399 -
Flags: review?(gwright) → review+
Comment 13•12 years ago
|
||
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+
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/135831bd2127
https://hg.mozilla.org/mozilla-central/rev/bbedfb9d4604
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•