1184996 - Create decoders with a DecoderFactory

Status: RESOLVED FIXED

Description

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

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.

Comment 1

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

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.

Comment 2

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

Attachment: (Part 1) - Create decoders with a DecoderFactory

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.

Comment 3

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

Attachment: (Part 2) - Clean up RasterImage's decoding API

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.

Comment 4

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

Attachment: (Part 3) - Replace all remaining references to 'size decodes' with 'metadata decodes'

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.

Comment 5

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

Attachment: (Part 4) - Forbid instantiation of decoders except via DecoderFactory

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.

Comment 6

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=782bb88fb13d

Comment 7

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

Attachment: (Part 1) - Create decoders with a DecoderFactory

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.

Comment 8

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

Attachment: (Part 2) - Clean up RasterImage's decoding API

Part 2 needed a rebase against the new version of part 1.

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2f5130ead62

Comment 10

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

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.

Comment 11

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

Bug 1186112 should resolve the conflicts with windows.h - not just for this patch, but permanently.

Comment 12

[Timothy Nikkel (:tnikkel)]

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?

Comment 13

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

(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.

Comment 14

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

Attachment: (Part 2) - Clean up RasterImage's decoding API

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().

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ac5cb9669db
https://hg.mozilla.org/integration/mozilla-inbound/rev/97a30fa80826
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c85da36168b
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaf3459a20b5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ac5cb9669db
https://hg.mozilla.org/mozilla-central/rev/97a30fa80826
https://hg.mozilla.org/mozilla-central/rev/9c85da36168b
https://hg.mozilla.org/mozilla-central/rev/aaf3459a20b5