Last Comment Bug 571739 - Use SIMD for conversion from RGB to YUV on Win64
: Use SIMD for conversion from RGB to YUV on Win64
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla6
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-13 00:04 PDT by Makoto Kato [:m_kato]
Modified: 2011-05-08 20:16 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.15 KB, patch)
2010-06-13 21:18 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
patch v1.1 (6.96 KB, patch)
2010-06-13 21:22 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
patch v1.2 (7.56 KB, patch)
2010-06-16 03:47 PDT, Makoto Kato [:m_kato]
cajbir.bugzilla: review+
kinetik: review+
Details | Diff | Splinter Review
fix v2 (7.87 KB, patch)
2010-12-19 21:59 PST, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v2.1 (15.25 KB, patch)
2010-12-19 22:02 PST, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v3 (18.21 KB, patch)
2011-03-29 22:49 PDT, Makoto Kato [:m_kato]
kinetik: review+
Details | Diff | Splinter Review
fix VC8 support (1.57 KB, patch)
2011-05-02 00:32 PDT, Makoto Kato [:m_kato]
neil: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2010-06-13 00:04:27 PDT
VC++ doesn't support MMX and inline assembler.  I need write SSE2 intrin verison for this.
Comment 1 Makoto Kato [:m_kato] 2010-06-13 21:18:14 PDT
Created attachment 450980 [details] [diff] [review]
patch
Comment 2 Makoto Kato [:m_kato] 2010-06-13 21:22:28 PDT
Created attachment 450982 [details] [diff] [review]
patch v1.1
Comment 3 Makoto Kato [:m_kato] 2010-06-16 03:47:32 PDT
Created attachment 451539 [details] [diff] [review]
patch v1.2
Comment 4 Chris Pearce (:cpearce) 2010-06-16 14:32:04 PDT
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) 2010-07-14 18:05:29 PDT
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) 2010-08-09 09:27:00 PDT
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] 2010-08-09 19:19:43 PDT
(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] 2010-08-09 19:55:42 PDT
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) 2010-08-10 11:50:06 PDT
(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] 2010-12-19 21:59:45 PST
Created attachment 498685 [details] [diff] [review]
fix v2
Comment 11 Makoto Kato [:m_kato] 2010-12-19 22:02:18 PST
Created attachment 498687 [details] [diff] [review]
fix v2.1
Comment 12 Justin Lebar (not reading bugmail) 2010-12-19 22:27:20 PST
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] 2011-01-12 18:26:51 PST
+// 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] 2011-03-29 21:05:50 PDT
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] 2011-03-29 22:49:41 PDT
Created attachment 522931 [details] [diff] [review]
fix v3

I forget updating patch...
Comment 16 Matthew Gregan [:kinetik] 2011-03-30 15:26:29 PDT
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] 2011-04-24 22:37:31 PDT
http://hg.mozilla.org/mozilla-central/rev/0eaee388eead
Comment 18 Mitchell Field [:Mitch] 2011-04-25 06:03:46 PDT
(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 neil@parkwaycc.co.uk 2011-04-29 06:14:23 PDT
VC8++ (x64) doesn't have _mm_castsi128_ps, is the minimum version documented?
Comment 20 Makoto Kato [:m_kato] 2011-04-30 21:27:48 PDT

(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] 2011-05-02 00:32:57 PDT
Created attachment 529425 [details] [diff] [review]
fix VC8 support
Comment 22 neil@parkwaycc.co.uk 2011-05-02 11:34:51 PDT
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] 2011-05-08 20:16:20 PDT
landed follow up
http://hg.mozilla.org/mozilla-central/rev/1461de626881

Note You need to log in before you can comment on or make changes to this bug.