Avoid creating a fake header for BMP files in ICO files

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8674630 [details] [diff] [review]
(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.
Attachment #8674630 - Flags: review?(seth)
(Assignee)

Comment 2

3 years ago
Created attachment 8674631 [details] [diff] [review]
(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.
Attachment #8674631 - Flags: review?(seth)
(Assignee)

Updated

3 years ago
Blocks: 1215361
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;".
Attachment #8674630 - Flags: review?(seth) → review+
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.
Attachment #8674631 - Flags: review?(seth) → review+
(Assignee)

Comment 5

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/09dc3489f429
https://hg.mozilla.org/mozilla-central/rev/255b8f808c0b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.