1051087 - arm_version is not populated to gecko/media/webrtc/trunk/webrtc/common_audio properly

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Jay]

[Blocking Requested - why for this release]:

gecko/media/webrtc/trunk/webrtc/common_audio/common_audio.gyp uses arm_version to decide when to include the neon optimized libraries. 

Here is the part of the common_audio.gyp:
    ['(target_arch=="arm" and arm_version==7) or target_arch=="armv7"', {
      'targets': [
        {
          'target_name': 'common_audio_neon',
          'type': 'static_library',
          'includes': ['../build/arm_neon.gypi',],
          'sources': [
            'resampler/sinc_resampler_neon.cc',
            'signal_processing/cross_correlation_neon.S',
            'signal_processing/downsample_fast_neon.S',
            'signal_processing/min_max_operations_neon.S',
            'signal_processing/vector_scaling_operations_neon.S',
          ],
        },
      ],  # targets

However, arm_version variable was stored as string instead of integer in build/gyp.mozbuild. This causes common_audio.gyp incorrectly picks up non-neon libraries.

Here is the proposed change:
diff --git a/build/gyp.mozbuild b/build/gyp.mozbuild
index fa52b2c..c9fd5d9 100644
--- a/build/gyp.mozbuild
+++ b/build/gyp.mozbuild
@@ -106,7 +106,7 @@ if CONFIG['ARM_ARCH']:
         # detection, so we have to set armv7=0 for non-Android target
         gyp_vars['armv7'] = 0
     # For libyuv
-    gyp_vars['arm_version'] = CONFIG['ARM_ARCH']
+    gyp_vars['arm_version'] = int(CONFIG['ARM_ARCH'])

Comment 1

[Brogan Zumwalt [Inactive]]

No STR is present to create test case to address bug.

Comment 2

[bhavana bajaj [:bajaj]]

Ni, :jesup for next steps or help with patch here. Jesup we are a week away to get all 2.0+ blockers closed, can you please help here or give us an eta?

Comment 3

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

Attachment: Ensure NEON optimizations are used for libyuv and webrtc audio code

Comment 4

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

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

Once on central will also nominate for 33

Comment 5

[Ryan VanderMeulen (PTO, back 6-April)]

Backed out for Android bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1254b0f2fd8

https://tbpl.mozilla.org/php/getParsedLog.php?id=46690301&tree=Mozilla-Inbound

Comment 6

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

Attachment: Ensure NEON optimizations are used for libyuv and webrtc audio code

resolve android bustage; confirmed locally

Comment 7

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

https://tbpl.mozilla.org/?tree=Try&rev=9a7402d892f6
waiting for tree opening

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/32269088de49
Try is green; one known orange

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/32269088de49

Comment 10

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/8f631c20c821

This seems like something we might on v1.4 as well?

Comment 11

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

Comment on attachment 8478372 [details] [diff] [review]
Ensure NEON optimizations are used for libyuv and webrtc audio code

Approval Request Comment
[Feature/regressing bug #]: gyp.mozbuild landing

[User impact if declined]: much worse performance in webrtc code (esp audio) on ARM

[Describe test coverage new/current, TBPL]: on b2g32 and nightly for a while.

[Risks and why]: low risk given taking on b2g32 already,  enabled codepaths used in upstream

[String/UUID change made/needed]: none

Comment 12

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ec5f67c9d712

Comment 13

[Ryan VanderMeulen (PTO, back 6-April)]

Should have been:
https://hg.mozilla.org/releases/mozilla-aurora/rev/84bdca67fffe