571739 - Use SIMD for conversion from RGB to YUV on Win64

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Makoto Kato [:m_kato]]

VC++ doesn't support MMX and inline assembler.  I need write SSE2 intrin verison for this.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch

Comment 2

[Makoto Kato [:m_kato]]

Attachment: patch v1.1

Comment 3

[Makoto Kato [:m_kato]]

Attachment: patch v1.2

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 451539 [details] [diff] [review]
patch v1.2

This is more Chris Double's domain than mine, so passing review to him.

Comment 5

[cajbir (:cajbir)]

Comment on attachment 451539 [details] [diff] [review]
patch v1.2

The changes to integrate into ycbcr and apply patches look ok to me but I don't know anything about the 64 bit sse2 intrinsics to comment on that code. I suggest either :derf or :kinetik to cast a quick review over that code.

Comment 6

[Justin Lebar (not reading bugmail)]

I think libjpeg-turbo may have SIMD code for this conversion which will work on 32- and 64-bit systems.

I'm not too familiar with what these functions need to do, but see e.g. bug 584652.

Comment 7

[Makoto Kato [:m_kato]]

(In reply to comment #6)
> I think libjpeg-turbo may have SIMD code for this conversion which will work on
> 32- and 64-bit systems.
> 
> I'm not too familiar with what these functions need to do, but see e.g. bug
> 584652.

This code is for media/video.   Do you replace gfx/ycbcr with libjpeg-turbo's code?

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 451539 [details] [diff] [review]
patch v1.2

Can you please file a bug upstream in the Chromium bug tracker and attach a patch, or at least point them to this bug?  This seems like a worthwhile contribution to make upstream.

+#if defined(ARCH_CPU_64_BITS)

Do we need to worry about IA64 on Windows?  I'm hoping not.  If we do, we need more ifdefs to make sure it falls back to the C code.

Otherwise, r+.  Thanks!

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to comment #7)
> This code is for media/video.   Do you replace gfx/ycbcr with libjpeg-turbo's
> code?

Oh, I see.  Nope; libjpeg-turbo has nothing to do with this.

Comment 10

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 11

[Makoto Kato [:m_kato]]

Attachment: fix v2.1

Comment 12

[Justin Lebar (not reading bugmail)]

It would be great if this were coordinated with the landing of bug 585708 and its dependencies.  If not, whoever lands second is going to have to rebase his patch(es).  :)

Bug 619178, bug 616778, and bug 616782 all touch this code, and of course bug 585708 changes how SSE detection works.  Two of those are blockers, so hopefully the code will get in there soon.

Comment 13

[Matthew Gregan [:kinetik]]

+// x64 compiler does'nt support MMX and inline assembler.  Use SSE2 intrin.

"doesn't", and "intrin" -> "intrinsics", please.

+void ScaleYUVToRGB32Row(const uint8* y_buf,  // rdi
+                        const uint8* u_buf,  // rsi
+                        const uint8* v_buf,  // rdx
+                        uint8* rgb_buf,      // rcx
+                        int width,           // r8
+                        int source_dx) {     // r9

Presumably the comments are from yuv_row_posix.cpp, and they don't apply directly for this code.  Can you please remove them to avoid confusion?

Everything else looks fine.

Comment 14

[Matthew Gregan [:kinetik]]

Comment on attachment 498687 [details] [diff] [review]
fix v2.1

Clearing review.  This needs to be rebased now that the bugs mentioned in comment 12 have landed.

Comment 15

[Makoto Kato [:m_kato]]

Attachment: fix v3

I forget updating patch...

Comment 16

[Matthew Gregan [:kinetik]]

Comment on attachment 522931 [details] [diff] [review]
fix v3

(In reply to comment #13)
> +// x64 compiler does'nt support MMX and inline assembler.  Use SSE2 intrin.
> 
> "doesn't", and "intrin" -> "intrinsics", please.

r+ with this fixed.

I haven't built or tested this as I don't have a 64-bit Windows build environment.

Comment 17

[Makoto Kato [:m_kato]]

http://hg.mozilla.org/mozilla-central/rev/0eaee388eead

Comment 18

[Mitchell Field [:Mitch]]

(In reply to comment #16)
> Comment on attachment 522931 [details] [diff] [review]
> fix v3
> 
> (In reply to comment #13)
> > +// x64 compiler does'nt support MMX and inline assembler.  Use SSE2 intrin.
> > 
> > "doesn't", and "intrin" -> "intrinsics", please.
> 
> r+ with this fixed.

Looks like those changes required for r+ didn't happen.

Comment 19

[[e-mail]]

VC8++ (x64) doesn't have _mm_castsi128_ps, is the minimum version documented?

Comment 20

[Makoto Kato [:m_kato]]

(In reply to comment #18)
> (In reply to comment #16)
> > Comment on attachment 522931 [details] [diff] [review]
> > fix v3
> > 
> > (In reply to comment #13)
> > > +// x64 compiler does'nt support MMX and inline assembler.  Use SSE2 intrin.
> > > 
> > > "doesn't", and "intrin" -> "intrinsics", please.
> > 
> > r+ with this fixed.
> 
> Looks like those changes required for r+ didn't happen.

Thanks. I forget updating patch file.  I will land comment fix with others.


(In reply to comment #19)
> VC8++ (x64) doesn't have _mm_castsi128_ps, is the minimum version documented?

Tinderbox uses VC++9.  But I should fix it by Makefile or emulate it.

Comment 21

[Makoto Kato [:m_kato]]

Attachment: fix VC8 support

Comment 22

[[e-mail]]

Comment on attachment 529425 [details] [diff] [review]
fix VC8 support

Patch builds for me with VC8x64

>+# VC8 doesn't support some SSE2 build-in functions
built-in

Comment 23

[Makoto Kato [:m_kato]]

landed follow up
http://hg.mozilla.org/mozilla-central/rev/1461de626881