webrtc's libyuv shouldnt inconditionally use ssse3

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

unspecified
mozilla24
x86
OpenBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC][blocking-webrtc-][qa-])

Attachments

(1 attachment, 1 obsolete attachment)

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'
Whiteboard: [WebRTC][blocking-webrtc-]
Doesnt work yet, but here's a wip. I think libyuv.target.mk gets in the way, or gyp itself.
Assignee: nobody → landry
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 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.
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
Posted patch v2Splinter Review
It seems gyp changed how it treats duplicates. This needs a Try run.
Attachment #757032 - Attachment is obsolete: true
Attachment #758448 - Flags: review?(rjesup)
Attachment #758448 - Flags: feedback?(landry)
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.
Attachment #758448 - Flags: feedback?(landry) → feedback+
Attachment #758448 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/ff4a92ea700b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc-][qa-]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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 ?
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)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.