1215334 - Avoid creating a fake header for BMP files in ICO files

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 1204394 comment 0 says:

> It'd be nice to rework the BMP decoder to use StreamingLexer.

This has now been done. It also says:

> A particular advantage of this is that currently, the ICO decoder has to
> actually synthesize a fake header to pass to BMP decoder, but if the BMP
> decoder used the StreamingLexer / method-per-state approach that the ICO
> decoder uses now, we could avoid that complexity easily and just start
> decoding in a different state for BMPs embedded in ICOs.

This bug is about doing this.

As part of this, I want to separate the data structures used by the BMP decoder and encoder. They currently share FileHeader and V5InfoHeader but the decoder only sets a small number of the fields in these structs. If we create a smaller decoder-only struct that just contains the fields it uses, then we'll avoid having to fake up the unused fields in FileHeader.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Avoid creating a fake header for BMP files in ICO files

The FileHeader and V5InfoHeader structs are shared by the BMP decoder and
encoder. But most of the fields within those structs are actually unused by the
decoder. It makes things clearer if we create a decoder-only struct that
contains the used fields, and then make FileHeader and V5InfoHeader only used
by the encoder. This patch does that.

This patch also renames BMPFileHeaders.h as BMPHeaders.h, which is now a better
name for it.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Avoid creating a fake header for BMP files in ICO files

This requires delaying the creation of the BMP decoder used by the ICO decoder.

Comment 3

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

Comment on attachment 8674630 [details] [diff] [review]
(part 1) - Avoid creating a fake header for BMP files in ICO files

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

Looks good!

::: image/decoders/nsBMPDecoder.h
@@ +31,5 @@
> +  uint32_t mImageSize;      // (compressed) image size. Can be 0 if
> +                            // mCompression==0.
> +  uint32_t mNumColors;      // Used colors.
> +
> +  Header() { memset(this, 0, sizeof(*this)); }

Why not just initialize the fields to 0? Post-C++11 you can just write "uint32_t mDataOffset = 0;".

Comment 4

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

Comment on attachment 8674631 [details] [diff] [review]
(part 2) - Avoid creating a fake header for BMP files in ICO files

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

Looks good! So glad we have delegating constructors now.

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09dc3489f429418d610e7e6b48ce914e0e44200d
Bug 1215334 (part 1) - Avoid creating a fake header for BMP files in ICO files. r=seth.

https://hg.mozilla.org/integration/mozilla-inbound/rev/255b8f808c0bc4bab320ed281e89b62f8053ef27
Bug 1215334 (part 2) - Avoid creating a fake header for BMP files in ICO files. r=seth.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09dc3489f429
https://hg.mozilla.org/mozilla-central/rev/255b8f808c0b