1214072 - Implement transparency properly for BMP images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

We currently don't support transparency at all for 16bpp BMP images, and only in a really hacky way for 32bpp BMP images. (We also don't implement arbitrary RGB values via masking for 32bpp images; that will need to be fixed in order to get alpha working properly.)

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Read BMP bitfields during metadata decoding

Currently we don't read BMP bitfields during metadata decoding. But we'll need
to in order implement alpha, because we need to know during metadata decoding
if alpha is used.

This patch moves code around to achieve this (and adds the required
mMayHaveTransparency field). The change has no noticeable effect for now.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Implement transparency properly for BMP images

Currently we don't implement transparency at all in BMP images except for an
odd-duck case of BMPs within ICOs.

This patch does the following.

- It implements transparency properly for 16bpp and 32bpp images via bitfield
  masking. (For 32bpp images this also requires handling colors via bitfield
  masking.) The patch maintains the existing BMP-within-ICO transparency
  handling.

- It also reworks BitFields::Value::Set().

  * It now works correctly if the run of 1s goes all the way to bit 31 (the
    old code didn't set mBitWidth).

  * If the mask is 0, will give an mRightShift of 0 (old code gave 32, and
    right-shifting by 32 is dodgy).

  * It's now easier to read.

- It renames transparent.bmp as transparent-if-within-ico.bmp. Ironically
  enough this file currently uses BITFIELDS compression and is WinBMPv5 format,
  which means it contains well-specified alpha data. In order to use it to test
  the hacky BMP-within-ICO transparency scheme the patch changes it to be
  WinBMPv3 format with RGB compression (i.e. no compression). I left the
  now-excess bytes (including the bitfields) in the info header in place
  because that's allowed -- thanks to the start of the pixel data being
  specified by the |dataoffset| field -- they'll just be ignored.

- It tweaks the naming of the relevant gtests and some of their finer details
  to work with the new way of doing things.

This fixes all four remaining failures in bmpsuite.

Comment 3

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a059d92c4c9

Comment 4

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8674062 [details] [diff] [review]
(part 1) - Read BMP bitfields during metadata decoding

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

::: image/decoders/nsBMPDecoder.cpp
@@ +509,5 @@
>  
>    // We treat BMPs as transparent if they're 32bpp and alpha is enabled, but
>    // also if they use RLE encoding, because the 'delta' mode can skip pixels
>    // and cause implicit transparency.
> +  bool mMayHaveTransparency = (mBIH.compression == Compression::RLE8) ||

You're shadowing mMayHaveTransparency here... you should fix this and then retest the code to make sure everything still works as expected.

Comment 5

[Nicholas Nethercote [inactive]]

> You're shadowing mMayHaveTransparency here... you should fix this and then
> retest the code to make sure everything still works as expected.

True! I'll fix it up. But part 2 changes that code again and removes the shadowing. This explains why it hasn't caused me any problems -- all the serious testing I did has had both patches applied.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8674113 [details] [diff] [review]
(part 2) - Implement transparency properly for BMP images

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

Thanks for taking this on! Mostly looks good but needs a retest with the issues mentioned below fixed.

::: image/decoders/nsBMPDecoder.cpp
@@ +297,5 @@
>    v2 |= v >> uint32_t(-i);
>    return v2;
>  }
>  
> +inline uint8_t

Nit: Don't bother with |inline|; use MOZ_ALWAYS_INLINE or leave it off.

