Closed
Bug 1214072
Opened 5 years ago
Closed 5 years ago
Implement transparency properly for BMP images
Categories
(Core :: ImageLib, defect)
Core
ImageLib
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files, 2 obsolete files)
7.45 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
35.96 KB,
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
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•5 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•5 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 | |
Comment 3•5 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a059d92c4c9
Comment 4•5 years ago
|
||
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•5 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 6•5 years ago
|
||
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)
Comment 7•5 years ago
|
||
(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•5 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.
Comment 9•5 years ago
|
||
(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•5 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•5 years ago
|
Attachment #8674062 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 11•5 years ago
|
||
Attachment #8679213 -
Flags: review?(seth)
![]() |
Assignee | |
Updated•5 years ago
|
Attachment #8674113 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 12•5 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 13•5 years ago
|
||
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 14•5 years ago
|
||
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+
![]() |
Assignee | |
Comment 15•5 years ago
|
||
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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e73b37f923c1 https://hg.mozilla.org/mozilla-central/rev/0bdbbc7673c5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•