1291071 - Stop passing a Decoder object to RasterImage::FinalizeDecoder() and rename it appropriately

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

RasterImage::FinalizeDecoder() is used to communicate the final results of the decoding process to RasterImage. However, the fact that it requires a real Decoder object requires some awkward workarounds when errors are detected outside of the Decoder itself. We should rework FinalizeDecoder() so it does not actually take a Decoder object as an argument, just the relevant data, the same way that NotifyProgress() works. Having done that, it'll probably also be appropriate to rename it.

Comment 1

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

Attachment: (Part 1) - Pass ImageMetadata explicitly to FinalizeDecoder.

All of these patches are pretty straightforward; we're just pulling all of the
data that FinalizeDecoder() needs to inspect off of the decoder and passing it
in directly.

This patch handles ImageMetadata.

Comment 2

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

Attachment: (Part 2) - Pass decoder progress explicitly to FinalizeDecoder.

Since we call NotifyProgress() one final time in FinalizeDecoder(), we need to
pass in the same stuff that we pass to NotifyProgress().

Comment 3

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

Attachment: (Part 3) - Pass telemetry explicitly to FinalizeDecoder.

This patch makes us pass in telemetry data explicitly.

Comment 4

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

Attachment: (Part 4) - Clean up Decoder::SpeedHistogram() and related code.

Decoder::SpeedHistogram() doesn't have the best design right now. It's
non-const, which prevented me from marking Decoder::Telemetry() const. It uses
an invalid telemetry ID value to indicate that we don't want to record telemetry
for a particular decoder. And at this point it should really be protected rather
than public. Let's fix all that stuff.

Comment 5

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

Attachment: (Part 5) - Pass the decoder's final status explicitly to FinalizeDecoder().

FinalizeDecoder() inspects what amounts to a few bits of state to determine
whether the decode succeeded and what kind of error handling to perform. This
patch bundles all that state up in a struct.

Comment 6

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

Attachment: (Part 6) - Stop passing a decoder to FinalizeDecoder() and rename it NotifyDecodeComplete().

Now we've eliminated any reason to pass a decoder to FinalizeDecoder(), so we
can stop doing so. Now that it's more of a notification callback like
NotifyProgress(), it makes sense to rename it to match the same pattern, and to
make the function that sends the callback like NotifyProgress() does.

Comment 7

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

Attachment: (Part 7) - Remove dead line of code in NotifyProgress().

I noticed there's a line of dead code in NotifyProgress(); let's remove it.

Comment 8

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

It's worth noting that we'll be able to clean this stuff up more later. In this bug I'm just trying to preserve the same behavior / state / etc that we have now, but without giving RasterImage direct access to the decoder. Once the animated image refactoring stuff lands, there will be some further simplification opportunities.

Comment 9

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

Thanks for the reviews, Edwin!

Comment 10

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

Comment on attachment 8777167 [details] [diff] [review]
(Part 7) - Remove dead line of code in NotifyProgress().

I ended up folding this into bug 1291045.

Comment 11

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

Here's a try job:

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d416df8849a0
(Part 1) - Move an assertion from RasterImage::FinalizeDecoder() to IDecodingTask::NotifyDecodeComplete(). r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce50dc53516
(Part 1) - Pass ImageMetadata explicitly to FinalizeDecoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/801b6b88b490
(Part 2) - Pass decoder progress explicitly to FinalizeDecoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b9a6dca2883
(Part 3) - Pass telemetry explicitly to FinalizeDecoder. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/eef2029cae9d
(Part 4) - Clean up Decoder::SpeedHistogram() and related code. r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/55a26e91a828
(Part 5) - Pass the decoder's final status explicitly to FinalizeDecoder(). r=edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/731d13870f6a
(Part 6) - Stop passing a decoder to FinalizeDecoder() and rename it NotifyDecodeComplete(). r=edwin

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d416df8849a0
https://hg.mozilla.org/mozilla-central/rev/5ce50dc53516
https://hg.mozilla.org/mozilla-central/rev/801b6b88b490
https://hg.mozilla.org/mozilla-central/rev/9b9a6dca2883
https://hg.mozilla.org/mozilla-central/rev/eef2029cae9d
https://hg.mozilla.org/mozilla-central/rev/55a26e91a828
https://hg.mozilla.org/mozilla-central/rev/731d13870f6a