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

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Justin Lebar (not reading bugmail)]

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

Comment 1

[Justin Lebar (not reading bugmail)]

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.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Testcase

Comment 3

[Justin Lebar (not reading bugmail)]

By joedrew's reasoning in bug 616782 comment 7, this should block final, unless I'm misunderstanding what's going on here.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: 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.

Comment 5

[Justin Lebar (not reading bugmail)]

Compile error is unrelated to this fix. See bug 619178.  Once I fix that one, I'll re-push to try.

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Ready for review.

Comment 7

[cajbir (:cajbir)]

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.

Comment 8

[Justin Lebar (not reading bugmail)]

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

[cajbir (:cajbir)]

Sounds great!

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I presume this is a regression in that some users will now crash when trying to play <video>s when they didn't before.

Comment 11

[Joe Drew (not getting mail)]

Justin, did you ever end up merging those files? Does this still need review?

Comment 12

[Justin Lebar (not reading bugmail)]

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

Comment 13

[Joe Drew (not getting mail)]

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.

Comment 14

[Justin Lebar (not reading bugmail)]

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

Comment 15

[Justin Lebar (not reading bugmail)]

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

Comment 16

[Justin Lebar (not reading bugmail)]

Attachment: Update NPOTB files (particularly convert.patch)

Comment 17

[Justin Lebar (not reading bugmail)]

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

[Matthew Gregan [:kinetik]]

Comment on attachment 503091 [details] [diff] [review]
Update NPOTB files (particularly convert.patch)

Justin asked me to review this on IRC.

Comment 19

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/d580ec700a11
http://hg.mozilla.org/mozilla-central/rev/ab3e03c79004

Comment 20

[u279076]

Is there a way I can verify this bug?

Comment 21

[Matthew Gregan [:kinetik]]

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

[u279076]

Unfortunately, I don't have access to such hardware.  I've put out a call to the community to help verify this bug.  Thanks.

Comment 23

[Justin Lebar (not reading bugmail)]

(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

[u279076]

Added to the nightly tester instructions, thanks.

Comment 25

[Matthew Gregan [:kinetik]]

(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

[u279076]

(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

[Matthew Gregan [:kinetik]]

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

[Makoto Kato [:m_kato]]

This causes bustage on Win64

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294920732.1294924843.19536.gz#err0

Microsoft compiler doesn't support MMX.

Comment 29

[Justin Lebar (not reading bugmail)]

Bustage from comment 28 is bug 625629.

Comment 31

[u279076]

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?

Comment 32

[Justin Lebar (not reading bugmail)]

Fine with me.