1215361 - Three BMP/ICO decoder tweaks

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

I have a couple of BMP decoder tweaks.

Comment 1

[Nicholas Nethercote [inactive]]

Actually, I have three, and some are in the ICO decoder.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Don't set mBPP twice in nsICODecoder.cpp

Seth's "XXX" comment is correct.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Streamline nsBMPDecoder's getters

- GetBitsPerPixel() and GetWidth() are no longer used.

- GetHeight() is now only used within nsBMPDecoder and can be renamed and
  inlined into the header.

- GetImageData() can be inlined into the header.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Deconvolute nsICODecoder's handling of endianness

nsICODecoder's reading and writing of little-endian values can be simplified
greatly.

Also, ReadBPP() was highly dodgy: BMP's bpp field is 16-bit
but ReadBPP() read it as if it's 32-bit. I think this currently works because
the bpp field is followed by the 32-bit compression field which is usually 0
for BMPs within ICOs!

Comment 5

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

Comment on attachment 8674633 [details] [diff] [review]
(part 1) - Don't set mBPP twice in nsICODecoder.cpp

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

Nice.

Comment 6

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

Comment on attachment 8674635 [details] [diff] [review]
(part 2) - Streamline nsBMPDecoder's getters

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

Looks good!

Comment 7

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

Comment on attachment 8674638 [details] [diff] [review]
(part 3) - Deconvolute nsICODecoder's handling of endianness

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

Much cleaner!

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4c60957e505a114a77646670576a03c09033f1c
Bug 1215361 (part 1) - Don't set mBPP twice in nsICODecoder.cpp. r=seth.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c6abaa798f99f17827cb06088fc371336b3b879
Bug 1215361 (part 2) - Streamline nsBMPDecoder's getters. r=seth.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6928d7087fedeeeb6cdcff2ddb5eba14ff96163b
Bug 1215361 (part 3) - Deconvolute nsICODecoder's handling of endianness. r=seth.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4c60957e505
https://hg.mozilla.org/mozilla-central/rev/1c6abaa798f9
https://hg.mozilla.org/mozilla-central/rev/6928d7087fed