1089046 - Remove imgDecoderObserver

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

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.

Comment 1

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

Attachment: (Part 1) - Remove imgDecoderObserver and related code

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.

Comment 2

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

Attachment: (Part 2) - Remove guards against recursive notifications

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.

Comment 3

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

Here's a try job:

https://tbpl.mozilla.org/?tree=Try&rev=d6808e98f534

Comment 4

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

Attachment: (Part 1) - Remove imgDecoderObserver and related code

I removed a little bit more apparently useless code in this version: imgStatusTracker::DecodeStateAsDifference.

Comment 5

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

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

Comment 6

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

Sigh. Had to repush that one.

https://tbpl.mozilla.org/?tree=Try&rev=d850ae4e6fd1

Comment 7

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

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

Comment 8

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

So it looks to me like that try job came back green. Nice! These patches should be ready to go too, then, modulo review.

Comment 9

[Timothy Nikkel (:tnikkel)]

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 10

[Timothy Nikkel (:tnikkel)]

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

r+ if you agree about comment 9.

Comment 11

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

(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!

Comment 12

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

Attachment: (Part 2) - Remove guards against recursive notifications

Addressed review comments.

Comment 13

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

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

Comment 14

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

OK, try looks good. Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b892bd18e0d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d3440cd34b

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b892bd18e0d9
https://hg.mozilla.org/mozilla-central/rev/02d3440cd34b