Implement transparency properly for BMP images

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

4 years ago
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.)
Assignee

Comment 1

4 years ago
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.
Attachment #8674062 - Flags: review?(seth)
Assignee

Comment 2

4 years ago
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.
Attachment #8674113 - Flags: review?(seth)
Assignee

Updated

4 years ago
Blocks: 1215334
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.
Attachment #8674062 - Flags: review?(seth)
Assignee

Comment 5

4 years ago
> 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 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?
Attachment #8674113 - Flags: review?(seth)
(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.
Assignee

Comment 8

4 years ago
> @@ +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.
(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.
Assignee

Comment 10

4 years ago
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.
Attachment #8679211 - Flags: review?(seth)
Assignee

Updated

4 years ago
Attachment #8674062 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8674113 - Attachment is obsolete: true
Assignee

Comment 12

4 years ago
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 on attachment 8679211 [details] [diff] [review]
(part 1) - Read BMP bitfields during metadata decoding

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

Looks good!
Attachment #8679211 - Flags: review?(seth) → review+
Comment on attachment 8679213 [details] [diff] [review]
(part 2) - Implement transparency properly for BMP images

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

Ship it!
Attachment #8679213 - Flags: review?(seth) → review+

Comment 16

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e73b37f923c1
https://hg.mozilla.org/mozilla-central/rev/0bdbbc7673c5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

4 years ago
Duplicate of this bug: 1014810

Updated

3 years ago
Duplicate of this bug: 861639
You need to log in before you can comment on or make changes to this bug.