905468 - Make imgStatusTracker::StatusDiff and friends cleaner and more orthogonal

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

The following pieces of code are currently quite coupled and hard to modify:

- imgStatusTracker::StatusDiff
- imgStatusTracker::CalculateAndApplyDifference
- imgStatusTracker::SyncNotifyDifference
- imgStatusTracker::SyncNotifyDecodeState

We need a clean and rational interface to fix problems like bug 867183 (which I've already got a patch for, but doing this stuff with the current interface is just a mess). The requirements are as follows:

- StatusDiff should encapsulate the _entire_ diff between the imgStatusTrackers.
- It should be possible to combine two StatusDiffs, so we can accumulate diffs if we can't safely notify.
- CalculateAndApplyDifference must become ComputeDifference and ApplyDifference, again making it possible to have finer control over when state changes happen.
- We need ComputeState, which captures the current state of an imgStatusTracker. This is essentially ComputeDifference without the other tracker as an argument.
- With ComputeState in hand, SyncNotifyDecodeState becomes a special case of SyncNotifyDifference. SyncNotifyDecodeState is then eliminated.

This is not strictly a requirement, but I will also take this opportunity to move StatusDiff out of the imgStatusTracker class so that it sits in the mozilla::image namespace and clean it up a little.

Comment 1

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

Attachment: (Part 1) - Make imgStatusTracker state diffing a cleaner process.

This part takes care of the first 3 bullet points above: StatusDiff now encapsulates the _entire_ diff, it's possible to combine two StatusDiffs, and CalculateAndApplyDifference is split into two separate functions. I've also done some housecleaning and renamed StatusDiff to ImageStatusDiff.

Comment 2

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

Try job for just part 1:

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

Comment 3

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

Attachment: (Part 2) - Replace SyncNotifyDecodeState with ComputeState followed by SyncNotifyDifference.

This part takes care of the remainder of the bullet points above; it implements ComputeState, and eliminates SyncNotifyDifference.

Comment 4

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

Try job for part 1 and 2 together.

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

Comment 5

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

Attachment: (Part 1) - Make imgStatusTracker state diffing a cleaner process.

This fixes all of the oranges I was able to reproduce locally. New try job here:

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

Comment 6

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

Attachment: (Part 2) - Replace SyncNotifyDecodeState with ComputeState followed by SyncNotifyDifference.

This version of part 2 is rebased against the new version of part 2 and fixes a minor bug in ComputeState.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=9504a2446409

Comment 7

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

Attachment: (Part 2) - Replace SyncNotifyDecodeState with ComputeState followed by SyncNotifyDifference.

Sigh. Fixed that compilation issue in the wrong patch.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=1d600eeca3ba

Comment 8

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

Part 1 seems alright on try right now. Part 2, on the other hand, is producing a lot of oranges due to assertions about extra UnblockOnload calls. Will investigate further.

Comment 9

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

Attachment: (Part 1) - Make imgStatusTracker state diffing a cleaner process.

Changed the name of ComputeDifference to Difference to match terminology changes made in the new version of part 2.

Comment 10

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

Attachment: (Part 2) - Replace SyncNotifyDecodeState with DecodeStateAsDifference.

This version of part 2 should fix the assertions. In the process of making the fix I realized that the names I had chosen for these functions were misleading, so I renamed things a bit.

Comment 11

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

New try job here:

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

Comment 12

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

Attachment: (Part 1) - Make imgStatusTracker state diffing a cleaner process.

Rename HasNoChange to IsNoChange. Update commit message.

Josh, can you review this?

Comment 13

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

Attachment: (Part 2) - Replace SyncNotifyDecodeState with DecodeStateAsDifference.

Rebase part 2 and update commit message.

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 792504 [details] [diff] [review]
(Part 1) - Make imgStatusTracker state diffing a cleaner process.

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

::: image/src/imgStatusTracker.cpp
@@ +548,5 @@
>  
>    // Record the invalid rectangles and reset them for another go.
>    if (doInvalidations) {
> +    diff.invalidRect = aOther->mInvalidRect;
> +    aOther->mInvalidRect.SetEmpty();

Leave a comment about how this is undesirable? Possibly even file a bug about it?

::: image/src/imgStatusTracker.h
@@ +43,5 @@
> +  { }
> +
> +  static ImageStatusDiff NoChange() { return ImageStatusDiff(); }
> +
> +  bool IsNoChange() const {

Could this just just be |*this == NoChange()|? That would avoid any problems if we ever add more fields.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 792505 [details] [diff] [review]
(Part 2) - Replace SyncNotifyDecodeState with DecodeStateAsDifference.

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

Nice.

Comment 16

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

Thanks for the review, Josh! Those are great suggestions; will upload an updated version of part 1 shortly.

Comment 17

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

Attachment: (Part 1) - Make imgStatusTracker state diffing a cleaner process.

Addressed review comments.

Comment 18

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

Landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/48c7132a8547
https://hg.mozilla.org/integration/mozilla-inbound/rev/995407bc149a

Comment 19

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

Comment on attachment 796921 [details] [diff] [review]
(Part 1) - Make imgStatusTracker state diffing a cleaner process.

(This patch is required to fix the issue in bug 867183; the answers to these questions really apply to the request there.)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 716140 (Off-main-thread image decode.)
User impact if declined: Nondeterministic image load failures. The same root cause may also be responsible for other regressions caused by 716140.
Testing completed (on m-c, etc.): Landing on m-c today. Plenty of testing on try beforehand.
Risk to taking this patch (and alternatives if risky): Minor. The alternative is to not take the patch and accept that there may be occasional issues with image loading.
String or IDL/UUID changes made by this patch: None

Comment 20

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

Comment on attachment 792505 [details] [diff] [review]
(Part 2) - Replace SyncNotifyDecodeState with DecodeStateAsDifference.

(This patch is required to fix the issue in bug 867183; the answers to these questions really apply to the request there.)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 716140 (Off-main-thread image decode.)
User impact if declined: Nondeterministic image load failures. The same root cause may also be responsible for other regressions caused by 716140.
Testing completed (on m-c, etc.): Landing on m-c today. Plenty of testing on try beforehand.
Risk to taking this patch (and alternatives if risky): Minor. The alternative is to not take the patch and accept that there may be occasional issues with image loading.
String or IDL/UUID changes made by this patch: None

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48c7132a8547
https://hg.mozilla.org/mozilla-central/rev/995407bc149a

Comment 22

[bhavana bajaj [:bajaj]]

Seth, this looks ok to land on aurora given we still have time to test it out and deal with regressions if any.

I am concerned to get this on beta as the existing behavior has been this way since Fx22 so not sure why the rush and we only have a couple more beta cycle's to discover fallout's and may not have enough time to deal with it. Please feel free to renominate for beta if anything was interpreted wrong or get in touch with me on irc if you think this should get into today's beta.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21545bbe54a8
https://hg.mozilla.org/releases/mozilla-aurora/rev/bfc9ae46d06e