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

RESOLVED FIXED in Firefox 33

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jaywang, Assigned: jesup)

Tracking

unspecified
mozilla34
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 fixed, firefox34 fixed, b2g-v1.4 ?, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [webrtc-uplift])

Attachments

(1 attachment, 1 obsolete attachment)

[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'])
Assignee: nobody → rjesup
Whiteboard: [webrtc-uplift]
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
blocking-b2g: 2.0? → 2.0+
Flags: in-moztrap?(bzumwalt)
No STR is present to create test case to address bug.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage?][2.0-signoff-need+]
QA Whiteboard: [QAnalyst-Triage?][2.0-signoff-need+] → [QAnalyst-Triage+][2.0-signoff-need+]
Flags: needinfo?(ktucker)
Flags: in-moztrap?(bzumwalt)
Flags: in-moztrap-
QA Whiteboard: [QAnalyst-Triage+][2.0-signoff-need+] → [QAnalyst-Triage+][lead-review+][2.0-signoff-need+]
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?
Flags: needinfo?(rjesup)
Attachment #8478223 - Flags: review?(ted)
Flags: needinfo?(rjesup)
Attachment #8478223 - Flags: review?(ted) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ee81c83a01

Once on central will also nominate for 33
Target Milestone: --- → mozilla34
resolve android bustage; confirmed locally
Attachment #8478223 - Attachment is obsolete: true
Attachment #8478372 - Flags: review?(ted)
Attachment #8478372 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/32269088de49
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8478372 - Flags: approval-mozilla-aurora?
Attachment #8478372 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.