Allow image decoders to run without an associated RasterImage

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(7 attachments, 2 obsolete attachments)

5.61 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.17 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.40 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.20 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
13.64 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.99 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
19.73 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
RasterImage is unfortunately a main-thread-only class. (Though certain methods can be called off-main-thread.) In bug 1181863 we want to support decoding images totally off-main-thread, from start to finish, with no main thread interaction. To do that, we need to decouple the image decoders from RasterImage.
(Assignee)

Comment 1

3 years ago
Created attachment 8638617 [details] [diff] [review]
(Part 1) - Make most decoder state private

The first thing we need to do is ensure that image decoders aren't touching
Decoder::mImage directly. This patch does that by making virtually all of
Decoder's fields private. This incidentally finally puts all of the boolean
flags together, so I went ahead and made them a bitfield, which should save a
little bit of memory.
Attachment #8638617 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Depends on: 1184996
(Assignee)

Comment 2

3 years ago
Created attachment 8638621 [details] [diff] [review]
(Part 2) - Rework decoder code to avoid calling Decode::GetImage()

Now that all uses of Decoder's RasterImage reference are called out by
GetImage() calls in the image decoder subclasses, we need to eliminate those
calls. The only tricky case is #-moz-samplesize; we now need to pass the
requested sample size through DecoderFactory to the decoder, like other decoder
parameters.
Attachment #8638621 - Flags: review?(tnikkel)
(Assignee)

Comment 3

3 years ago
Created attachment 8638623 [details] [diff] [review]
(Part 3) - Don't destroy Decoder::mImage if Decoder::mImage is null

We go through a lot of fuss to release Decoder::mImage on the main thread which
isn't necessary if Decoder::mImage is null, so let's check for that case.
Attachment #8638623 - Flags: review?(tnikkel)
(Assignee)

Comment 4

3 years ago
Created attachment 8638626 [details] [diff] [review]
(Part 4) - Make imgFrame::SetOptimizable() callable from off-main-thread

There's no particular reason why we shouldn't call imgFrame::SetOptimizable()
off-main-thread, since we only read imgFrame::mOptimizable with the monitor
held. So let's start doing that.
Attachment #8638626 - Flags: review?(tnikkel)
(Assignee)

Comment 5

3 years ago
Created attachment 8638629 [details] [diff] [review]
(Part 5) - Merge Decoder::SetSizeOnImage() into ImageMetadata::SetOnImage()

It has always seemed a bit odd to me that we don't set the size on the image in
the same code that sets the rest of the ImageMetadata. We do this mainly because
we want to call PostResizeError() if it fails. I'm pretty sure that's completely
unnecessary, but I'm going to avoid changing that in this bug. =) So, at least
as a temporary thing, ImageMetadata::SetOnImage() now returns an nsresult.
Attachment #8638629 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Blocks: 1187401
(Assignee)

Comment 6

3 years ago
Created attachment 8638641 [details] [diff] [review]
(Part 6) - Merge Decoder::Finish() and RasterImage::OnDecodingComplete() into RasterImage::FinalizeDecoder()

Alright, this is the big one. Everything left in Decoder::Finish() is stuff
that's only needed if we have a RasterImage, so it's time to just let
RasterImage take care of that work itself.

The nice thing is that this lets us fold no fewer than *three* methods that
clean up a Decoder - Decoder::Finish(), RasterImage::OnDecodingComplete(), and
RasterImage::FinalizeDecoder() - into one, which feels a lot cleaner.

I took pains to preserve the existing behavior here, even at the cost of a bit
of uglification. In particular, I introduced Decoder::GetDecodeTotallyDone() out
of paranoia, as I didn't want to subtlely change when we would set
RasterImage::mHasBeenDecoded, which using the existing Decoder::GetDecodeDone()
has the potential to do. I want to remove that method ASAP in a followup which I
just filed, bug 1187401, but there's a risk of changing behavior there and I
want to avoid that if possible in a refactoring bug.
Attachment #8638641 - Flags: review?(tnikkel)
(Assignee)

Comment 7

