Closed Bug 1213613 Opened 5 years ago Closed 5 years ago

Fix color-scaling of 16bpp BMP images


(Core :: ImageLib, defect)

Not set



Tracking Status
firefox44 --- fixed


(Reporter: njn, Assigned: njn)


(Regressed 1 open bug)


(Whiteboard: [gfx-noted])


(3 files)

We decode a number of images in bmpsuite incorrectly. They mostly/all have to do with incorrect handling of the BITFIELDS compression type, which involves RGB and possibly A bitmasks.
Blocks: 1214072
I've repurposed this bug to be about color-scaling for 16bpp images only. The transparency and 32bpp issues will be done in bug 1214072.
Summary: Fix BITFIELDS problems in the BMP decoder → Fix color-scaling of 16bpp BMP images
Two-space indents is the Gecko standard, and it's what nsBMPDecoder.cpp uses.
This will help avoid possible mis-indentation if code is moved from the .h to
the .cpp or vice versa (as subsequent patches in this bug will do).
Attachment #8673407 - Flags: review?(seth)
This patch moves them into less exposed places. It also moves the RLE_*
constants into the RLE enum.
Attachment #8673408 - Flags: review?(seth)
This patch implements proper color-scaling, instead of bit-shifting, and uses
it for 16bpp images.

It also cleans up the code relating to color masking in the process, by making
BitFields a proper class and introducing the Value class within it.

This fixes sub-optimal handling of four images in bmpsuite.
Attachment #8673409 - Flags: review?(seth)
Comment on attachment 8673407 [details] [diff] [review]
(part 1) - Convert nsBMPEncoder.h to two-space indents

Review of attachment 8673407 [details] [diff] [review]:

This had really annoyed me in the past. Thanks for splitting this patch out to make it easier to review the later patches.
Attachment #8673407 - Flags: review?(seth) → review+
Comment on attachment 8673408 [details] [diff] [review]
(part 2) - Move some BMP-related structs

Review of attachment 8673408 [details] [diff] [review]:

Much better!
Attachment #8673408 - Flags: review?(seth) → review+
Comment on attachment 8673409 [details] [diff] [review]
(part 3) - Fix color-scaling of 16bpp BMP images

Review of attachment 8673409 [details] [diff] [review]:


::: image/decoders/nsBMPDecoder.cpp
@@ +238,2 @@
>    // Find the rightmost 1.
>    uint8_t pos;

We can just define |pos| in the loop header. (I realize you didn't write this line, though!)

@@ +277,5 @@
> +  int32_t i;      // must be a signed integer
> +  for (i = 8 - mBitWidth; i > 0; i -= mBitWidth) {
> +    v2 |= (v << uint32_t(i));
> +  }
> +  v2 |= (v >> uint32_t(-i));

Nice, very clean!

Nit: do you need the outer parens in the right-hand expressions on the two |v2 |= ...| lines? It's up to you whether to keep them; whatever seems more readable.

::: image/decoders/nsBMPDecoder.h
@@ +35,5 @@
> +    void Set(uint32_t aMask);
> +
> +  public:
> +    // Extracts the single color value from the multi-color value.
> +    inline uint8_t Get(uint32_t aVal) const;

Nit: is there actually any point in declaring this inline? I'm under the impression that the compiler doesn't actually listen to |inline| when making inlining decisions, so this is really just allowing you to define these methods in multiple translation units, which you're not doing AFAICT.

@@ +39,5 @@
> +    inline uint8_t Get(uint32_t aVal) const;
> +
> +    // Specialized version of Get() for the case where the bit-width is 5.
> +    // (It will assert if called and the bit-width is not 5.)
> +    inline uint8_t Get5(uint32_t aVal) const;

Nit: see above.
Attachment #8673409 - Flags: review?(seth) → review+
Regressions: 1610106
You need to log in before you can comment on or make changes to this bug.