744686 - Remove dead alpha_bits in nsPNGDecoder.cpp

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

GCC tells me this:

nsPNGDecoder.cpp: In static member function ‘static void mozilla::image::nsPNGDecoder::info_callback(png_structp, png_infop)’:
/home/njn/moz/mi3/image/decoders/nsPNGDecoder.cpp:612:11: warning: variable ‘alpha_bits’ set but not used [-Wunused-but-set-variable]

There's a whole complex series of nested statements (including a loop!) to compute alpha_bits.  Quite odd, but definitely dead.  (It's also conceivable that alpha_bits should be used subsequently, but I'm not qualified to determine that.)

I haven't run this past the try server yet, if I get r+ I'll do that before landing.

Comment 1

[Glenn Randers-Pehrson]

This is related to Alfred's patch to bug #517713 (which keeps going dormant for some reason).

Comment 2

[Glenn Randers-Pehrson]

I agree that the code can be removed.  Once upon a time we made use of the alpha_bits variable but not any more.

Comment 3

[Alfred Kayser]

The code cannot be removed if my patch is to be applied. In that case, the alphaBit/Byte check is required.

The patch in bug 517713 is dormant, because it requires superreview...
Who wants to take that on?

Comment 4

[Glenn Randers-Pehrson]

(In reply to Alfred Kayser from comment #3)
> The code cannot be removed if my patch is to be applied. In that case, the
> alphaBit/Byte check is required.
>

That's correct.  If/when Alfred's patch is landed after this one, it'll have to restore the
alpha_bits code.  The reason for storing alpha_bits is that premultiplying can be skipped later on, and finding alpha_bits is pretty quick compared to examining the entire alpha channel.  If Alfred's patch is landed first, then this one becomes obsolete.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 614260 [details] [diff] [review]
patch

Sure. We can add something like this back if needed.

Comment 7

[Daniel Holbert [:dholbert]]

Looks like the patch in bug 517713 has some review feedback waiting to be addressed before it can land.  When another patch is ready in that bug, it can easily restore the otherwise-dead code here. 

So -- it looks like the patch here is reviewed and needs-landing.  Can we check this in?

Comment 8

[Nicholas Nethercote [inactive]]

> So -- it looks like the patch here is reviewed and needs-landing.  Can we
> check this in?

Is there much point removing code that a subsequent patch will just restore?  I suppose it might make us faster in the interim, and will avoid the warning -- dholbert, is that your desire?

Comment 9

[Daniel Holbert [:dholbert]]

Depends on how soon the subsequent patch will be landing. :)

In the meantime, I believe this is the only warning keeping this directory from being flagged as FAIL_ON_WARNINGS (as noted in bug 802490 comment 0), so it's holding us back from automatically preventing build warnings elsewhere in this file & directory.

Comment 10

[Nicholas Nethercote [inactive]]

Ok!  I'll land a patch that comments out the code in question and leaves a pointer to bug 517713.

Comment 11

[Daniel Holbert [:dholbert]]

...though I suppose we could also add
  //  This code is currently unused, but it's needed for bug 517713, when it's ready to land:
  #ifdef 0
   [... DEAD CODE ...]
  #endif 
to preserve hg blame etc. while still disabling the code for now.  Maybe that'd be better?

Comment 12

[Daniel Holbert [:dholbert]]

er #if 0 (not #ifdef)

Comment 13

[Nicholas Nethercote [inactive]]

Good idea.

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/357a60386b52

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/357a60386b52