1213613 - Fix color-scaling of 16bpp BMP images

Status: RESOLVED FIXED

(Core :: ImageLib, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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.

Comment 1

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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.

Comment 2

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 1) - Convert nsBMPEncoder.h to two-space indents

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).

Comment 3

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 2) - Move some BMP-related structs

This patch moves them into less exposed places. It also moves the RLE_*
constants into the RLE enum.

Comment 4

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 3) - Fix color-scaling of 16bpp BMP images

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.

Comment 5

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

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.

Comment 6

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

Comment on attachment 8673408 [details] [diff] [review]
(part 2) - Move some BMP-related structs

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

Much better!

Comment 7

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

Comment on attachment 8673409 [details] [diff] [review]
(part 3) - Fix color-scaling of 16bpp BMP images

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

Nice!

::: 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.

Comment 8

[Nicholas Nethercote [:njn] (On Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9007531fb2561c0a49355780407759448bc2a5a5
Bug 1213613 (part 1) - Formatting cleanups for nsBMPEncoder.h. r=seth.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4d2d8b9af3c4b31156ab96e299c089c4cc2d09e
Bug 1213613 (part 2) - Move some BMP-related structs. r=seth.

https://hg.mozilla.org/integration/mozilla-inbound/rev/855a47b42af501331be597231de9c4bb8a7b3600
Bug 1213613 (part 3) - Fix color-scaling of 16bpp BMP images. r=seth.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9007531fb256
https://hg.mozilla.org/mozilla-central/rev/f4d2d8b9af3c
https://hg.mozilla.org/mozilla-central/rev/855a47b42af5