Create decoders with a DecoderFactory

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Assignee

Description

4 years ago
In order to support using Decoders independently of Image objects, which we want in the short term for bug 1181863 and in the long term to allow better testing and fuzzing of the decoders, we need to pull the code for creating a decoder out of RasterImage.

In this bug we'll add a DecoderFactory which will be responsible for creating decoders.
Assignee

Updated

4 years ago
Depends on: 1185582
Assignee

Updated

4 years ago
Depends on: 1185592
Assignee

Comment 1

4 years ago
This is a purely refactoring bug - no functionality will change. But I want to do some reasonably substantial refactoring here, because I'd like DecoderFactory to have an interface closer to what I think we'll want long term, and so I'll need to update the calling code to follow the new design.

In particular, the two big changes I want to make in DecoderFactory compared to the existing code are:

- Start calling "size decodes" "metadata decodes". The name "size decode" confuses everybody that doesn't work on ImageLib, and it won't be accurate soon anyway since we are going to start gathering more information than just size during these decodes.

- Stop using the pattern of passing a Maybe<IntSize> to decoding methods, with Nothing() indicating a request for a size decode. This is unnecessarily confusing to the reader - again, this is something that always trips up people who don't work on ImageLib. Let's get rid of this and start using two separate Decode()-like methods, one for metadata decodes and one for full decodes.
Assignee

Comment 2

4 years ago
This first part is the core change here; the rest of the parts just propagate
the ideas in this patch outward so that the rest of ImageLib is in harmony with
this code.

Basically all that's happening here is that the core functionality related to
creating decoders has been moved into DecodeFactory. There are methods for
converting a MIME type to a DecoderType, for creating a metadata decoder, and
for creating a full decoder.

Moving that code around required updating various files, both to call the new
code and in some cases to adjust #include directives.

There are some things that are a bit awkward here (for example, callers don't
use the "metadata decoder" terminology that DecodeFactory uses), but those
things will get fixed in later parts.
Attachment #8636362 - Flags: review?(tnikkel)
Assignee

Comment 3

4 years ago
This patch propagates outward the new approach of having separate methods for
starting a metadata decode vs. a full decode. In the old code,
RasterImage::Decode() and RasterImage::CreateDecoder() were full of lots of |if
(aSize)| checks - the code that ran for metadata decodes had very little in
common with the code that ran for full decodes, but having them interleaved made
reading them harder. Also, as mentioned above, calling RasterImage::Decode() and
passing Nothing() for the aSize parameter to get a metadata decode is just
confusing.

The solution is to split out Decode() into DecodeMetadata() (for metadata
decodes) and Decode() (for full decodes). The relevant portion of
CreateDecoder() (which isn't that much now that much of the work is done in
DecoderFactory) is then inlined into each method. The result is something which
I think is easier to read, both at callsites and in the implementation.
Attachment #8636366 - Flags: review?(tnikkel)
Assignee

Comment 4

4 years ago
This patch propagates the switch from "size decodes" to "metadata decodes" to
the rest of ImageLib. All that happens here is renaming methods and variables
and updating comments.
Attachment #8636368 - Flags: review?(tnikkel)
Assignee

Comment 5

4 years ago
Finally, we want to forbid instantiation of decoders except via DecoderFactory,
just as we do with images and ImageFactory.

We can't actually forbid it totally, because nsICODecoder has some special needs
that DecoderFactory doesn't currently support - in particular, it needs to
instantiate its contained decoders without providing a SourceBuffer iterator.
Instead of using an iterator, nsICODecoder writes to its contained decoders
using Decoder::Write(), an API which is otherwise private.

Rather than make DecoderFactory support this pattern, I'd like to make
nsICODecoder interact with its contained decoders using only the public API.
I'll file a followup about making that change since it's too complex to be
another part in this bug. For now, we'll just allow nsICODecoder to instantiate
the PNG and BMP decoders directly, as it currently does.
Attachment #8636370 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
Blocks: 1185799
Assignee

Updated

4 years ago
Blocks: 1185800
Assignee

Comment 7

4 years ago
Whoops. I was calling SetImageIsLocked() after Decoder::Init(), which triggered
an assertion that made these patches fail everywhere in debug builds. I fixed
that; this version of part 1 is otherwise unchanged.
Attachment #8636393 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
Attachment #8636362 - Attachment is obsolete: true
Attachment #8636362 - Flags: review?(tnikkel)
Assignee

Comment 8

4 years ago
Part 2 needed a rebase against the new version of part 1.
Attachment #8636394 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
Attachment #8636366 - Attachment is obsolete: true
Attachment #8636366 - Flags: review?(tnikkel)
Assignee

Comment 10

4 years ago
So try is green on Linux. There's still an issue with some BMP-related definitions conflicting with definitions in Windows.h; once I get that cleared up, this should be green on every platform.
Assignee

Updated

4 years ago
Depends on: 1186112
Assignee

Comment 11

4 years ago
Bug 1186112 should resolve the conflicts with windows.h - not just for this patch, but permanently.
Attachment #8636393 - Flags: review?(tnikkel) → review+
Comment on attachment 8636394 [details] [diff] [review]
(Part 2) - Clean up RasterImage's decoding API

>+  // XXX(seth): It doesn't make sense to only report metadata decodes, but
>+  // that's what we're doing right now.

I looked up the history of the original code in RasterImage::CreateDecoder. It seems to be a bug introduced by http://hg.mozilla.org/mozilla-central/rev/08965492c770 where we changed |bool aDoSizeDecode| to |const Maybe<nsIntSize>& aSize| and |if (!aDoSizeDecode)| to |if (!aSize)|. Can we just fix this in a patch that lands before this stack and then do the proper behaviour in this page?
Attachment #8636394 - Flags: review?(tnikkel) → review+
Attachment #8636368 - Flags: review?(tnikkel) → review+
Attachment #8636370 - Flags: review?(tnikkel) → review+
Assignee

Comment 13

4 years ago
(In reply to Timothy Nikkel (:tn) from comment #12)
> Comment on attachment 8636394 [details] [diff] [review]
> (Part 2) - Clean up RasterImage's decoding API
> 
> >+  // XXX(seth): It doesn't make sense to only report metadata decodes, but
> >+  // that's what we're doing right now.
> 
> I looked up the history of the original code in RasterImage::CreateDecoder.
> It seems to be a bug introduced by
> http://hg.mozilla.org/mozilla-central/rev/08965492c770 where we changed
> |bool aDoSizeDecode| to |const Maybe<nsIntSize>& aSize| and |if
> (!aDoSizeDecode)| to |if (!aSize)|. Can we just fix this in a patch that
> lands before this stack and then do the proper behaviour in this page?

Sure, I'll write up a patch now.
Assignee

Updated

4 years ago
Depends on: 1186667
Assignee

Comment 14

4 years ago
This version of part 2 is rebased against bug 1186667. Practically speaking,
what's changed is that the telemetry code for
IMAGE_DECODE_COUNT/IMAGE_MAX_DECODE_COUNT has moved from DecodeMetadata() to
Decode().
Assignee

Updated

4 years ago
Attachment #8636394 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Blocks: 1187386
You need to log in before you can comment on or make changes to this bug.