Closed Bug 1089046 Opened 10 years ago Closed 10 years ago

Remove imgDecoderObserver

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

The next step in bringing sanity to imgStatusTracker is removing imgDecoderObserver. We don't need it, because:

- In non-decoder code we can inform imgStatusTracker of changes directly.
- In decoder code, the entire model we have now with decoders sending updates to a cloned imgStatusTracker via the imgDecoderObserver interface is extremely difficult to reason about, and it adds no value since we can just record an ImageStatusDiff and an invalidation rect directly in the decoder.

The shortest path to getting multiple decoders for a single RasterImage working is to remove imgDecoderObserver. It will also help make the code cleaner and easier to understand, especially once the other refactoring that it enables gets done in followup bugs. Let's do it.
Blocks: 910441
No longer depends on: 910441
Blocks: 1089880
Depends on: 1091229
Here we go. This patch removes imgDecoderObserver and also rips out imgStatusTracker code that was trivial to remove once imgDecoderObserver is gone. I avoided ripping out anything that was complicated to remove. Things will continue to get cleaner in the following patches.
Attachment #8514506 - Flags: review?(tnikkel)
Slightly different notification timing caused by part 1 made us fail some reftests because Draw failed to sync decode. This happened because |mInDecoder| was true. However, that's not actually a problem anymore, since there's no way we can trigger notifications from Decoder::Write or Decoder::Finish. (Indeed, it's not clear this was a problem even before part 1, but it's 100% clear that it's not a problem now.) This patches therefore removes |mInDecoder| totally.

Only removing |mInDecoder| is actually required to pass the tests, but none of the old recursive notification guards are useful at this point, so this patch removes them all.
Attachment #8514514 - Flags: review?(tnikkel)
Depends on: 1091921
I removed a little bit more apparently useless code in this version: imgStatusTracker::DecodeStateAsDifference.
Attachment #8514723 - Flags: review?(tnikkel)
Attachment #8514506 - Attachment is obsolete: true
Attachment #8514506 - Flags: review?(tnikkel)
Here's a try job that both checks the new version of part 1 and verifies that Windows builds are now fixed:

https://tbpl.mozilla.org/?tree=Try&rev=386d8e3a53c0
Sigh. Had to repush that one.

https://tbpl.mozilla.org/?tree=Try&rev=d850ae4e6fd1
Blocks: 1063973
OK, I *think* that the issues with patches lower in the stack have been resolved, so I think it's worth pushing this to try again. Nothing has changed with the patches in this bug.

https://tbpl.mozilla.org/?tree=Try&rev=c27c1e674ff7
So it looks to me like that try job came back green. Nice! These patches should be ready to go too, then, modulo review.
No longer blocks: 1089880
Attachment #8514723 - Flags: review?(tnikkel) → review+
Comment on attachment 8514514 [details] [diff] [review]
(Part 2) - Remove guards against recursive notifications

>@@ -2586,19 +2541,21 @@ RasterImage::Draw(gfxContext* aContext,
>-  // Disallowed in the API
>-  if (mInDecoder && (aFlags & imgIContainer::FLAG_SYNC_DECODE))
>-    return NS_ERROR_FAILURE;
>+  Maybe<ReentrantMonitorAutoEnter> maybeLock;
>+  if (aFlags & imgIContainer::FLAG_SYNC_DECODE) {
>+    // Acquire the monitor, which ensures that we're able to sync decode.
>+    maybeLock.emplace(mDecodingMonitor);
>+  }

Why is it necessary to enter the monitor here? SyncDecode() does that already.

Otherwise looks good.
Comment on attachment 8514514 [details] [diff] [review]
(Part 2) - Remove guards against recursive notifications

r+ if you agree about comment 9.
Attachment #8514514 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #9)
> Why is it necessary to enter the monitor here? SyncDecode() does that
> already.

On reflection, this was excessive paranoia on my part. I'll remove that code.

Thanks for the review!
Addressed review comments.
Attachment #8514514 - Attachment is obsolete: true
Since this had to be rebased and got changed (for comment 9) I think it'd be wise to push another try job:

https://tbpl.mozilla.org/?tree=Try&rev=69982de69e07
https://hg.mozilla.org/mozilla-central/rev/b892bd18e0d9
https://hg.mozilla.org/mozilla-central/rev/02d3440cd34b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1100725
Depends on: 854506
Blocks: 854506
No longer depends on: 854506
You need to log in before you can comment on or make changes to this bug.