878446 - webrtc's libyuv shouldnt inconditionally use ssse3

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Landry Breuil (:gaston)]

We have proper SSSE3 detection in configure.in since bug 786995 (HAVE_TOOLCHAIN_SUPPORT_SSSE3), it'd be nice if libyuv.gyp could make use of this to turn the yuv_disable_asm knob, since libyuv inconditionally uses SSSE3 if asm isnt disabled.

Otherwise right now, on OpenBSD/i386 with g++ 4.6, webrtc builds fails with :

/home/landry/m-c/media/webrtc/trunk/third_party/libyuv/source/rotate.cc:383: Error: no such instruction: `palignr $0x8,%xmm3,%xmm3'

Comment 1

[Landry Breuil (:gaston)]

Attachment: conditionally disable asm in libyuv if the toolchain doesnt support it

Doesnt work yet, but here's a wip. I think libyuv.target.mk gets in the way, or gyp itself.

Comment 2

[Jan Beich]

Comment on attachment 757032 [details] [diff] [review]
conditionally disable asm in libyuv if the toolchain doesnt support it

Review of attachment 757032 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +9140,5 @@
>     $PYTHON ${srcdir}/media/webrtc/trunk/build/gyp_chromium \
>       $GYP_WEBRTC_OPTIONS \
>       --generator-output=${_objdir}/media/webrtc/trunk \
> +     ${srcdir}/media/webrtc/trunk/peerconnection.gyp \
> +     ${srcdir}/media/webrtc/trunk/third_party/libyuv/libyuv.gyp

libyuv.gyp is part of peerconnection.gyp's "dependencies", no need to explicitly list it

::: media/webrtc/trunk/build/common.gypi
@@ +1544,5 @@
>        ['use_libjpeg_turbo==1', {
>          'defines': ['USE_LIBJPEG_TURBO=1'],
>        }],
> +      ['yuv_disable_asm==1', {
> +        'defines': ['YUV_DISABLE_ASM=1'],

Does the macro span several directories? If not why are you adding it to common.gypi? libyuv.gyp alread has -DYUV_DISABLE_ASM check.

::: media/webrtc/webrtc_config.gypi
@@ +15,5 @@
>      'clang_use_chrome_plugins': 0,
>      'enable_protobuf': 0,
>      'include_tests': 0,
>      'enable_android_opensl': 1,
> +    'yuv_disable_asm': 0,

Are you trying to force it enabled?

https://code.google.com/p/gyp/wiki/InputFormatReference#Providing_Default_Values_for_Variables_%28%%29

and libyuv.gyp alread has it

  'variables': {
     'yuv_disable_asm%': 0,
  },

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 757032 [details] [diff] [review]
conditionally disable asm in libyuv if the toolchain doesnt support it

Review of attachment 757032 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/webrtc_config.gypi
@@ +15,5 @@
>      'clang_use_chrome_plugins': 0,
>      'enable_protobuf': 0,
>      'include_tests': 0,
>      'enable_android_opensl': 1,
> +    'yuv_disable_asm': 0,

Jan is right - only do this if you want to override the value in the variable%: value statement in the regular .gyp/gypi file.  Since that's already 0...  also this will override any value passed in by command-line params.  If you want to change the default but still need to override from command-line (configure.in), use a % here.

Comment 4

[Landry Breuil (:gaston)]

I dont want to change any default, i only want to see -DYUV_DISABLE_ASM=1 in the generated $objdir/media/webrtc/trunk/third_party/libyuv/libyuv_libyuv/Makefile when either HAVE_TOOLCHAIN_SUPPORT_SSSE3 or HAVE_TOOLCHAIN_SUPPORT_MSSE4_1 empty. But so far, everything i tried failed, while it used to work (i think) when the first chunks of 807492 related to MSSE4.1 landed. If i only change the configure.in test to also trigger if HAVE_TOOLCHAIN_SUPPORT_SSSE3, there's no YUV_DISABLE_ASM=1 in libyuv's build chain thus it fails.

MSSE4_1 isnt enough either, here both are empty and still no YUV_DISABLE_ASM. So something broke it ?

$grep MSSE /usr/obj/m-c/config.status 
    (''' HAVE_TOOLCHAIN_SUPPORT_MSSSE3 ''', r'''  '''),
    (''' HAVE_TOOLCHAIN_SUPPORT_MSSE4_1 ''', r'''  '''),

$grep YUV_DISABLE_ASM /usr/obj/m-c/media/webrtc/trunk/third_party/libyuv/libyuv_libyuv/Makefile
->nothing

Comment 5

[Jan Beich]

Attachment: v2

It seems gyp changed how it treats duplicates. This needs a Try run.

Comment 6

[Landry Breuil (:gaston)]

https://tbpl.mozilla.org/?tree=Try&rev=1c70068f57e6

Comment 7

[Landry Breuil (:gaston)]

Comment on attachment 758448 [details] [diff] [review]
v2

Fwiw, this seems to do the right thing for me (ie -DYUV_DISABLE_ASM on OpenBSD/i386) with all the other webrtc patches.

Comment 8

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4a92ea700b

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ff4a92ea700b

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Your try run shows that Android (for example) still is getting YUV_DISABLE_ASM=1 since it doesn't support SSE3.....  And yes, a grep will show it has tests for YUV_DISABLE_ASM in NEON-focused builds.

My apologies; I didn't review v2 of the patch with a lot of thought about how it works.

Comment 11

[Landry Breuil (:gaston)]

(In reply to Randell Jesup [:jesup] from comment #10)
> Your try run shows that Android (for example) still is getting
> YUV_DISABLE_ASM=1 since it doesn't support SSE3.....  And yes, a grep will
> show it has tests for YUV_DISABLE_ASM in NEON-focused builds.

I'm confused now - should YUV_DISABLE_ASM=1 only be set on x86* archs where ssse3/sse4.1 is unsupported ? Is android/arm not supposed to get YUV_DISABLE_ASM ?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Correct - we don't want to disable NEON assembly on ARM if it doesn't support SSE3 :-)

Also, I'm about to land an update to libyuv (bug 880419), though the same mechanism for disabling assembly will exist (yuv_disable_asm).

I will note that it not working on ARM certainly can be considered a separate bug, so I'll I'll reclose this and fix ARM in another bug (or the update)

Comment 13

[Landry Breuil (:gaston)]

Right, your call - maybe it's only a matter of doing the ssse3/sse4.1 disable_yuv dance only on platforms where it makes sense, like x86/x86_64.