Closed Bug 1126146 Opened 5 years ago Closed 5 years ago

Disable the single-color optimization by default in reftests

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 1 obsolete file)

We should disable the single-color optimization in reftests. Why?

- It can cause intermittent oranges because there can be a delay between when the image is decoded and when the optimization is applied, since we can only cause imgFrame::Optimize on the main thread. This can mean that if the same image is used in the test and the reference of a reftest, we may use the single-color optimization code path in one case and not in the other, which may cause small rendering differences.

- Even if that weren't true, the fact is that most real images on the web are not single-color images, but most of our reftests use single-color images. We should be testing the same code path that real images on the web use.

Right now it's unclear where we want to keep the single-color optimization around at all. I'm going to add telemetry that will tell us how often it gets used; if it's very rarely helpful on the modern web, we'll just remove the code. If we decide to keep it, we'll want to add new tests that specifically test it, and we can enable the appropriate pref only for those tests. Since we don't know yet which of these things we're going to do, I'm not going to add any single-color optimization tests in this bug.
Here's the patch. We had to add a fuzzy annotation to a couple of tests, but
only on OS X. I'd guess that this actually affects far more tests, but most of
the affected tests have already gotten some fuzz.
Attachment #8555021 - Flags: review?(tnikkel)
Daniel, the only test I had to add a fairly large fuzz factor to was |image-object-position-with-background-1.html|. Should we file a followup to investigate that?
Flags: needinfo?(dholbert)
(In reply to Seth Fowler [:seth] from comment #2)
> Daniel, the only test I had to add a fairly large fuzz factor to was
> |image-object-position-with-background-1.html|. Should we file a followup to
> investigate that?

I'd be interested to see what the failure looks like, if you have an earlier try job with reftest snapshots.

If it's just blurriness, I suspect we could work around it by adjusting image-object-position-with-background-1-ref.html to render its blue PNG with a 5px of top/left margin in a 32-by-32 overflow:hidden div. (instead of rendering as a 27-by-27 region, with 5px of padding)  That way, both the testcase and the reference case should be rendering the blue image to the same size region and clipping.

We (or I) can definitely take care of that in a followup, if it's the sort of failure-mode that I'm imagining, so feel free to spin one off & tag me.
Flags: needinfo?(dholbert)
Comment on attachment 8555021 [details] [diff] [review]
Disable the single-color optimization in reftests

r+ on the code, but I don't feel I can review turning this pref on in reftests.
Attachment #8555021 - Flags: review?(tnikkel)
Attachment #8555021 - Flags: review?(dbaron)
Attachment #8555021 - Flags: review+
(In reply to Daniel Holbert [:dholbert] from comment #4)
> I'd be interested to see what the failure looks like, if you have an earlier
> try job with reftest snapshots.

 Yeah, it's just blurriness. I'll file a followup and put a link to the failure there.
Blocks: 1126163
Blocks: 1126227
+// Should we optimization away the surfaces of single-color images?

Should we optimize away...
As noted in bug 1126163: You shouldn't need to annotate image-object-position-with-background-1.html at all anymore (in layout/reftests/image/reftest.list ), if this patch lands after that bug's patch.
Comment on attachment 8555021 [details] [diff] [review]
Disable the single-color optimization in reftests

ok, although I'm a bit concerned about the fuzzy annotations
Attachment #8555021 - Flags: review?(dbaron) → review+
Thanks for the review! Removed the now-unnecessary fuzzy annotation for
|image-object-position-with-background-1-ref.html| (fixed in bug 1126163) and
the typo that Robert pointed out. Also made one other comment-only tweak.

This should be ready to land now.
Attachment #8555021 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a389cb22000d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8555606 [details] [diff] [review]
Disable the single-color optimization in reftests

Approval Request Comment
[Feature/regressing bug #]: None. This patch makes reftests much more deterministic, reducing intermittent oranges.
[User impact if declined]: None. This is a testing-only change. (Although it does modify source code, it's just a trivial modification to add a new pref.)
[Describe test coverage new/current, TreeHerder]: In 38 for a long time.
[Risks and why]: Low risk; in 38 for a long time.
[String/UUID change made/needed]: None.
Attachment #8555606 - Flags: approval-mozilla-aurora?
This is causing OSX reftest failures on Aurora. Please fix or backout ASAP (go ahead and push on the closed tree if need-be).
Flags: needinfo?(seth)
I just uplifted bug 1126163 to Aurora, which will hopefully solve it. Would have been nice if it had landed alongside this patch in the first place.
Flags: needinfo?(seth)
Comment on attachment 8555606 [details] [diff] [review]
Disable the single-color optimization in reftests

Aurora approval previously granted for a collection of ImageLib related bugs that Seth worked on and QE verified before uplift. Unfortunate about the reftest failures. Glad to see that the fix in bug 1126163 has already landed and I hope that it does indeed fix the failures.
Attachment #8555606 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.