839383 - gfx/2d/SourceSurfaceCG.cpp:64:11: warning: enumeration value 'FORMAT_R5G6B5' not handled in switch [-Wswitch]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Holbert [:dholbert]]

{
../../../gfx/2d/SourceSurfaceCG.cpp:64:11: warning: enumeration value 'FORMAT_R5G6B5' not handled in switch [-Wswitch]
  switch (aFormat) {
          ^
../../../gfx/2d/SourceSurfaceCG.cpp:153:11: warning: enumeration value 'FORMAT_R5G6B5' not handled in switch [-Wswitch]
  switch (aFormat) {
          ^
2 warnings generated.
}

This is for this chunk of code:
> 49 bool
> 50 SourceSurfaceCG::InitFromData(unsigned char *aData,
> 51                                const IntSize &aSize,
> 52                                int32_t aStride,
> 53                                SurfaceFormat aFormat)
> 54 {
[...]
> 64   switch (aFormat) {
> 65     case FORMAT_B8G8R8A8:
> 66       colorSpace = CGColorSpaceCreateDeviceRGB();
> 67       bitinfo = kCGImageAlphaPremultipliedFirst | kCGBitmapByteOrder32Host;
> 68       bitsPerComponent = 8;
> 69       bitsPerPixel = 32;
> 70       break;
> 71 
> 72     case FORMAT_B8G8R8X8:
> 73       colorSpace = CGColorSpaceCreateDeviceRGB();
> 74       bitinfo = kCGImageAlphaNoneSkipFirst | kCGBitmapByteOrder32Host;
> 75       bitsPerComponent = 8;
> 76       bitsPerPixel = 32;
> 77       break;
> 78 
> 79     case FORMAT_A8:
> 80       // XXX: why don't we set a colorspace here?
> 81       bitsPerComponent = 8;
> 82       bitsPerPixel = 8;
> 83   };
https://mxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceCG.cpp#49

FORMAT_R5G6B5 is the only value of this enum type that isn't checked in this switch statement. I'm guessing that it probably should be in there?

Comment 1

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> This is for this chunk of code:

(And a chunk of duplicated code lower down in the file, at
 https://mxr.mozilla.org/mozilla-central/source/gfx/2d/SourceSurfaceCG.cpp#140
)

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: fix unhandled enumeration values in SourceSurfaceCG.cpp

Move common code to AssignSurfaceParametersForFormat and add a default
switch case that does nothing.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: use new AssignSurfaceParametersFromFormat for EnsureImage methods

Now that we have this shiny new method, we might as well use it in a few
other places.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: remove tabs in SourceSurfaceCG.cpp

Not strictly part of this bug, but the tab-width: 20 at the top of the file
makes the tabs *really* obvious in emacs...

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Created attachment 721737 [details] [diff] [review]
> fix unhandled enumeration values in SourceSurfaceCG.cpp
> 
> Move common code to AssignSurfaceParametersForFormat and add a default
> switch case that does nothing.

Do we really want to add a "default" case here? Doesn't that just suppress the problem rather than addressing it?

If we're really sure there's nothing that we need to do for FORMAT_R5G6B5, we should just add an explicit empty case for it, IMHO. (And if we're not sure, we shouldn't just silence the warning w/ "default" when it could be a real bug.)

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 721737 [details] [diff] [review]
fix unhandled enumeration values in SourceSurfaceCG.cpp

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

If we're going to be handle the default case we should make things blow up.

::: gfx/2d/SourceSurfaceCG.cpp
@@ +73,5 @@
> +      bitsPerComponent = 8;
> +      bitsPerPixel = 8;
> +      break;
> +
> +    default:

We should abort in the default case.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 721738 [details] [diff] [review]
use new AssignSurfaceParametersFromFormat for EnsureImage methods

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

::: gfx/2d/SourceSurfaceCG.cpp
@@ +314,4 @@
>  
> +      AssignSurfaceParametersFromFormat(FORMAT_B8G8R8A8,
> +                                        colorSpace, bitinfo,
> +                                        bitsPerComponent, bitsPerPixel);

I find passing the zero values as parameters confusing. I'm not sure what a better approach is. Perhaps putting things in a struct and returning that by value. Or even better create a better helper for constructing CGImages for a format.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 721739 [details] [diff] [review]
remove tabs in SourceSurfaceCG.cpp

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

This is still probably a good thing to do.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Comment on attachment 721737 [details] [diff] [review]
[...]
> If we're going to be handle the default case we should make things blow up.
[...]
> We should abort in the default case.

It's better to just handle the cases we expect (including FORMAT_R5G6B5) and have *no* default case at all.

Then, once we have FAIL_ON_WARNINGS for this directory, we'll turn TBPL red if we ever add another enum value without handling it. (This is the only bug that's preventing that from happening, FWIW.)  That lets us blow up *really* early -- at compile-time instead of at runtime-under-the-right-conditions. :)

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: fix unhandled enumeration values in SourceSurfaceCG.cpp

Updated to crash on the default case per review.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: factor out CGImageRef creation code in SourceSurfaceCG

Here's a better idea for a helper routine for creating CGImageRef.

AssignSurfaceParamatersFromFormat will be inlined in a subsequent patch; just
separating this out for ease of review.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: inline AssignSurfaceParametersFromFormat into its only caller

Trivial patch.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

I'll try to get to this tomorrow.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Nathan Froyd (:froydnj) from comment #10)
> Created attachment 724125 [details] [diff] [review]
> fix unhandled enumeration values in SourceSurfaceCG.cpp
> 
> Updated to crash on the default case per review.

It'd still be better to have *no* default case at all, IMHO (as I suggested in comments 5 and 9), especially once this bug is fixed & we can turn on FAIL_ON_WARNINGS for this code.

Consider what happens when someone writes a patch adding a new enum value, and they forget to add it to this switch statement.
 A) If there is a default case, then everything will compile fine & work fine until someone happens to encounter a configuration/test-case that hits this switch statement w/ the new value. (which might not happen right away, depending on how well-tested the code is)

 B) If there's *not* a default case, then there will be a build warning for the missing enum value, which we'll treat as an error, so we'll fail to compile & catch the bug right away.

Having said that, if Jeff really wants a default case that aborts, he's got the say-so over this code, so I'll defer to him. :)

Comment 15

[Nathan Froyd [:froydnj]]

Committed with minor comment fixes in the second patch.

https://tbpl.mozilla.org/?tree=Try&rev=609396a9add1

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b354780c5ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2715e049b3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06fa2317215

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3b354780c5ac
https://hg.mozilla.org/mozilla-central/rev/c2715e049b3c
https://hg.mozilla.org/mozilla-central/rev/b06fa2317215