Closed Bug 905468 Opened 6 years ago Closed 6 years ago

Make imgStatusTracker::StatusDiff and friends cleaner and more orthogonal

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- affected
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 8 obsolete files)

5.41 KB, patch
jdm
: review+
Details | Diff | Splinter Review
15.75 KB, patch
Details | Diff | Splinter Review
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.
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.
Attachment #791066 - Flags: review?(tnikkel)
This part takes care of the remainder of the bullet points above; it implements ComputeState, and eliminates SyncNotifyDifference.
Attachment #791069 - Flags: review?(tnikkel)
Try job for part 1 and 2 together.

https://tbpl.mozilla.org/?tree=Try&rev=12ec3d9eba0a
This fixes all of the oranges I was able to reproduce locally. New try job here:

https://tbpl.mozilla.org/?tree=Try&rev=158a5d6089aa
Attachment #791066 - Attachment is obsolete: true
Attachment #791066 - Flags: review?(tnikkel)
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
Attachment #791069 - Attachment is obsolete: true
Attachment #791069 - Flags: review?(tnikkel)
Sigh. Fixed that compilation issue in the wrong patch.

New try job here: https://tbpl.mozilla.org/?tree=Try&rev=1d600eeca3ba
Attachment #791586 - Attachment is obsolete: true
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.
Changed the name of ComputeDifference to Difference to match terminology changes made in the new version of part 2.
Attachment #791486 - Attachment is obsolete: true
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.
Attachment #791619 - Attachment is obsolete: true
Rename HasNoChange to IsNoChange. Update commit message.

Josh, can you review this?
Attachment #792504 - Flags: review?(josh)
Attachment #791761 - Attachment is obsolete: true
Rebase part 2 and update commit message.
Attachment #792505 - Flags: review?(josh)
Attachment #791762 - Attachment is obsolete: true
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.
Attachment #792504 - Flags: review?(josh) → review+
Comment on attachment 792505 [details] [diff] [review]
(Part 2) - Replace SyncNotifyDecodeState with DecodeStateAsDifference.

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

Nice.
Attachment #792505 - Flags: review?(josh) → review+
Thanks for the review, Josh! Those are great suggestions; will upload an updated version of part 1 shortly.
Blocks: 910441
Attachment #792504 - Attachment is obsolete: true
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
Attachment #796921 - Flags: approval-mozilla-beta?
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
Attachment #792505 - Flags: approval-mozilla-beta?
Attachment #792505 - Flags: approval-mozilla-aurora?
Attachment #796921 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/48c7132a8547
https://hg.mozilla.org/mozilla-central/rev/995407bc149a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Attachment #792505 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #796921 - Flags: approval-mozilla-beta?
Attachment #796921 - Flags: approval-mozilla-beta-
Attachment #796921 - Flags: approval-mozilla-aurora?
Attachment #796921 - Flags: approval-mozilla-aurora+
Attachment #792505 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Whiteboard: [qa-]
Depends on: 922613
You need to log in before you can comment on or make changes to this bug.