1298145 - Simplify image decoder initialization

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

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

Right now image decoders have this weird initialization model where we construct a Decoder object, call a bunch of setters (some of which are non-optional or effectively non-optional), and then call Init(). If we accidentally forget to call some of the setters, it breaks stuff. If we accidentally call one of these setters after calling Init(), it breaks stuff. This whole situation is actually one of the reasons DecoderFactory was originally created: initializing decoders is very fragile.

We've now de-crufted Decoder enough to where we can clean this stuff up, and I think it makes sense to do so, as it constitutes a major de-crufting in its own right.

Comment 1

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

Attachment: (Part 1) - Accept a SourceBufferIterator in Decoder's constructor.

OK, step one is to pass a SourceBufferIterator to Decoder's constructor, because
we always have to have one, and since they're move-only, stack-based objects,
passing them to a setter or to Init() requires that we store them in a Maybe,
which in turn results in a bunch of assertions that we really shouldn't need.

Comment 2

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

Attachment: (Part 2) - Pass Decoder initialization information to Decoder::Init().

This patch consolidates every setters except for SetSampleSize(), which I'm
inclined to keep separate since it's slated for removal soon and it's specific
to the JPEG decoder. The things that we used to pass to the setters are now all
passed to Init(). There's also a variant, InitForMetadataDecode(), which accepts
only the parameters that a metadata decoder needs.

Comment 3

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

Attachment: (Part 3) - Get rid of Decoder::DecodeStyle.

The JPEG decoder is all sorts of special. Beyond SetSampleSize(), it also
accepts a DecodeStyle argument to its constructor which tells it whether to do a
progressive decode or not. Other decodes make similar decisions based upon
whether DecoderFlags::IS_REDECODE is set, and indeed that's what DecoderFactory
checks when deciding which DecodeStyle to pass to nsJPEGDecoder's constructor.
Since all decoders have access to their DecoderFlags already, let's just check
this value directly in nsJPEGDecoder and get rid of DecodeStyle totally.

Comment 4

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

Try job here:

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

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8784972 [details] [diff] [review]
(Part 1) - Accept a SourceBufferIterator in Decoder's constructor.

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

::: image/Decoder.cpp
@@ +98,5 @@
>    {
>      PROFILER_LABEL("ImageDecoder", "Decode", js::ProfileEntry::Category::GRAPHICS);
>      AutoRecordDecoderTelemetry telemetry(this);
>  
> +    lexerResult =  DoDecode(mIterator, aOnResume);

nit: remove extra space after '='

(which was already there, but it's worth fixing while we're here)

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.