Use SIMD for conversion from RGB to YUV on Win64

RESOLVED FIXED in mozilla6

Status

()

Core
Audio/Video
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

Trunk
mozilla6
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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

Updated

7 years ago
Summary: Use SIMD for conversion to RGB to YUV on Win64 → Use SIMD for conversion from RGB to YUV on Win64
Created attachment 450980 [details] [diff] [review]
patch
Created attachment 450982 [details] [diff] [review]
patch v1.1
Attachment #450980 - Attachment is obsolete: true
Created attachment 451539 [details] [diff] [review]
patch v1.2
Attachment #450982 - Attachment is obsolete: true
(Assignee)

Updated

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

7 years ago
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+
(Assignee)

Updated

7 years ago
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.
Created attachment 498685 [details] [diff] [review]
fix v2
Created attachment 498687 [details] [diff] [review]
fix v2.1
Attachment #498685 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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)
Created attachment 522931 [details] [diff] [review]
fix v3

I forget updating patch...
Attachment #451539 - Attachment is obsolete: true
Attachment #498687 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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
Last Resolved: 6 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.
Created attachment 529425 [details] [diff] [review]
fix VC8 support
(Assignee)

Updated

6 years ago
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+
landed follow up
http://hg.mozilla.org/mozilla-central/rev/1461de626881
You need to log in before you can comment on or make changes to this bug.