620526 - yv12_to_rgb565_neon should not be used on CPUs without NEON

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Michael Wu [:mwu]]

Not all modern ARM cpus have NEON. In particular, the tegra does not have it.

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: ARM neon check

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 498966 [details] [diff] [review]
ARM neon check

this certainly looks right to me, but its not my module

Comment 3

[Michael Wu [:mwu]]

(In reply to comment #1)
> Created attachment 498966 [details] [diff] [review]
> ARM neon check

I meant we need to have a runtime check.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #3)
> (In reply to comment #1)
> > Created attachment 498966 [details] [diff] [review] [details]
> > ARM neon check
> 
> I meant we need to have a runtime check.

both would be good, but yea you'd need a runtime check for the tegra to work

Comment 5

[Oleg Romashin (:romaxa)]

something like this:
http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-cpu.c#223
?

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

yea. We currently do just about the same check in jstracer.cpp and jstracer.c.

Comment 7

[Michael Wu [:mwu]]

(In reply to comment #5)
> something like this:
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-cpu.c#223
> ?

Well, closer to http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-cpu.c#247 but yeah, need some variant of it. There's also cpu detection in media/libvpx/vpx_ports/arm_cpudetect.c and another cpu detector in js. We have too many of them.

Comment 8

[tom brinkman]

> Well, closer to
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/pixman-cpu.c#247

How about something like this?

static pixman_bool_t
pixman_have_ycpcr_to_rgb565 ()
{
    int fd;
    Elf32_auxv_t aux;
	pixman_bool_t arm_has_neon = 0;
	
    fd = open ("/proc/self/auxv", O_RDONLY);
    if (fd >= 0)
    {
	while (read (fd, &aux, sizeof(Elf32_auxv_t)) == sizeof(Elf32_auxv_t))
	{
	    if (aux.a_type == AT_HWCAP)
	    {
			arm_has_neon = (hwcap & 4096) != 0;
			break;
	    }
	}
	close (fd);
    }
    
    return arm_has_neon;
}

Comment 9

[Oleg Romashin (:romaxa)]

tom could you attach working patch and ask for review?

Comment 10

[tom brinkman]

Attachment: Patch - have_ycbcr_to_rgb565

I copied the logic from piximan-cpu file and moved it 
here for runtime test in ycbcr folder.  This file could easily be moved
to a more central common location.

There is a test case in the following attachment.

Comment 11

[tom brinkman]

Attachment: Test case for previous patch

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 501747 [details] [diff] [review]
Patch - have_ycbcr_to_rgb565

I suspect Chris Double is the right reviewer for this.

Comment 13

[Doug Turner (:dougt)]

g++-4.2 -arch i386 -o ycbcr_to_rgb565.o -c  -fvisibility=hidden -D_IMPL_NS_GFX -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Darwin\" -DOSARCH=Darwin  -I/builds/slave/try-osx64/build/gfx/ycbcr -I. -I../../dist/include -I../../dist/include/nsprpub  -I/builds/slave/try-osx64/build/obj-firefox/i386/dist/include/nspr -I/builds/slave/try-osx64/build/obj-firefox/i386/dist/include/nss       -fPIC  -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -gdwarf-2 -isysroot /Developer/SDKs/MacOSX10.5.sdk -fno-strict-aliasing -fpascal-strings -fno-common -fshort-wchar -pthread -DNO_X11  -DNDEBUG -DTRIMMED -gdwarf-2 -O3 -fomit-frame-pointer   -DMOZILLA_CLIENT -include ../../mozilla-config.h /builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp:60:17: error: elf.h: No such file or directory
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp: In function 'int have_ycbcr_to_rgb565()':
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp:96: error: 'Elf32_auxv_t' was not declared in this scope
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp:96: error: expected `;' before 'aux'
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp:101: error: 'aux' was not declared in this scope
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp:103: error: 'AT_HWCAP' was not declared in this scope
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp:105: error: 'uint32_t' was not declared in this scope
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp:105: error: expected `;' before 'hwcap'
/builds/slave/try-osx64/build/gfx/ycbcr/ycbcr_to_rgb565.cpp:106: error: 'hwcap' was not declared in this scope



Does not compile on the mac.

Comment 14

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/c0db6f2597a2 with build bustage fixes.

Comment 15

[[e-mail]]

Comment on attachment 501747 [details] [diff] [review]
Patch - have_ycbcr_to_rgb565

So, for those of you still building non-libxul:

(3x)
>+int have_ycbcr_to_rgb565 ()
NS_GFX_(int) have_ycbcr_to_rgb565 ()

>+int have_ycbcr_to_rgb565();
#include "gfxCore.h"
NS_GFX_(int) have_ycbcr_to_rgb565();