YCbCr conversion code may run SSE2 instructions on Windows machines which don't support SSE2

VERIFIED FIXED

Status

()

Core
Audio/Video
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

({regression})

unspecified
x86
Windows XP
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
(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

7 years ago
No longer blocks: 585708
Depends on: 585708
(Assignee)

Updated

7 years ago
OS: Windows 7 → Windows XP
(Assignee)

Comment 1

7 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

7 years ago
Created attachment 495779 [details]
Testcase
(Assignee)

Comment 3

7 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

7 years ago
Created attachment 496521 [details] [diff] [review]
Patch v1

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

7 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

7 years ago
Depends on: 619178
(Assignee)

Comment 6

7 years ago
Created attachment 498148 [details] [diff] [review]
Patch v2

Ready for review.
Attachment #498148 - Flags: review?(joe)
(Assignee)

Updated

7 years ago
Attachment #496521 - Attachment is obsolete: true

Comment 7

7 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

7 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

7 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
Justin, did you ever end up merging those files? Does this still need review?
(Assignee)

Comment 12

7 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 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

7 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

7 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

7 years ago
Created attachment 503091 [details] [diff] [review]
Update NPOTB files (particularly convert.patch)
Attachment #503091 - Flags: review?(chris.double)
(Assignee)

Comment 17

7 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 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

7 years ago
http://hg.mozilla.org/mozilla-central/rev/d580ec700a11
http://hg.mozilla.org/mozilla-central/rev/ab3e03c79004
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Is there a way I can verify this bug?
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.
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

7 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.
Added to the nightly tester instructions, thanks.
(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.
(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...
Athlon XP added SSE, SSE2 wasn't added until the Athlon 64.  So:

Intel Pentium 3 range
AMD Athlon XP range

I think.
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)

Updated

7 years ago
Depends on: 625629
(Assignee)

Comment 29

7 years ago
Bustage from comment 28 is bug 625629.
Duplicate of this bug: 620207
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

7 years ago
Fine with me.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.