Fix color-scaling of 16bpp BMP images

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(3 attachments)

Assignee

Description

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

Updated

4 years ago
Blocks: 1214072
Assignee

Comment 1

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

Updated

4 years ago
Summary: Fix BITFIELDS problems in the BMP decoder → Fix color-scaling of 16bpp BMP images
Assignee

Comment 2

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

Comment 3

4 years ago
This patch moves them into less exposed places. It also moves the RLE_*
constants into the RLE enum.
Attachment #8673408 - Flags: review?(seth)
Assignee

Comment 4

4 years ago
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]:
-----------------------------------------------------------------

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.
Attachment #8673409 - Flags: review?(seth) → review+
You need to log in before you can comment on or make changes to this bug.