3 years ago
Created attachment 8638651 [details] [diff] [review]
(Part 7) - Eliminate remaining dependencies on a non-null mImage in Decoder

Alright, there are just a couple of dependencies on a non-null mImage left.
Without an associated Image, we can't store surfaces in the SurfaceCache, as we
can't construct a unique key. We don't need to do that anyway if we don't have
an associated Image, so that's fine; this patch makes us just skip over the
SurfaceCache-related code in that case. We also don't need to send
OnAddedFrame() notifications without an Image, so we skip that too.

You might ask yourself: "Sure, we can retrieve the *last* frame that got decoded
without going through the SurfaceCache, by calling Decoder::GetCurrentFrame().
But won't we want the *first* frame in the case of animated images?" Yes, we
will. The last step I'll take before tackling bug 1181863 itself is to add a new
mode for decoders where they will decode only the first frame of an image, even
if the image is animated. That way the first frame will be the last frame, and
all will be well. It turns out that we'll need such a mode anyway if we start
playing animated images through the media framework, because we'll still need to
decode the first frame of such images in the traditional way for cases like
|canvas.drawImage()|, so it's doubly useful.
Attachment #8638651 - Flags: review?(tnikkel)
Attachment #8638617 - Flags: review?(tnikkel) → review+
Attachment #8638621 - Flags: review?(tnikkel) → review+
Attachment #8638623 - Flags: review?(tnikkel) → review+
Attachment #8638626 - Flags: review?(tnikkel) → review+
Attachment #8638629 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 9

3 years ago
Thanks for these reviews!

Based on those try results, it looks like I managed to break #-moz-samplesize in part 2. Other than that things look fine.
(Assignee)

Comment 10

3 years ago
Created attachment 8638795 [details] [diff] [review]
(Part 2) - Rework decoder code to avoid calling Decode::GetImage()

OK, fixed the issue with #-moz-samplesize. There were two bugs:

1. I swapped two of the arguments to CreateDecoder() by accident, and since
   int's and bool's are implicitly convertible to each other, I didn't get a
   warning about this from the compiler.

2. I realized we also need to pass the sample size to the metadata decoder to
   avoid an assertion that the image's size changed when we do the full decode.

Both of those issues are fixed in this version of the patch, which should
resolve all of the oranges on try. I won't actually push a new try job until the
last two parts are reviewed, though, in case there are review changes.
(Assignee)

Updated

3 years ago
Attachment #8638621 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
Created attachment 8640013 [details] [diff] [review]
(Part 2) - Rework decoder code to avoid calling Decode::GetImage()

I noticed one more issue while writing the tests for bug 1181863: nsICODecoder
calls RasterImage::GetRequestedResolution() as part of the implementation of
#-moz-resolution. Since nsICODecoder is, unfortunately, still a friend of
Decoder, the compiler didn't catch this case. I've fixed it by passing the
requested resolution through DecoderFactory, in the same way that I handled
#-moz-samplesize.

With this change, the tests in bug 1181863 pass, and everything looks good.
(Assignee)

Updated

3 years ago
Attachment #8638795 - Attachment is obsolete: true
Comment on attachment 8638641 [details] [diff] [review]
(Part 6) - Merge Decoder::Finish() and RasterImage::OnDecodingComplete() into RasterImage::FinalizeDecoder()

GetDecodeTotallyDone seems a little confusingly named. Perhaps GetFullDecodeDone?
Attachment #8638641 - Flags: review?(tnikkel) → review+
Attachment #8638651 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 13

3 years ago
Thanks for the reviews!

(In reply to Timothy Nikkel (:tn) from comment #12)
> Comment on attachment 8638641 [details] [diff] [review]
> (Part 6) - Merge Decoder::Finish() and RasterImage::OnDecodingComplete()
> into RasterImage::FinalizeDecoder()
> 
> GetDecodeTotallyDone seems a little confusingly named. Perhaps
> GetFullDecodeDone?

The only problem is that a "full decode" is what I've been calling non-metadata decodes. =\

It's hard to name this method because it shouldn't exist. I'll upload a patch to rip it out next week.
You need to log in before you can comment on or make changes to this bug.