Closed
Bug 1184996
Opened 7 years ago
Closed 7 years ago
Create decoders with a DecoderFactory
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(4 files, 3 obsolete files)
26.62 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
10.04 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
20.87 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
16.35 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•7 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•7 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•7 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•7 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•7 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 | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=782bb88fb13d
Assignee | ||
Comment 7•7 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•7 years ago
|
Attachment #8636362 -
Attachment is obsolete: true
Attachment #8636362 -
Flags: review?(tnikkel)
Assignee | ||
Comment 8•7 years ago
|
||
Part 2 needed a rebase against the new version of part 1.
Attachment #8636394 -
Flags: review?(tnikkel)
Assignee | ||
Updated•7 years ago
|
Attachment #8636366 -
Attachment is obsolete: true
Attachment #8636366 -
Flags: review?(tnikkel)
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2f5130ead62
Assignee | ||
Comment 10•7 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 | ||
Comment 11•7 years ago
|
||
Bug 1186112 should resolve the conflicts with windows.h - not just for this patch, but permanently.
Updated•7 years ago
|
Attachment #8636393 -
Flags: review?(tnikkel) → review+
Comment 12•7 years ago
|
||
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?
Updated•7 years ago
|
Attachment #8636394 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8636368 -
Flags: review?(tnikkel) → review+
Updated•7 years ago
|
Attachment #8636370 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 13•7 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 | ||
Comment 14•7 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 | ||
Comment 15•7 years ago
|
||
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•7 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Updated•7 years ago
|
Attachment #8636394 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•