Closed Bug 880419 Opened 11 years ago Closed 10 years ago

Import updated libyuv for webrtc

Categories

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

22 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jesup, Assigned: jesup)

References

Details

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

Attachments

(5 files, 5 obsolete files)

2.34 MB, patch
Details | Diff | Splinter Review
7.22 KB, patch
Details | Diff | Splinter Review
5.00 KB, patch
glandium
: review+
Details | Diff | Splinter Review
5.41 KB, patch
glandium
: review+
Details | Diff | Splinter Review
2.86 KB, patch
glandium
: review+
Details | Diff | Splinter Review
Since we're updating webrtc, we should update the dependent projects

This updates libyuv to rev719

Note that this will require a license update for src/x86inc.asm
Comment on attachment 759375 [details] [diff] [review]
libyuv import of rev 719 June 06 2013 15:49:00

Note: this doesn't have a few make-related mods we've made applied yet

f? to tim - any issues you can think of?  I can easily switch to a different rev; I did this almost as a proof-of-concept of importing it separately.
Attachment #759375 - Flags: feedback?(tterribe)
builds and runs with these mods of changes to the old rev of libyuv
Attachment #759375 - Flags: feedback?(tterribe) → feedback+
Don't forget to take into account bug 878446 in the rollup mods patch.
Depends on: 880879
Depends on: 959773
Attachment #759375 - Attachment is obsolete: true
Attachment #759447 - Attachment is obsolete: true
Ok, needs an update to GCC (really bintools) from bug 938510 to work on the Linux builders.  I'll disable AVX2 support until that lands; a better solution would be to predicate it on a configure.in check and add a #define to disable AVX2 support (overriding the compiler version checks in row.h).

With AVX2 disabled, green on all Linux builds.
Depends on: 938510
updated to fix build issues with Android 2.x and B2G.  B2G Desktop Linux still fails on configure (can't build executables), probably due to an issue with the GCC update patch (which hasn't landed yet due to a B2G problem).  All other builds are clean.
Attachment #8359979 - Attachment is obsolete: true
Blocks: 813645
Attachment #8360527 - Attachment is obsolete: true
Attachment #8367169 - Flags: review?(mh+mozilla)
Comment on attachment 8367169 [details] [diff] [review]
disable AVX2 asm if the compiler/assembler don't support it

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

::: configure.in
@@ +1359,5 @@
> +                     result="yes", result="no")
> +      AC_MSG_RESULT("$result")
> +      if test "$result" = "yes"; then
> +          HAVE_X86_AVX2=1
> +          AC_DEFINE(HAVE_X86_AVX2)

You don't need the AC_DEFINE.

@@ +1362,5 @@
> +          HAVE_X86_AVX2=1
> +          AC_DEFINE(HAVE_X86_AVX2)
> +          AC_SUBST(HAVE_X86_AVX2)
> +      fi
> +    esac

As I mentioned on irc, it may be desirable to have a configure flag to make configure fail when the test fail, so that we ensure the avx assembly is not silently disabled on e.g. release linux builds (but then we should do the same for the other similar optional instruction sets, so it could be a followup).
Attachment #8367169 - Flags: review?(mh+mozilla) → review+
backed this out on central and integration branches as part of https://tbpl.mozilla.org/?rev=735a648bca0d because of the suspicion of causing bustages on device builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=33874773&tree=Mozilla-Central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
in the update, they added arm_neon_optional to the webrtc common.gypi, but our gyp.mozbuild doesn't define it, so when I moved libyuv out to a standalone, it broken B2G devicebuilds
Comment on attachment 8368600 [details] [diff] [review]
make sure that NEON detection is on for B2G libyuv

https://tbpl.mozilla.org/?tree=Try&rev=48a193ffab42

I'll take any build peer for review....  1-liner fix.  

Try doesn't run device builds, but I ran a Helix local device build (failed without this, works with it)
Attachment #8368600 - Flags: review?(ted)
Attachment #8368600 - Flags: review?(mh+mozilla)
Attachment #8368600 - Flags: review?(gps)
Attachment #8368600 - Attachment is obsolete: true
Attachment #8368600 - Flags: review?(ted)
Attachment #8368600 - Flags: review?(mh+mozilla)
Attachment #8368600 - Flags: review?(gps)
Comment on attachment 8368848 [details] [diff] [review]
make sure that NEON detection is on for B2G libyuv, make sure NEON ifdefs match

Upstream didn't have their ifdefs matching....  All the others use "if !disabled && (defined(__ARM_NEON__) || defined(LIBYUV_NEON))"; the *_neon.cc files left off LIBYUV_NEON
Attachment #8368848 - Flags: review?(mh+mozilla)
Attachment #8369290 - Flags: review?(mh+mozilla)
Comment on attachment 8369290 [details] [diff] [review]
minor tweaks for building Android ARMv6 (we define HAVE_ARM_NEON...)

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

Please fold in the other one.
Attachment #8369290 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8368848 [details] [diff] [review]
make sure that NEON detection is on for B2G libyuv, make sure NEON ifdefs match

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

r+ with the code changes reverted.

::: media/libyuv/source/compare_neon.cc
@@ +14,5 @@
>  namespace libyuv {
>  extern "C" {
>  #endif
>  
> +#if !defined(LIBYUV_DISABLE_NEON) && (defined(__ARM_NEON__) || defined(LIBYUV_NEON))

This is unnecessary. The reason the other parts have defined(LIBYUV_NEON) is because those other parts are *not* built with -mfpu=neon.

::: media/webrtc/signaling/test/Makefile.in
@@ +17,5 @@
>    $(DEPTH)/media/libyuv/libyuv_libyuv/$(LIB_PREFIX)yuv.$(LIB_SUFFIX) \
>    $(DEPTH)/netwerk/srtp/src/$(LIB_PREFIX)nksrtp_s.$(LIB_SUFFIX) \
>    $(NULL)
>  
> +ifdef HAVE_ARM_NEON

BUILD_ARM_NEON
Attachment #8368848 - Flags: review?(mh+mozilla) → review+
Depends on: 1151628
Depends on: 1152016
See Also: → 1282711
See Also: 1282711
See Also: → 1284803
You need to log in before you can comment on or make changes to this bug.