1282259 - Make ImageLib's DecodePool run IDecodingTasks instead of treating Decoders as tasks directly

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Right now DecodePool is a task pool in which the only possible tasks are Decoder objects. This is bad for two reasons:

(1) We probably eventually want to run other kinds of tasks in the same task pool.

(2) We need to stop treating the Decoders themselves as tasks in order to start allowing other objects to externally control the Decodes' state machines. In particular, this is necessary for streaming playback of animated GIFs and PNGs; if external objects can't control the decoders, we have no way of advancing playback in a controlled fashion to the frame we actually want to display. This means that we want to start moving control and coordination logic out of the Decoders.

To enable this, let's modify DecodePool to run IDecodingTasks instead of Decoders directly. Conveniently, this change will enable us to remove most or all of the non-thread-pool-related code from DecodePool.

I'm trying not to expand this bug's scope beyond what's necessary to accomplish this refactoring. In particular, for now DecoderFactory functions will continue to return Decoder objects. Soon we'll refactor them to return IDecodingTask objects, but changing that now would substantially increase the size of the patch in this bug.

Comment 1

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

Here's a try job for a version of the patch which is complete except for comments and other formatting issues:

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

Comment 2

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

Attachment: Run IDecodingTasks instead of Decoders directly in image::DecodePool.

Here's the patch. It should be pretty straightforward.

One note: this patch makes us do some extra allocation in that we now have an
IDecodingTask wrapping each Decoder, and they're allocated separately. A patch
in the very near future will make the IDecodingTasks store the Decoder objects
as members, so the extra allocation will go away.

Comment 3

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

Attachment: Run IDecodingTasks instead of Decoders directly in image::DecodePool.

A small tweak: all pointers in the patch have been modified to use NotNull. I'm
trying to gradually introduce NotNull into ImageLib.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8765300 [details] [diff] [review]
Run IDecodingTasks instead of Decoders directly in image::DecodePool.

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

r=me

::: image/Decoder.h
@@ +40,5 @@
>    /**
>     * Decodes, reading all data currently available in the SourceBuffer.
>     *
>     * If more data is needed, Decode() will schedule @aOnResume to be called when
> +   * more data is available.

In the documentation here, it's probably worth mentioning the fact that |aOnResume| must be non-null. (That's a new requirement that you're introducing in this patch.)

::: image/IDecodingTask.cpp
@@ +35,5 @@
> +
> +  // We're forced to notify asynchronously.
> +  NotNull<RefPtr<Decoder>> decoder = aDecoder;
> +  NS_DispatchToMainThread(NS_NewRunnableFunction([=]() -> void {
> +    decoder->GetImage()->NotifyProgress(decoder->TakeProgress(),

I'm not familiar enough with C++ lambdas to be confident that this code sufficiently refcounts |decoder| such that it's alive inside of the async runnable here.  (And as I type this, I'm currently on a plane, so I can't do further research at the moment. :) )

I trust your familiarity with newish C++ features, so for now I'll take it for granted that you've investigated this sufficiently to be 100% confident that the refcounting is kosher here. If that's not the case, please say so. :)  (Same goes for the FinalizeDecoder lambda a bit further down.)

@@ +77,5 @@
> +
> +DecodingTask::DecodingTask(NotNull<Decoder*> aDecoder)
> +  : mDecoder(aDecoder)
> +{
> +  MOZ_ASSERT(!mDecoder->IsMetadataDecode());

This assert is a bit enigmatic, without a bit more context.

Consider adding an one-liner message here to make the assertion's point clearer; e.g. "Metadata decodes should use MetadataDecodingTask, not DecodingTask"

@@ +113,5 @@
> +
> +MetadataDecodingTask::MetadataDecodingTask(NotNull<Decoder*> aDecoder)
> +  : mDecoder(aDecoder)
> +{
> +  MOZ_ASSERT(mDecoder->IsMetadataDecode());

...and maybe worth adding a message here as well, for symmetry with the suggested message above?  e.g. "Only metadata decodes should use  MetadataDecodingTask"

(admittedly, this assert is a bit more self-evident than the other one, based on this class's name. But seems worth documenting for symmetry/completeness.)

@@ +139,5 @@
> +///////////////////////////////////////////////////////////////////////////////
> +
> +AnonymousDecodingTask::AnonymousDecodingTask(NotNull<Decoder*> aDecoder)
> +  : mDecoder(aDecoder)
> +{ }

Nit on "{ }" -- I think Gecko coding style requires that the curly braces be on their own lines here (in a .cpp file), like so:
{
}

::: image/IDecodingTask.h
@@ +20,5 @@
> +namespace image {
> +
> +class Decoder;
> +
> +/// A priority hint DecodePool can use when scheduling an IDecodingTask.

Nit: s/hint/hint that/

(I stumbled over this the first time I read it; it initially sounds like "A priority hint DecodePool" might be a noun phrase of its own, until you get to the latter part of the sentence.  If you add "that", the correct reading is more self-evident.)

@@ +39,5 @@
> +
> +  /// @return true if, given the option, this task prefers to run synchronously.
> +  virtual bool ShouldPreferSyncRun() const = 0;
> +
> +  /// @return a priority hint DecodePool can use when scheduling this task.

As above: s/hint/hint that/  (to make this visually parse a bit more clearly).

::: image/ImageOps.cpp
@@ +127,5 @@
>    }
>  
>    // Run the decoder synchronously.
> +  RefPtr<IDecodingTask> task = new AnonymousDecodingTask(WrapNotNull(decoder));
> +  task->Run();

Please add an #include for IDecodingTask.h to this file, to legitimize the usages of these "Task" types here. (Otherwise, this fails to compile, if you switch this file from being unified to non-unified -- and that means this is a unified-bustage timebomb.)

Comment 5

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

Attachment: Run IDecodingTasks instead of Decoders directly in image::DecodePool.

Thanks for the thorough review! Here's an updated version of the patch.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d51379e5475d
Run IDecodingTasks instead of Decoders directly in image::DecodePool. r=dholbert

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d51379e5475d