Closed
Bug 616778
Opened 14 years ago
Closed 14 years ago
YCbCr conversion code may run SSE2 instructions on Windows machines which don't support SSE2
Categories
(Core :: Audio/Video, defect)
Tracking
()
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
(Keywords: regression, Whiteboard: [hardblocker])
Attachments
(3 files, 1 obsolete file)
218 bytes,
text/html
|
Details | |
10.73 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
89.18 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
(Split off from a bug 583138 comment 16.) In yuv_convert.cpp: > #if defined(MOZILLA_COMPILE_WITH_SSE2) > static void FilterRows(uint8* ybuf, const uint8* y0_ptr, const uint8* y1_ptr, > int source_width, int source_y_fraction) { I don't have an old CPU to test on, but I think this will try and run SSE2 code on any Windows machine, even if it doesn't support SSE2. MOZILLA_COMPILE_WITH_SSE2 is always defined on Windows, and I don't see any cpuid guards around the call to FilterRows. In fact, the unvectorized implementation isn't even compiled when MOZILLA_COMPILE_WITH_SSE2 is defined. Even if I'm misunderstanding this and the code here is fine, it will still need to be changed when bug 585708 lands. (This will also make the FitlerRows optimizations available on Linux 32, where they're currently unavailable.) So changes to this file should probably land together with bug 585708.
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Updated•14 years ago
|
OS: Windows 7 → Windows XP
Assignee | ||
Comment 1•14 years ago
|
||
I'd love to confirm that this causes a crash on old hardware. Juan, do you know if QA has any pre-SSE2 hardware available? I'll attach a testcase in a moment.
Assignee | ||
Comment 2•14 years ago
|
||
Assignee | ||
Comment 3•14 years ago
|
||
By joedrew's reasoning in bug 616782 comment 7, this should block final, unless I'm misunderstanding what's going on here.
blocking2.0: --- → ?
Assignee: nobody → chris.double
blocking2.0: ? → final+
Assignee | ||
Comment 4•14 years ago
|
||
Need to test this, but I first need to fix the Windows compile error in bug 616782. Once I get it working, I'll update gfx/ycbcr/convert.patch.
Assignee: chris.double → justin.lebar+bug
Assignee | ||
Comment 5•14 years ago
|
||
Compile error is unrelated to this fix. See bug 619178. Once I fix that one, I'll re-push to try.
Assignee | ||
Updated•14 years ago
|
Attachment #496521 -
Attachment is obsolete: true
Comment 7•14 years ago
|
||
You'll need to include a patch file with this change, update the README to reference this patch file and bug number, and change update.sh to apply this patch file before landing. This is so your change doesn't get lost when we next update agaisnt the Chromium code.
Assignee | ||
Comment 8•14 years ago
|
||
I'm thinking I'll just merge all my changes (this bug, bug 619178, bug 616782) into the one current ycbcr patch. That sound OK?
Comment 9•14 years ago
|
||
Sounds great!
I presume this is a regression in that some users will now crash when trying to play <video>s when they didn't before.
Keywords: regression
Comment 11•14 years ago
|
||
Justin, did you ever end up merging those files? Does this still need review?
Assignee | ||
Comment 12•14 years ago
|
||
(In reply to comment #11) > Justin, did you ever end up merging those files? Does this still need review? Sorry, comment 8 isn't particularly clear. There's a patch in the ycbcr directory which converts the upstream code into our version. Once I get reviews on the three bugs, I'll merge those changes into the ycbcr patch in the tree. So yes, this and the patches in the two other bugs each need a separate review.
Whiteboard: [hardblocker]
Comment 13•14 years ago
|
||
Comment on attachment 498148 [details] [diff] [review] Patch v2 Do we not have to change the CXXFLAGS for MSVC? r+ as long as the bodies of those functions are unchanged.
Attachment #498148 -
Flags: review?(joe) → review+
Assignee | ||
Comment 14•14 years ago
|
||
(In reply to comment #13) > Comment on attachment 498148 [details] [diff] [review] > Patch v2 > > Do we not have to change the CXXFLAGS for MSVC? No, MSVC doesn't need a flag to accept intrinsics.
Assignee | ||
Comment 15•14 years ago
|
||
(In reply to comment #8) > I'm thinking I'll just merge all my changes (this bug, bug 619178, bug 616782) > into the one current ycbcr patch. That sound OK? There's now a second patch in gfx/ycbcr, arm.patch. I think it's going to be more effort than it's worth to keep this patch separate, since my changes touch code near that patch. Oleg and Chris, is that OK with you?
Assignee | ||
Comment 16•14 years ago
|
||
Attachment #503091 -
Flags: review?(chris.double)
Assignee | ||
Comment 17•14 years ago
|
||
I created the new version of convert.patch as follows: * Starting at hg patch update-yuv-patch * Apply patches from this bug and bug 619178. * Check out chromium tree, update to r63840. * hg qnew backwards-patch * Remove calls to |patch| from update.sh, so it just copies straight from the chromium tree and doesn't patch the files. * Run update.sh * hg rm files added in previous patches * hg qref * hg diff backwards-patch:update-yuv-patch > ~/convert.patch * hg qpop (pop backwards-patch, arrive at update-yuv-patch) * mv ~/convert.patch gfx/ycbcr/convert.patch * hg qref I also tested that the patch works by running update.sh on a clean tree and diff'ing the result against my patched tree.
Comment 18•14 years ago
|
||
Comment on attachment 503091 [details] [diff] [review] Update NPOTB files (particularly convert.patch) Justin asked me to review this on IRC.
Attachment #503091 -
Flags: review?(chris.double) → review+
Assignee | ||
Comment 19•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d580ec700a11 http://hg.mozilla.org/mozilla-central/rev/ab3e03c79004
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 20•14 years ago
|
||
Is there a way I can verify this bug?
Comment 21•14 years ago
|
||
Play a video on a machine without the SSE instruction set (e.g. AMD Athlon before the Athlon XP, or a Pentium 2) and with HW acceleration disabled.
Comment 22•14 years ago
|
||
Unfortunately, I don't have access to such hardware. I've put out a call to the community to help verify this bug. Thanks.
Assignee | ||
Comment 23•14 years ago
|
||
(In reply to comment #21) > Play a video on a machine without the SSE instruction set (e.g. AMD Athlon > before the Athlon XP, or a Pentium 2) and with HW acceleration disabled. I think the video needs to be scaled down, specifically. See testcase attached to bug. The machine needs to be running Windows. Also, it would be good to verify that it was broken before and now fixed, since, for lack of hardware, I couldn't verify that it was broken either.
Comment 24•14 years ago
|
||
Added to the nightly tester instructions, thanks.
Comment 25•14 years ago
|
||
(In reply to comment #21) > Play a video on a machine without the SSE instruction set (e.g. AMD Athlon > before the Athlon XP, or a Pentium 2) and with HW acceleration disabled. Sorry, I misread the bug title. You need a machine with SSE but not SSE2. If it doesn't have SSE at all, I think we'd hit the MMX or pure C code instead.
Comment 26•14 years ago
|
||
(In reply to comment #25) > (In reply to comment #21) > > Play a video on a machine without the SSE instruction set (e.g. AMD Athlon > > before the Athlon XP, or a Pentium 2) and with HW acceleration disabled. > > Sorry, I misread the bug title. You need a machine with SSE but not SSE2. If > it doesn't have SSE at all, I think we'd hit the MMX or pure C code instead. So what does that mean for hardware requirements? If I am correct, the Athlon XP was the last non-SSE2 AMD CPU and Pentium 3 was the last non-SSE2 Intel CPU...
Comment 27•14 years ago
|
||
Athlon XP added SSE, SSE2 wasn't added until the Athlon 64. So: Intel Pentium 3 range AMD Athlon XP range I think.
Comment 28•14 years ago
|
||
This causes bustage on Win64 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294920732.1294924843.19536.gz#err0 Microsoft compiler doesn't support MMX.
Assignee | ||
Comment 29•14 years ago
|
||
Bustage from comment 28 is bug 625629.
Comment 31•14 years ago
|
||
It's been well over a month and no one has seemed to step up to verify this is fixed. Also, we have not seen much duping against this bug. Based on that, I think we can mark this bug verified. Justin, Matthew, what do you think?
Assignee | ||
Comment 32•14 years ago
|
||
Fine with me.
You need to log in
before you can comment on or make changes to this bug.
Description
•