Provide a NEON flavor

Bug #388388 reported by Dave Martin
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
x264 (Ubuntu)
Fix Released
Medium
Loïc Minier

Bug Description

Binary package hint: x264

The NEON optimisation work done by David Conrad is now committed to mainline:
git://git.videolan.org/x264.git

Final commit:
http://git.videolan.org/?p=x264.git;a=commit;h=c8edc12043086b8f52a3b6b2176b70e0d48ff271

Would it be possible to merge these updates into the Ubuntu package for Karmic?

Tags: armel

Related branches

Paul Larson (pwlars)
tags: added: armel
description: updated
Paul Larson (pwlars)
Changed in x264 (Ubuntu):
importance: Undecided → Medium
status: New → Triaged
Revision history for this message
Dave Martin (dave-martin-arm) wrote :

The most recent relevant change now appears to be from 11 Nov:

http://git.videolan.org/?p=x264.git;a=commit;h=b28d98bbc3174e6f38caea49a7c545204e3930d3

(Fix 10l in weightp on ARM)

Can the relevant updates be pulled in for lucid?

Note that x264 doesn't appear to have any runtime detection for the availability of NEON yet, so 2 flavours will need to be built (in the same way as is currently done for the ffmpeg-debian package). NEON is built in by default unless CFLAGS contains explicit -march, -mfpu options: to get vanilla binaries which will run across all ARMv7 implementations, you would need to configure with CFLAGS='-march=armv7-a' (or change the configure script).

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package x264 - 2:0.85.1442.1+git781d30-1

---------------
x264 (2:0.85.1442.1+git781d30-1) lucid; urgency=low

  * use proper upstream version
    (NB: the first number (85) is the API version,
         the second the number of git commits so far)
  * New upstream version should fix: LP: #315697, #463516
  * Many ARM Neon Optimisations, LP: #388388
  * this package is properly maintained in ubuntu. Indicate this by using
    "-N" as versioning scheme.
  * change versioning scheme as discussed with upstream. Requires bumping
    epoch, though.
  * ensure that MP4 output support is enabled, LP: #464797
 -- Reinhard Tartler <email address hidden> Thu, 18 Feb 2010 07:39:51 +0100

Changed in x264 (Ubuntu):
status: Triaged → Fix Released
Loïc Minier (lool)
summary: - Integrate ARM NEON optimisations
+ Provide a NEON flavor
Changed in x264 (Ubuntu):
status: Fix Released → New
Revision history for this message
Loïc Minier (lool) wrote :

See also bug #524859

Revision history for this message
Loïc Minier (lool) wrote :

Actually, there is no need for a NEON flavor, since NEON is autodetected at runtime in v6t2 mode which is the mode we should use in the Ubuntu build. It's autodetected with SIGILL, is that good enough?

I'm closing this bug since we reworking the default opts is the subject of bug #524859, but that shouldn't impact NEON support (we should switch from -mfpu=neon to v6t2 mode with neon runtime detection with sigill).

Do reopen this bug if you object, particularly if sigill is not an acceptable way of supporting NEON in Ubuntu.

Thanks,

Changed in x264 (Ubuntu):
status: New → Fix Released
Revision history for this message
Loïc Minier (lool) wrote :

I fixed this in the packaging repo; an armv7-a flavor is built by default with the v6t2 opts builtin and NEON is runtime detected with SIGILL; under Debian, a v4t flavor is built by default and an extra NEON flavor with cortex-a8 is also built and shipped in the same package.

Changed in x264 (Ubuntu):
assignee: nobody → Loïc Minier (lool)
Changed in x264 (Ubuntu):
status: Fix Released → Confirmed
Revision history for this message
Dave Martin (dave-martin-arm) wrote :

Hi there, I've just been taking a quick look at this myself... looks like you saw and fixed the same ftbfs I got.

For the NEON autodetection code, I have some concerns:

  * SIGILL may be bad for libx264, and may cause unpredictable results for ths usual reasons, especially if the library may be used in a multithreaded process where the signal handler disposition for SIGILL could be left in an execpected state.

  * Using SIGILL to detect NEON will misdetect in some cases, particularly on the older Babbage boards where we explicitly want to lie about the hardware features. Of less concern to Ubuntu, it will also unpredictably misdetect if the kernel is built with CONFIG_NEON=n but the hardware has NEON (in this configuration, you can get a true or false test result, depending on what code ran since the last context switch).

...so the test for neon should be ported to hwcaps in the usual way, maybe falling back on SIGILL at build time if the right headers and defines aren't available (<linux/auxvec.h> and <asm/hwcap.h> present and HWCAP_NEON defined).

  * The x264_cpu_fast_neon_mrc_test() code is likely to be unsafe -- the read of the performance monitor User Enable Register (PMUSERENR) may cause a SIGILL on Dove and other architecture lisensee platforms (since the performance monitor counters are an optional part of the architecture and may not be present at all). The test will also do nothing useful on most kernels (including lucid imx51), since non-standard patches are required in order to enable userspace access to the performance monitor counters anyway (in any case, there is no protection to stop multiple processes/threads accessing the counters at the same time and causing incorrect results). Note also that although we don't do it for Ubuntu now, the code is supposed to support buliding for pre-v7 architectures, where the PMUSERENR read will likely SIGILL.

This test should be stubbed out and assumed to fail in the Ubuntu build, since currently it always fails on the current imx51 kernel and may SIGILL on Dove (can someone with hardware test it?). Optionally though, we could read /proc/cpuinfo and attempt to make a more intelligent decision based on whether we recognise the CPU implementer/architecture/variant/part/revision fields.

Revision history for this message
Dave Martin (dave-martin-arm) wrote :

Oops, posted to the wrong bug.

See the comments and patches posted here for runtime NEON detection support:

https://bugs.launchpad.net/ubuntu/+source/x264/+bug/524859/comments/6

Revision history for this message
Dave Martin (dave-martin-arm) wrote :
Loïc Minier (lool)
Changed in x264 (Ubuntu):
status: Confirmed → Fix Committed
Revision history for this message
Loïc Minier (lool) wrote :

This upload builds a NEON flavor by default if that's needed, but doesn't actually build a NEON flavor when the toolchain defaults are sufficiently high for v6 since NEON is runtime detected in this case.

x264 (2:0.85.1448+git1a6d32-1) lucid; urgency=low

  [ Loic Minier ]
  * Drop --enable-pic, let's see what breaks, LP: #524859

  [ Reinhard Tartler ]
  * New upstream snapshot, no new features, LP: #526396.
  * remove quilt infrastructure
  * don't set CFLAGS in debian/rules, upstream build system overrides
    this anyways

 -- Reinhard Tartler <email address hidden> Sun, 21 Feb 2010 16:57:21 +0100

Changed in x264 (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.