Simplify image decoder initialization

RESOLVED INACTIVE

Status

()

Core
ImageLib
RESOLVED INACTIVE
2 years ago
2 days ago

People

(Reporter: seth, Assigned: seth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

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

Comment 3

2 years ago
Created attachment 8784980 [details] [diff] [review]
(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.
Attachment #8784980 - Flags: review?(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)
Attachment #8784972 - Flags: review?(edwin) → review+

Comment 6

2 days ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Last Resolved: 2 days ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.