@@ +569,5 @@
>      // Need to read bitfields.
>      if (mBIH.bihsize >= InfoHeaderLength::WIN_V4) {
>        // Bitfields are present in the info header, so we can read them
>        // immediately.
> +      mBitFields.ReadFromHeader(aData + 36, /* readAlpha = */ true);

Nit: aReadAlpha.

@@ +605,5 @@
>  
>    // If aLength is zero there are no bitfields to read, or we already read them
>    // in ReadInfoHeader().
>    if (aLength != 0) {
> +    mBitFields.ReadFromHeader(aData, /* readAlpha = */ false);

Same.

@@ +761,5 @@
>            uint16_t val = LittleEndian::readUint16(src);
>            SetPixel(dst, mBitFields.mRed.Get(val),
>                          mBitFields.mGreen.Get(val),
> +                        mBitFields.mBlue.Get(val),
> +                        mBitFields.mAlpha.GetAlpha(val, hasAlpha));

So this only ends up testing the last pixel, right? Gotta || the alpha values together.

@@ +820,5 @@
> +          uint32_t val = LittleEndian::readUint32(src);
> +          SetPixel(dst, mBitFields.mRed.Get(val),
> +                        mBitFields.mGreen.Get(val),
> +                        mBitFields.mBlue.Get(val),
> +                        mBitFields.mAlpha.GetAlpha(val, hasAlpha));

Same issue here; need ||.

::: image/decoders/nsICODecoder.cpp
@@ +659,4 @@
>  
> +  nsRefPtr<nsBMPDecoder> bmpDecoder =
> +    static_cast<nsBMPDecoder*>(mContainedDecoder.get());
> +  bmpDecoder->SetHasTransparency();

Not sure I understand this change. You want to treat all BMP-in-ICO as transparent?

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #6)
> So this only ends up testing the last pixel, right? Gotta || the alpha
> values together.

"Gotta || the *hasAlpha* values together" is what I mean to say.

Comment 8

[Nicholas Nethercote [inactive]]

> @@ +761,5 @@
> >            uint16_t val = LittleEndian::readUint16(src);
> >            SetPixel(dst, mBitFields.mRed.Get(val),
> >                          mBitFields.mGreen.Get(val),
> > +                        mBitFields.mBlue.Get(val),
> > +                        mBitFields.mAlpha.GetAlpha(val, hasAlpha));
> 
> So this only ends up testing the last pixel, right? Gotta || the alpha
> values together.

GetAlpha() effectively does that -- it sets |hasAlpha| to true if appropriate but doesn't set it to false in the other case.


> ::: image/decoders/nsICODecoder.cpp
> @@ +659,4 @@
> >  
> > +  nsRefPtr<nsBMPDecoder> bmpDecoder =
> > +    static_cast<nsBMPDecoder*>(mContainedDecoder.get());
> > +  bmpDecoder->SetHasTransparency();
> 
> Not sure I understand this change. You want to treat all BMP-in-ICO as
> transparent?

Hmm. Maybe that code should stay within the |if (mHasMaskAlpha)| block.

Comment 9

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> GetAlpha() effectively does that -- it sets |hasAlpha| to true if
> appropriate but doesn't set it to false in the other case.

OK, that works. It's easy for the reader to misunderstand what's happening, though. (At least, I did =) I think it'd be a little clearer if |hasAlpha| was called something like |anyPixelHasAlpha| or maybe just |anyHasAlpha| - that would prime the reader to interpret this as a fold.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Read BMP bitfields during metadata decoding

Currently we don't read BMP bitfields during metadata decoding. But we'll need
to in order implement alpha, because we need to know during metadata decoding
if alpha is used.

This patch moves code around to achieve this (and adds the required
mMayHaveTransparency field). The change has no noticeable effect for now.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Implement transparency properly for BMP images

Comment 12

[Nicholas Nethercote [inactive]]

I updated the patches to address review comments. Local reftest and gtest runs look good. I'll do a try push before landing. Re-requesting review.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8679211 [details] [diff] [review]
(part 1) - Read BMP bitfields during metadata decoding

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

Looks good!

Comment 14

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8679213 [details] [diff] [review]
(part 2) - Implement transparency properly for BMP images

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

Ship it!

Comment 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e73b37f923c1c90bd0d2d9037d604537a9cdd495
Bug 1214072 (part 1) - Read BMP bitfields during metadata decoding. r=seth.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bdbbc7673c5c6f8a6223424e3ac386d2d1675bb
Bug 1214072 (part 2) - Implement transparency properly for BMP images. r=seth.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e73b37f923c1
https://hg.mozilla.org/mozilla-central/rev/0bdbbc7673c5