Closed Bug 571739 Opened 9 years ago Closed 8 years ago

Use SIMD for conversion from RGB to YUV on Win64

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: m_kato, Assigned: m_kato)

Details

Attachments

(2 files, 5 obsolete files)

VC++ doesn't support MMX and inline assembler.  I need write SSE2 intrin verison for this.
Summary: Use SIMD for conversion to RGB to YUV on Win64 → Use SIMD for conversion from RGB to YUV on Win64
Attached patch patch (obsolete) — Splinter Review
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #450980 - Attachment is obsolete: true
Attached patch patch v1.2 (obsolete) — Splinter Review
Attachment #450982 - Attachment is obsolete: true
Attachment #451539 - Flags: review?(chris)
Comment on attachment 451539 [details] [diff] [review]
patch v1.2

This is more Chris Double's domain than mine, so passing review to him.
Attachment #451539 - Flags: review?(chris) → review?(chris.double)
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.
Attachment #451539 - Flags: review?(chris.double) → review+
Attachment #451539 - Flags: review?(kinetik)
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.
(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 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!
Attachment #451539 - Flags: review?(kinetik) → review+
(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.
Attached patch fix v2 (obsolete) — Splinter Review
Attached patch fix v2.1 (obsolete) — Splinter Review
Attachment #498685 - Attachment is obsolete: true
Attachment #498687 - Flags: review?(kinetik)
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.
+// 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 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.
Attachment #498687 - Flags: review?(kinetik)
Attached patch fix v3Splinter Review
I forget updating patch...
Attachment #451539 - Attachment is obsolete: true
Attachment #498687 - Attachment is obsolete: true
Attachment #522931 - Flags: review?(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.
Attachment #522931 - Flags: review?(kinetik) → review+
http://hg.mozilla.org/mozilla-central/rev/0eaee388eead
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(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.
VC8++ (x64) doesn't have _mm_castsi128_ps, is the minimum version documented?

(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.
Attached patch fix VC8 supportSplinter Review
Attachment #529425 - Flags: review?(neil)
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
Attachment #529425 - Flags: review?(neil) → review+
You need to log in before you can comment on or make changes to this bug.