Closed Bug 1079653 Opened 10 years ago Closed 10 years ago

Eliminate the DecodeRequest class from RasterImage and move its contents elsewhere

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 17 obsolete files)

9.19 KB, patch
Details | Diff | Splinter Review
3.77 KB, patch
Details | Diff | Splinter Review
11.22 KB, patch
Details | Diff | Splinter Review
21.53 KB, patch
Details | Diff | Splinter Review
To support multiple decoders at once for a single RasterImage, we need to eliminate state that's tied to a single decoder. A large step in that direction is to eliminate the DecodeRequest class.

Most of the contents of DecodeRequest can live on Decoder instead. There are two exceptions:

(1) mBytesToDecode doesn't actually seem to add much value, so unless there's something I've missed, we can just calculate that when it's needed.

(2) mRequestStatus is still needed for now, so I've moved this onto RasterImage directly as mDecodeStatus. This will be removed in a future bug.
Attachment #8501521 - Flags: review?(tnikkel)
Alright, there we go! That's a big step towards allowing multiple decoders at once. Here's a try job:

https://tbpl.mozilla.org/?tree=Try&rev=7077e20ad84a
Attachment #8501519 - Flags: review?(tnikkel) → review+
Attachment #8501521 - Flags: review?(tnikkel) → review+
Attachment #8501522 - Flags: review?(tnikkel) → review+
Attachment #8501524 - Flags: review?(tnikkel) → review+
This patch needed a change in the order of the fields in the initializer list to avoid triggering "-Werror=reorder".
Attachment #8501519 - Attachment is obsolete: true
It looks like MOZ_BEGIN_ENUM_CLASS expands into something that MSVC and GCC 4.4 don't like seeing nested within another class. I pulled DecodeStatus out of RasterImage.
Attachment #8501556 - Flags: review?(tnikkel)
Attachment #8501525 - Attachment is obsolete: true
Attachment #8501525 - Flags: review?(tnikkel)
Here's a new try job, since between those two issues all the builds failed on the last one:

https://tbpl.mozilla.org/?tree=Try&rev=8d7efcfbf400
The oranges above make me suspect part 2. Here's a try job with just part 1 applied:

https://tbpl.mozilla.org/?tree=Try&rev=63a5a36e4ea0
Here's part 1 and part 2 only:

https://tbpl.mozilla.org/?tree=Try&rev=523ab55d3fb0
OK, so part 1 and 2 appear to actually be clean! In that case, I suspect part 4. Here's a new try job with everything up to part 3:

https://tbpl.mozilla.org/?tree=Try&rev=2d075d4cc44f
Here's everything up to part 4. If part 4 is at fault, I suspect comment 12's try job to be green and this one to be orange.

https://tbpl.mozilla.org/?tree=Try&rev=df28ad6c11cc
Looks like one chunk from part 4 ended up in part 5.
Attachment #8501524 - Attachment is obsolete: true
Here's a new "everything up to part 4" try job:

https://tbpl.mozilla.org/?tree=Try&rev=e8c000824902
Removed the part 4 chunk from this part.
Attachment #8502154 - Flags: review?(tnikkel)
Attachment #8501556 - Attachment is obsolete: true
Attachment #8501556 - Flags: review?(tnikkel)
So part 3 had a bug; we needed to clear the mNeedsNewFrame bit before checking for errors in Decoder::Write. The same kind of issue as in bug 1079628, alas!
Attachment #8501522 - Attachment is obsolete: true
OK, I tracked down the issue. It looks like in some circumstances (for example, in OnImageDataComplete) we currently shut down the decoder before we're ready to call FinishedSomeDecoding and notify. Right now, that works, because the recording imgStatusTracker is stored on mDecodeRequest. If we want to store the recording imgStatusTracker on the decoder, though, that means that it may have disappeared by the time we want to notify!

The solution is pretty simple: keep the decoder, or at least the recording imgStatusTracker, alive long enough to notify correctly. This requires that we keep a reference to the decoder in some of the decode pool helper classes. It so happens that we want this anyway, since eventually we want many decoders and referring to the single decoder stored on the image won't be good enough.

In order to avoid some circular dependency issues I had to move the definition of DecodeJob out of RasterImage and into RasterImage.cpp. This has had the result of making the code in DecodeJob::Run in particular uglier than it was before, since now many types have to be qualified. This will get cleaned up soon; I've already got a patch mostly complete that will move the decode pool and all its helper classes to their own source file and make all this ugliness go away. I think that change is complex enough to deserve its own bug, though.
Attachment #8503007 - Flags: review?(tnikkel)
Attachment #8502151 - Attachment is obsolete: true
Here's a try job for parts 1 through 4, including the updated version of part 4:

