589743 - gfx/ycbcr doesn't build on !x86 && !x64 && !arm && !ppc && !sparc

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Replace #error with #warning in gfx/ycbcr/chromium_types.h

gfx/ycbcr/chromium_types.h defines various ARCH macros for x86, x64, arm, ppc and sparc, and errors out for any other architecture. It turns out these macros are used to select between specific assembly versions and a C fallback, which means the C fallback can be used for those "unsupported" architectures.

Replacing the #error with #warning allows to build.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Isn't the motivation here that porters to a new architecture need to at least be aware that they should consider writing an assembly codepath here (and explicitly adding their arch otherwise)?  Perhaps we should consider having a uniform format for #warnings about this sort of thing so it's easy to grep to detect them while porting?

Comment 2

[Benjamin Smedberg]

Let's not overthink this. We should either have an #error and --enable-slow-ycbcr or a #warning and not worry about the formatting.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Seems like the former would be better, to me...

Comment 4

[Mike Hommey [:glandium]]

Having a #error and --enable-slow-ycbcr will certainly lead to a) maintenance burden for linux distros, who will have to special case some architectures to have --enable-slow-ycbcr b) most likely, these architectures using the slow path today, still using the slow path when the fast path is patched in, because the --enable-slow-ycbcr is still used.

Comment 5

[Benjamin Smedberg]

Oh no, I wouldn't make --enable-slow-ycbcr force the slow path, just disable the #error.

Comment 6

[Mike Hommey [:glandium]]

(In reply to comment #5)
> Oh no, I wouldn't make --enable-slow-ycbcr force the slow path, just disable
> the #error.

Would be sensible. But now that I think of it, there are already plenty of areas in the mozilla codebase with slow paths on some architectures, yet, no such thing happen (most of the time, there aren't even #warnings). All in all, the patch as it is seems fine to me.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> yet, no such thing happen (most of the time, there aren't even #warnings)

An I'm saying that's a problem.

I have no issues with taking the attached patch for now to get things building, but I would really like to see a followup to identify the arch-dependent slow paths and make it easier for someone porting to deal with them instead of just silently getting terrible performance.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe all we need is a single symbol that porters can search on to find all the places in the code where we have SSE code.

Or better still, we should just ensure that every Gecko file that has Intel-specific optimizations uses SSE.h, so porters can just search for that.

Comment 9

[Mike Hommey [:glandium]]

Filed bug 590459.

Comment 10

[Mike Hommey [:glandium]]

http://hg.mozilla.org/mozilla-central/rev/a8dc1f514e9b