Seth Fowler [:seth] [:s2h] Assignee
	Description • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 1 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 2 • 11 years ago

Try job for just part 1:

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 3 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 4 • 11 years ago

Try job for part 1 and 2 together.

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 5 • 11 years ago

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

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 6 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 7 • 11 years ago

Sigh. Fixed that compilation issue in the wrong patch.

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 8 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 9 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 10 • 11 years ago

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.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 11 • 11 years ago

New try job here:

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 12 • 11 years ago

Rename HasNoChange to IsNoChange. Update commit message.

Josh, can you review this?

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 13 • 11 years ago

Rebase part 2 and update commit message.

	Josh Matthews [:jdm]
	Comment 14 • 11 years ago

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.

	Josh Matthews [:jdm]
	Comment 15 • 11 years ago

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

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

Nice.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 16 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 17 • 11 years ago

Addressed review comments.

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 18 • 11 years ago

Landed:

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 19 • 11 years ago

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

	Seth Fowler [:seth] [:s2h] Assignee
	Comment 20 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

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

	bhavana bajaj [:bajaj]
	Comment 22 • 11 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 11 years ago

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