https://tbpl.mozilla.org/?tree=Try&rev=0236212f509a
Here's a rebased version of part 5. It gets a little smaller, as small of the changes were effectively replaced by changes in the new part 4.

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=7ffd3f8e96ab
Attachment #8503017 - Flags: review?(tnikkel)
Attachment #8502154 - Attachment is obsolete: true
Attachment #8502154 - Flags: review?(tnikkel)
(In reply to Seth Fowler [:seth] from comment #18)
> I've already got a patch mostly complete that will move the
> decode pool and all its helper classes to their own source file and make all
> this ugliness go away. I think that change is complex enough to deserve its
> own bug, though.

Filed as bug 1081012.
It looks like part 4 needed another tweak: in some circumstances, we may already have shutdown the decoder when we call OnImageDataComplete. This version of the patch uses the recording status tracker from the decoder if it's still around, but otherwise falls back to the real status tracker.

I'm really not happy with how unpredictable the lifetime of the decoder is right now. This should get saner very soon as part of the process of supporting multiple decoders at once, though.

Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=1900a0175a1f
Attachment #8503264 - Flags: review?(tnikkel)
Attachment #8503007 - Attachment is obsolete: true
Attachment #8503007 - Flags: review?(tnikkel)
Blocks: 656351
Alright, after investigating this pretty thoroughly, I've come to the conclusion that making the recording imgStatusTracker per-decoder is more complex than is feasible in this bug. It's going to require some serious changes to how imgStatusTracker is implemented. There's already a bug about fixing imgStatusTracker's design: bug 910441. I'm going to need to fix that bug before I can make the next set of big changes that will move us toward multiple decoders for a single RasterImage.

For now, I think it's still worth getting rid of the DecodeRequest class and generally whittling down the amount of code that blocks multiple decoders. In this much more conservative part 4, we just move DecodeRequest::mStatusTracker to RasterImage::mDecodeStatusTracker. After bug 910441 is taken care of, I should be able to remove mDecodeStatusTracker in a followup.

Here's a try job for the new part 4:

https://tbpl.mozilla.org/?tree=Try&rev=aefd661a0778
Attachment #8504429 - Flags: review?(tnikkel)
OK, here's a rebased version of part 5.

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=25747917d9ed
Attachment #8504433 - Flags: review?(tnikkel)
Attachment #8503017 - Attachment is obsolete: true
Attachment #8503017 - Flags: review?(tnikkel)
Attachment #8503264 - Attachment is obsolete: true
Attachment #8503264 - Flags: review?(tnikkel)
Comment on attachment 8504429 [details] [diff] [review]
(Part 4) - Move the recording imgStatusTracker onto RasterImage

>   struct DecodeRequest
>   {
>-    explicit DecodeRequest(RasterImage* aImage)
>-      : mImage(aImage)
>-      , mRequestStatus(REQUEST_INACTIVE)
>-    {
>-      MOZ_ASSERT(aImage, "aImage cannot be null");
>-      MOZ_ASSERT(aImage->mStatusTracker,
>-                 "aImage should have an imgStatusTracker");
>-      mStatusTracker = aImage->mStatusTracker->CloneForRecording();
>-    }
>+    explicit DecodeRequest()
>+      : mRequestStatus(REQUEST_INACTIVE)
>+    { }
> 
>     NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DecodeRequest)
> 
>-    // The status tracker that is associated with a given decode request, to
>-    // ensure their lifetimes are linked.
>-    nsRefPtr<imgStatusTracker> mStatusTracker;
>-
>     RasterImage* mImage;

Looks like you can get rid of mImage here already. I know you kill DecodeRequest totally in the next patch, so it's not really important.
Attachment #8504429 - Flags: review?(tnikkel) → review+
Attachment #8504433 - Flags: review?(tnikkel) → review+
Backed out for mochitest-dt asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13927f78353

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3030149&repo=mozilla-inbound

I also suspect this is why bug 910533 re-appeared on inbound. It also may be responsible for a spike in bug 956211 (though that happens enough that it could be happenstance too).
This modified version of part 3 avoids flushing in nsJPEGDecoder::FinishInternal, which is what triggered the assert. There were existing 'XXX: why are we doing this?' comments by both me and bholley on that code, so I think this is a good opportunity to remove it. It's not obvious to me that flushing there had any value whatsoever; after all, the decode is already complete.

Regarding the other issues that cropped up on inbound, I haven't been able to reproduce them locally. I'm going to push some more "all" Try jobs to try to figure out which patch introduces these issues. I suspect part 4.
Attachment #8502203 - Attachment is obsolete: true
Here's an "all" Try job that includes part 3 and earlier:

https://tbpl.mozilla.org/?tree=Try&rev=6a28612ee17d
Here's an "all" Try job that also includes part 4:

https://tbpl.mozilla.org/?tree=Try&rev=5031f12d3097
Hmmm. Right now I'm focused on bug 910533, which should appear in Mochitest part 7 on Android 4.0. We're not hitting it in the try jobs above. Could part 5 introduce the problem? I pushed a new try job to look into it:

https://tbpl.mozilla.org/?tree=Try&rev=973d390561ac
Attachment #8504429 - Attachment is obsolete: true
Depends on: 910441
Renumbered part 5 to part 4. We don't need part 4 anymore, because bug 910441 essentially replaces part 4. (And goes much further, in fact; the entire imgStatusTracker mechanism has been completely revamped.)
Attachment #8504433 - Attachment is obsolete: true
Attachment #8501552 - Attachment is obsolete: true
Attachment #8501521 - Attachment is obsolete: true
Attachment #8505855 - Attachment is obsolete: true
Attachment #8524805 - Attachment is obsolete: true
Here's a new try job, which looks green. I think it's time to try pushing this in again.

https://tbpl.mozilla.org/?tree=Try&rev=80fd15a9cb1b
Depends on: 1101759
You need to log in before you can comment on or make changes to this bug.