Closed Bug 1189593 Opened 9 years ago Closed 9 years ago

Make sure that SSE2 is available before using it in image::Downscaler

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox39 --- unaffected
firefox40 + fixed
firefox41 + fixed
firefox42 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

(Keywords: regression)

Attachments

(1 file)

Right now we're attempting to run SSE2 code on machines that don't support SSE2 in Downscaler, because we unconditionally pass |true| for the |use_sse2| parameter to skia::ConvolveHorizontally/ConvolveVertically. Somehow, this doesn't crash, it just results in black images. (Which pretty much means that the code doesn't do anything, and we end up with a buffer containing all zeros.)

To do the right thing here, we should pass |true| only if we're running on a machine that supports SSE2. This is easy to implement using mozilla::supports_sse2().
(Note that I've pulled this out of bug 1180317 because it's currently unclear whether there are multiple "black image" bugs. However, I've confirmed that this  fixes the issue for some users.)
Attachment #8641441 - Flags: review?(tnikkel) → review+
Comment on attachment 8641441 [details] [diff] [review]
Only use SSE2 in image::Downscaler if we're running on an SSE2-capable machine

Approval Request Comment
Approval Request Comment
[Feature/regressing bug #]: Bug 1124084.
[User impact if declined]: Users with older machines that don't support SSE2 will see any JPEG image that's downscaled replaced with a black rectangle. This affects very popular sites like reddit.com and flickr.com.
[Describe test coverage new/current, TreeHerder]: I don't know how we could test for this. We've obtained a machine that doesn't support SSE2 for the SF office, which should help us identify this kind of issue much faster next time.
[Risks and why]: Extremely low risk. All we do is check for SSE2 before actually using it. Only two lines of C++ code are touched.
[String/UUID change made/needed]: None.
Attachment #8641441 - Flags: approval-mozilla-beta?
Attachment #8641441 - Flags: approval-mozilla-aurora?
Flags: needinfo?(lmandel)
Lawrence, just to be clear: I know we already built 40 beta 9, but I think this is of such high importance, and so unlikely to introduce a new issue, that we should do a respin to include this patch.
Seth, how can we check if we have a machine that would reproduce the issue... are there some clear steps to reproduce this issue (maybe a URL that would show the issue)? Most of our machines are fairly new, so we might not be able to reproduce this.
Flags: needinfo?(seth)
https://hg.mozilla.org/mozilla-central/rev/4a0e7714b3ce
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #7)
> Seth, how can we check if we have a machine that would reproduce the
> issue... are there some clear steps to reproduce this issue (maybe a URL
> that would show the issue)? Most of our machines are fairly new, so we might
> not be able to reproduce this.

It may be hard, unfortunately. Beta breakers was able to reproduce, but I borrowed the machine that they used to reproduce it! You should be able to reproduce if you have a machine that is old enough that it doesn't support SSE2. That'd be Pentium III or before on the Intel side and Athlon XP or before on the AMD side.

To reproduce, it's enough to visit these URLs on an affected machine:

http://www.breitbart.com/
https://www.reddit.com/r/firefox/
http://phys.org/
http://flickr.com

All of these URLs contain scaled JPEG images which will be replaced by black rectangles when the bug occurs. (You may need to scroll down a page or two to get to the affected images on some of these pages, depending on your monitor's resolution.)
Flags: needinfo?(seth)
Tracking 40+ as this is a user visible regressions that may result in black rectangles being drawn instead of jpgs.
Flags: needinfo?(lmandel)
Keywords: regression
(In reply to Seth Fowler [:seth] [:s2h] from comment #9)
> It may be hard, unfortunately. Beta breakers was able to reproduce, but I
> borrowed the machine that they used to reproduce it! You should be able to
> reproduce if you have a machine that is old enough that it doesn't support
> SSE2. That'd be Pentium III or before on the Intel side and Athlon XP or
> before on the AMD side.

We have a few somewhat older machines, but nothing this old, so it seems that we won't be able to verify this on our side.
Comment on attachment 8641441 [details] [diff] [review]
Only use SSE2 in image::Downscaler if we're running on an SSE2-capable machine

Let's take it for the RC then. 
Adding to approval-mozilla-release to make sure it lands in the 40 branch as we are going to do the m-b => m-r merge today.
Attachment #8641441 - Flags: approval-mozilla-release+
Attachment #8641441 - Flags: approval-mozilla-beta?
Attachment #8641441 - Flags: approval-mozilla-beta+
Attachment #8641441 - Flags: approval-mozilla-aurora?
Attachment #8641441 - Flags: approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> Let's take it for the RC then. 
> Adding to approval-mozilla-release to make sure it lands in the 40 branch as
> we are going to do the m-b => m-r merge today.

Whew! Thanks for approving, Sylvestre. That's a major relief.
You need to log in before you can comment on or make changes to this bug.