Closed Bug 1157768 Opened 9 years ago Closed 9 years ago

Use ffmpeg FFT on x86 instead of kissfft

Categories

(Core :: Web Audio, defect, P1)

Product:
Core
Component:
Web Audio
Version:
32 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: padenot, Assigned: qdot)

References

(Blocks 4 open bugs)

Details

(Keywords: perf)

Attachments

(4 files, 19 obsolete files)

Patch 3 (v4) - FFTBlock changes for libav FFT; r=padenot
6.22 KB, patch
Details | Diff | Splinter Review
Patch 1 (v5) - Minimal libav files for implementing fft; r=padenot
602.23 KB, patch
Details | Diff | Splinter Review
Patch 2 (v11) - Build files and config headers for libav fft; r=glandium
128.16 KB, patch
Details | Diff | Splinter Review
Patch 4 (v2) - libav FFT dummy functions for Windows PGO/NoOpt builds; r=glandium
2.64 KB, patch
Details | Diff | Splinter Review 
We will be using openmax dl's FFT on ARM (that is hand-optimzed NEON assembly code written by people from ARM), but we need to have a faster FFT on x86 as well. 

ffmpeg has code to perform an FFT, that is reportedly faster than kissfft. It's LGPL-licensed, and easy to import. This is also what Blink uses.
This is a low hanging fruit: it will make the most expensive node much faster.
Priority: -- → P1
Blocks: 1100351
Is this just a matter of switching out the calls in FFTBlock? If so, I think I might give it a shot. Seems like the build integration might be the more difficult part.
Yes, everything you said is right, it's just a matter of switching the calls in FFTBlock, and the build system is going to be the most complicated (although it's easier than ever, these days).
Assignee: nobody → kyle
What are your thoughts about where to put the fft code out of libavcodec? I see we've already got some ffmpeg code for mp4 stuff that late binds to libavcodec when needed, but it seems like that's not going to work for this.
Flags: needinfo?(padenot)
Usually, we put those code imports for the media stack in `media/`. Indeed we already have some code, but not the right bits.

Maybe looking at the build system patches in bug 926838 can help you write the build system bits? It's basically the same thing, but for ARM (importing code, wiring it up to the build system based on the target architecture).
Flags: needinfo?(padenot)
Adding dependency on Bug 1162845, since that'll change how we deal with assembler arguments for the x86 assembly side of avfft.
Depends on: 1162845
Not sure whether avfft is the same code as rdft used in chromium, but when I looked at rdft, it was LGPL.

If it is LGPL only, then it should be linked dynamically, or we need to check whether it needs to be disabled in some builds.

To see what is involved in dynamic linking, the changes in bug 816576 might give some clues to the reverse of the required differences, if they are not obsolete now.
Yeah, avfft is part of libavcodec, which is part of libav, so we're stuck with LGPL (and the chromium rdft is most likely from libav, too). 

Thanks for reminding me, will see if I can get dynamic linking going. I know some of the library build calls have changed recently, so who knows, maybe it'll be even easier.
:padenot, what do we have in the way of vectorized math around gecko? The IFFT in avfft needs scaling to put out what we expect.
Flags: needinfo?(padenot)
Gerv, we're looking at bringing in more libav code to speed up webaudio, and I've been told by almost everyone in the company to ping you. So I'm pinging you. :)

We've already got some bits of libav in the code, but it's all headers. This will require actual compilation units, so I'm working on figuring out how we can keep those dynamically linked.
Flags: needinfo?(gerv)
:qdot: this code is all LGPLed? If so, we can use it, but you need to:

* stick it in its own dynamically linked library (or one with only other LGPLed or permissively-licensed code)
* update the list of LGPL code sources in about:license to reference libav, if it's not in there already

Gerv
Flags: needinfo?(gerv)
It's all LGPL as far as I'm aware, and we've already got it in the license file because we used some libav headers for mp4 work. So it's mainly a matter of figuring out how the dynamic linking thing is going to work.
(clearing NEEDINFO, we chat with Kyle on irc).
Flags: needinfo?(padenot)
(In reply to Gervase Markham [:gerv] from comment #11)
> :qdot: this code is all LGPLed? If so, we can use it, but you need to:
> 
> * stick it in its own dynamically linked library (or one with only other
> LGPLed or permissively-licensed code)

I don't think that's true. AFAIK, LGPL doesn't require this. What it does require is that a modified version of the LGPL code can be relinked, and that everything needed for that is provided. That doesn't prevent proprietary software statically linking LGPL code, as long as they provide e.g. a static library of their proprietary code that can be linked with a modified version of the LGPL code. In our case, we provide the source code for everything, so the requirement is, aiui, already fulfilled by that. No need to specifically dynamically link.
glandium: this is, briefly, how I understand LGPL 2.1 and how it applies to Mozilla.
http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html

We are doing the act mentioned in section 6, i.e. we want to "combine or link a "work that uses the Library" with the Library to produce a work containing portions of the Library, and distribute that work under terms of your choice". So we have to do the stuff in the first para of that section (which we do in about:license), and we also have to do one of a) to e).

a) we don't do this as we don't ship source _along with the binaries_, even taking into account d), and other people who 
   distribute Firefox binaries almost certainly don't either 
b) we do this one - "Use a suitable shared library mechanism for linking with the Library."
c) the written offer is a pain in the ass
d) doesn't always apply for our downloads
e) this won't apply

So because we do b), I tell engineers that if we use LGPLed code, it must be in a dynamically-linked shared library containing only LGPLed code.

Let me know if you think my analysis is wrong.

Gerv
ni'ing myself to look at this again tomorrow.
Flags: needinfo?(mh+mozilla)
My reading of b) is that's not what we do. b) would be us using the system libavwhatever ("uses at run time a copy of the library already present on the user's computer system, rather than copying library functions into the executable").

But rather than section 6, aren't we doing section 4?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(gerv)
(In reply to Mike Hommey [:glandium] from comment #17)
> My reading of b) is that's not what we do. b) would be us using the system
> libavwhatever ("uses at run time a copy of the library already present on
> the user's computer system, rather than copying library functions into the
> executable").

I think that is just a definition of static vs. dynamic linking; it's not talking about "shipped with OS" vs. "shipped with your product".

> But rather than section 6, aren't we doing section 4?

No, because we need the right to "distribute that work under terms of [our] choice", which is only available under section 6. Firefox builds as a whole are distributed under the MPL.

Also, looking at section 4, we don't ship source with the product, and it's arguable that we don't offer access to copy "from the same place". Hg wouldn't count; sourceballs on our download servers probably would, but we don't have a "Download source" link next to the "Download" links on https://www.mozilla.org/firefox/all/ or https://www.mozilla.org/firefox/new/ .

Gerv
Flags: needinfo?(gerv)
(In reply to Gervase Markham [:gerv] from comment #18)
> I think that is just a definition of static vs. dynamic linking; it's not
> talking about "shipped with OS" vs. "shipped with your product".

With the fact that it talks about "suitable shared library mechanism" and "already present on the user's computer", I think it sounds more about "shipped with OS".

LGPL 2.1 appears to be a PITA when you want to pick small parts out of a library. LGPL 3.0 seems to make it even worse. Seems to me (but ianal, etc.) 6c is the closest to what we could do, and well, it's the written offer thing... with terms written in an age where internet was not omnipresent.

How big is the fmpeg fft source code? How hard would it be to track down the authors and ask for dual licensing or something?
(In reply to Paul Adenot (:padenot) from comment #0)
> ffmpeg has code to perform an FFT, that is reportedly faster than kissfft.
> It's LGPL-licensed, and easy to import. This is also what Blink uses.

Wait a second. Blink uses it? So Chrome uses it? Smells like LGPL infringement to me. Or they have a different license for that code.
Ah, but maybe blink is largely LGPL by way of being originally derived from KHTML, I guess. But I've never seen any section 6 thing for Chrome...
(In reply to Mike Hommey [:glandium] from comment #19)
> With the fact that it talks about "suitable shared library mechanism" and
> "already present on the user's computer", I think it sounds more about
> "shipped with OS".

I just checked the GPL FAQ:
http://www.gnu.org/licenses/gpl-faq.html#LGPLStaticVsDynamic
It seems that your interpretation is the one the FSF uses. However, at least one person:
http://teem.sourceforge.net/lgpl.html
thinks differently, see 4.2 at the very bottom of the page. Still, we should go with the FSF interpretation.

Another reason to do LGPL-only in a shared library seems to be section 7, which has some requirements which are better avoided.

I've filed bug 1167570 to improve the way we provide source.

Gerv
Yeah, 7a seems like a PITA. In fact, it sounds as if the only case it's trying to cover is closed-source software using the LGPL code.
Do we currently have LGPL-only code in Firefox, or is everything multi-licensed?
We currently have LGPL-only libraries, IIRC. See the list at the top of the LGPL section in about:license. libsoundtouch and libav.

Gerv
aiui, we're not using libav, only its headers. As for libsoundtouch, it's currently linked into libxul...
There is a license exception for SoundTouch.
Blocks: webaudioperf

        Attachment #8620610 -
        Attachment is obsolete: true

        Attachment #8620611 -
        Attachment is obsolete: true

    
    

    
  
Ok. I now have this building on windows 32/64-bit, mac universal, and linux 64-bit. The one platform I'm stuck on is linux 32-bit. Here's a try that at least covers the builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63caab31404e

The problem with Linux 32-bit is the "read-only segment has dynamic relocation" linking issue. This is happening somewhere in libavcodec/x86/fft.asm. Rather than block everything while I root through that and also try to figure out if this is a wrong yasm argument or what, I'm going to work on getting the current patches cleaned up and into review, and we'll punt linux 32-bit until the rest of this is landed in order to try to make the 41 release.

    
    

    
  
It's 2015, 32bit Linux should not block the landing of this. We should just simply disable it at build time for now. People that run 32bits Linux probably don't have machines powerful enough to compute FFTs anyways.

    
    

    
  
Ok, got a completely clean try with libav's fft running on mac, linux, and windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff1e3c9adf50

I'll put the initial libav file checking and the changes to FFTBlock in for review now. I need to do a bit more cleaning on the build system files, but will hopefully have those in monday or tuesday. Just going to hope people will have time during Whistler to take a look.

    
    

    
  


  

        Attachment #8623906 -
        Attachment is obsolete: true

        Attachment #8625013 -
        Flags: review?(padenot)

    
    

    
  


  
All of the ifdef's around FFTBlock are getting a little messy, but hopefully this will be the end of it. If things get more complicated (for some reason we want to go fftw or iml or whatever at some point), might be worth splitting the functions into their own headers.

        Attachment #8623908 -
        Attachment is obsolete: true

        Attachment #8625015 -
        Flags: review?(padenot)

    
    

    
  
Comment on attachment 8625014 [details] [diff] [review]
Patch 2 (v3) - WIP: Build files and config headers for libav fft

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

Have you had a build peer look at this ?

::: configure.in
@@ +6286,5 @@
> +    MOZ_LIBAV_FFT=1
> +  ;;
> +esac
> +
> +dnl Don't build libav fft on x86 android

Why is that?

    
    

    
  
Comment on attachment 8625015 [details] [diff] [review]
Patch 3 (v3) - FFTBlock changes for libav FFT

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

Yeah it's a bit of a mess. We can file a followup to split those into three cpp files with the conditionals in the build system ?

::: dom/media/webaudio/FFTBlock.h
@@ +79,5 @@
>    {
>      EnsureFFT();
> +#if defined(MOZ_LIBAV_FFT)
> +    AlignedTArray<FFTSample> complex(mFFTSize);
> +    memcpy(complex.Elements(), aData, sizeof(FFTSample) * mFFTSize);

Prefer PodCopy (In PodOperations.h), please (in all the file of course).

@@ +112,5 @@
> +      memcpy(aDataOut, (float*)mOutputBuffer.Elements(), sizeof(float) * mFFTSize);
> +      av_rdft_calc(mAvIRDFT, aDataOut);
> +      // TODO: There has to be a way to vectorize this.
> +      for (uint32_t i = 0; i < mFFTSize; ++i) {
> +        aDataOut[i] *= 2.0;

Where does this magic 2.0 come from ? Also you can reference bug 877662 that will implement the vector operations for x86.

@@ +149,5 @@
> +    memcpy(aRealDataOut, inputBuffer.Elements(), sizeof(FFTSample) * FFTSize());
> +    // TODO: There has to be a way to vectorize this.
> +    for (uint32_t i = 0; i < mFFTSize; ++i) {
> +      aRealDataOut[i] /= mFFTSize;
> +    }

Again, you can reference bug 877662.

        Attachment #8625015 -
        Flags: review?(padenot) → review+

    
    

    
  
Comment on attachment 8625013 [details] [diff] [review]
Patch 1 (v3) - Minimal libav files for implementing fft

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

I see some cpuid things in there, looks like this is doing CPU capabilities detection? Are we also detecting SSE support at compile time ? I'm fine with just falling back to kissfft for machines that can't build SSE code.

Also this really needs a build peer review, and an addition in about:license.

        Attachment #8625013 -
        Flags: review?(padenot)

        Attachment #8625013 -
        Flags: review?(mh+mozilla)

        Attachment #8625013 -
        Flags: review+

    
    

    
  
(In reply to Paul Adenot (:padenot) from comment #41)
> > +
> > +dnl Don't build libav fft on x86 android
> 
> Why is that?

Sorry, should've made that comment more descriptive. Android is missing certain headers I was expecting, and I wasn't really aware of where we were using x86 android outside of testing? I can file a followup to try and get it working if need be.

> Yeah it's a bit of a mess. We can file a followup to split those into three
> cpp files with the conditionals in the build system ?

Yeah, that's similar to what blink does, comes out cleaner. Will file followup.

> @@ +112,5 @@
> > +      memcpy(aDataOut, (float*)mOutputBuffer.Elements(), sizeof(float) * mFFTSize);
> > +      av_rdft_calc(mAvIRDFT, aDataOut);
> > +      // TODO: There has to be a way to vectorize this.
> > +      for (uint32_t i = 0; i < mFFTSize; ++i) {
> > +        aDataOut[i] *= 2.0;
> 
> Where does this magic 2.0 come from ? Also you can reference bug 877662 that
> will implement the vector operations for x86.

For some reason libav's FFT hands us back half of what we expect. I'll comment that just to make it obvious.

> I see some cpuid things in there, looks like this is doing CPU capabilities
> detection? Are we also detecting SSE support at compile time ? I'm fine with
> just falling back to kissfft for machines that can't build SSE code.

It does capability detection, and I just compile in everything by default. I'll file a followup about doing fallback also.

> Also this really needs a build peer review, and an addition in about:license.

All of the build stuff is in Patch 2, which is still WIP. I'm hoping to have that done in the next couple of days, and it'll go to both you and a build peer because it's pretty nasty, since it involves a new library and all. 

As for about:license, I believe we're already covered, since we already had libav code in for mp4 stuff and had to include the LPGL then.

    
    

    
  


  
Ok, here's the patch with all of the build system changes and lgpl library addition for libav's fft. The main messy point here is the config headers. I tried to reduce the amount of repetition at least.

        Attachment #8625014 -
        Attachment is obsolete: true

        Attachment #8625384 -
        Flags: review?(padenot)

        Attachment #8625384 -
        Flags: review?(mh+mozilla)

    
    

    
  


  
Review comments addressed. Moved to PodCopy, added more comments.

        Attachment #8625015 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8625384 [details] [diff] [review]
Patch 2 (v4) - Build files and config headers for libav fft

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

::: b2g/installer/package-manifest.in
@@ +131,5 @@
> +#ifdef MOZ_LIBAV_FFT
> +#ifdef MOZ_FOLD_LIBS
> +@BINPATH@/@DLL_PREFIX@lgpllibs@DLL_SUFFIX@
> +#else
> +@BINPATH@/@DLL_PREFIX@avfft@DLL_SUFFIX@

No need to ever try to build this separately. MOZ_FOLD_LIBS is special, and only exists because nss and nspr *need* to not be folded on linux.

::: config/external/lgpllibs/Makefile.in
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +include $(topsrcdir)/config/config.mk

this include should not be needed, if I'm not mistaken

@@ +4,5 @@
> +
> +include $(topsrcdir)/config/config.mk
> +
> +ifeq ($(OS_ARCH),WINNT)
> +DEFFILE = $(topsrcdir)/config/external/lgpllibs/lgpllibs.def

DEFFILE should be defined in moz.build

::: config/external/lgpllibs/moz.build
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +if CONFIG['MOZ_LIBAV_FFT']:
> +    DIRS += ['../../../media/libav']

DIRS += [
    '/media/libav',
]

::: configure.in
@@ +6281,5 @@
> +case "$CPU_ARCH" in
> +  x86)
> +    MOZ_LIBAV_FFT=1
> +  ;;
> +  x86_64)

You can group both with x86|x86_64)

@@ +6295,5 @@
> +
> +dnl don't build libav on 32-bit linux, won't compile yet
> +if test "${OS_TARGET}" = "Linux" -a "${CPU_ARCH}" = "x86"; then
> +  MOZ_LIBAV_FFT=
> +fi

Usually, we'd not set it to 1 in the first place, so moving the tests inside the case would be better.

@@ +6299,5 @@
> +fi
> +
> +MOZ_ARG_DISABLE_BOOL(libavfft,
> +[ --disable-libavfft  Disable optimized libav fft routines (x86 32/64-bit only)],
> +    MOZ_LIBAV_FFT=)

Do we really want to expose an option for this?

@@ +6368,5 @@
> +elif test -n "$MOZ_LIBAV_FFT"; then
> +    dnl Warn if we're not building the optimized routines, even though the user
> +    dnl didn't specify --disable-libav-fft.
> +    AC_MSG_WARN([No assembler or assembly support for libav-fft.  Using unoptimized C routines.])
> +fi

I'm torn between asking you to go through the pain of factoring all the above because of all the repetition with libjpeg-turbo and libvpx or leaving it alone.

::: media/libav/config_common.asm
@@ +1,1 @@
> +%define ARCH_AARCH64 0

For whoever updates in the future, it would be useful to indicate how this was generated. Same for the other config files.

::: media/libav/moz.build
@@ +8,5 @@
> +    SharedLibrary('avfft')
> +    SHARED_LIBRARY_NAME = 'avfft'
> +    if CONFIG['OS_ARCH'] == 'Linux':
> +        # Need global symbols, otherwise generated cos/sin tables can't be found
> +        LDFLAGS += ['-Wl,-Bsymbolic']

This doesn't do what you think it does. Please remove this.

@@ +12,5 @@
> +        LDFLAGS += ['-Wl,-Bsymbolic']
> +        if CONFIG['GCC_USE_GNU_LD']:
> +            LD_VERSION_SCRIPT = 'libav-processed.def'
> +
> +DIRS = ['libavcodec', 'libavutil', 'libavutil/x86']

Considering the number of sources involved, just put everything in this moz.build.

        Attachment #8625384 -
        Flags: review?(mh+mozilla) → review-

    
    

    
  
Comment on attachment 8625013 [details] [diff] [review]
Patch 1 (v3) - Minimal libav files for implementing fft

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

The moz.builds from this patch should go in the other patch.

        Attachment #8625013 -
        Flags: review?(mh+mozilla)

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #47)
> Comment on attachment 8625384 [details] [diff] [review]
> Patch 2 (v4) - Build files and config headers for libav fft
> 
> 
> @@ +6299,5 @@
> > +fi
> > +
> > +MOZ_ARG_DISABLE_BOOL(libavfft,
> > +[ --disable-libavfft  Disable optimized libav fft routines (x86 32/64-bit only)],
> > +    MOZ_LIBAV_FFT=)
> 
> Do we really want to expose an option for this?

I wasn't sure if it mattered or not since there may be some case where people don't want to build LGPL libs? Will option for now and just implement if that actually becomes the case at some point.

> 
> @@ +6368,5 @@
> > +elif test -n "$MOZ_LIBAV_FFT"; then
> > +    dnl Warn if we're not building the optimized routines, even though the user
> > +    dnl didn't specify --disable-libav-fft.
> > +    AC_MSG_WARN([No assembler or assembly support for libav-fft.  Using unoptimized C routines.])
> > +fi
> 
> I'm torn between asking you to go through the pain of factoring all the
> above because of all the repetition with libjpeg-turbo and libvpx or leaving
> it alone.

How about I file a followup and deal with it after this lands? I've got more build system work to do with bug 1176300 anyways, so I'll still be around the code. We'd just like to make sure this patch gets into firefox 41.

> ::: media/libav/moz.build
> @@ +8,5 @@
> > +    SharedLibrary('avfft')
> > +    SHARED_LIBRARY_NAME = 'avfft'
> > +    if CONFIG['OS_ARCH'] == 'Linux':
> > +        # Need global symbols, otherwise generated cos/sin tables can't be found
> > +        LDFLAGS += ['-Wl,-Bsymbolic']
> 
> This doesn't do what you think it does. Please remove this.

Ok, so there's two reasons this is here:

- I was trying to basically replicate what libav uses for its compiles
- If I remove it, I'll get this linker error:

 0:14.50 Executing: /usr/bin/ccache c++ -Wall -Wempty-body -Woverloaded-virtual -Wsign-compare -Wwrite-strings -Wno-invalid-offsetof -Wcast-align -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DDEBUG -DTRACING -g -fno-omit-frame-pointer -fPIC -shared -Wl,-z,defs -Wl,-h,liblgpllibs.so -o liblgpllibs.so -lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -B ../../../build/unix/gold -Wl,-rpath-link,/share/code/mozbuild/gecko-hg/obj-ff-rel-gcc/dist/bin -Wl,-rpath-link,/usr/local/lib /share/code/mozbuild/gecko-hg/obj-ff-rel-gcc/config/external/lgpllibs/tmp2bnWfp.list -ldl
 0:14.50 /share/code/mozbuild/gecko-hg/obj-ff-rel-gcc/config/external/lgpllibs/tmp2bnWfp.list:
 0:14.50     INPUT("../../../media/libav/libavcodec/avfft.o")
 0:14.50     INPUT("../../../media/libav/libavcodec/fft_fixed.o")
 0:14.50     INPUT("../../../media/libav/libavcodec/fft_float.o")
 0:14.50     INPUT("../../../media/libav/libavcodec/rdft.o")
 0:14.50     INPUT("../../../media/libav/libavcodec/fft_init.o")
 0:14.51     INPUT("../../../media/libav/libavcodec/fft.o")
 0:14.51     INPUT("../../../media/libav/libavutil/avstring.o")
 0:14.51     INPUT("../../../media/libav/libavutil/cpu.o")
 0:14.51     INPUT("../../../media/libav/libavutil/dict.o")
 0:14.51     INPUT("../../../media/libav/libavutil/error.o")
 0:14.51     INPUT("../../../media/libav/libavutil/eval.o")
 0:14.51     INPUT("../../../media/libav/libavutil/file.o")
 0:14.51     INPUT("../../../media/libav/libavutil/file_open.o")
 0:14.51     INPUT("../../../media/libav/libavutil/intmath.o")
 0:14.51     INPUT("../../../media/libav/libavutil/log.o")
 0:14.51     INPUT("../../../media/libav/libavutil/log2_tab.o")
 0:14.51     INPUT("../../../media/libav/libavutil/mathematics.o")
 0:14.51     INPUT("../../../media/libav/libavutil/mem.o")
 0:14.51     INPUT("../../../media/libav/libavutil/opt.o")
 0:14.51     INPUT("../../../media/libav/libavutil/parseutils.o")
 0:14.51     INPUT("../../../media/libav/libavutil/random_seed.o")
 0:14.51     INPUT("../../../media/libav/libavutil/rational.o")
 0:14.51     INPUT("../../../media/libav/libavutil/sha.o")
 0:14.51     INPUT("../../../media/libav/libavutil/x86/cpu.o")
 0:14.51     INPUT("../../../media/libav/libavutil/x86/cpuid.o")
 0:14.51 
 0:14.51 ../../../build/unix/gold/ld: error: /share/code/mozbuild/gecko-hg/obj-ff-rel-gcc/config/external/lgpllibs/../../../media/libav/libavcodec/fft.o: requires dynamic R_X86_64_PC32 reloc against 'ff_cos_32' which may overflow at runtime; recompile with -fPIC
 0:14.51 ../../../build/unix/gold/ld: error: read-only segment has dynamic relocations
 0:14.51 collect2: error: ld returned 1 exit status
 0:14.51 make[5]: *** [liblgpllibs.so] Error 1
 0:14.51 make[4]: *** [config/external/lgpllibs/target] Error 2
 0:14.51 make[4]: *** Waiting for unfinished jobs....

So, I need to figure out another way to make ff_cos_32 visible, but the whole -Bsymbolic thing is probably overkill and apparently dangerous. Any thoughts on how to fix this?

> 
> @@ +12,5 @@
> > +        LDFLAGS += ['-Wl,-Bsymbolic']
> > +        if CONFIG['GCC_USE_GNU_LD']:
> > +            LD_VERSION_SCRIPT = 'libav-processed.def'
> > +
> > +DIRS = ['libavcodec', 'libavutil', 'libavutil/x86']
> 
> Considering the number of sources involved, just put everything in this
> moz.build.

So the reason this happened was due to there being two cpu.c files. Since we can't have the same file in the same sources list twice, and I was doing my best not to change the original files, I put them in different directories. I suppose I could just rename one of them, and put the instructions on that in a README along with the instructions on how the config files were generated?
Flags: needinfo?(mh+mozilla)

    
    

    
  
Ok, actually, I found a better fix for the -Bsymbolic. I can use --dynamic_list and just expose _ff_cos_32, which seems to make things compile. So now, the moz.build file has:

    if CONFIG['OS_ARCH'] == 'Linux':
        # Need global symbols, otherwise generated cos/sin tables can't be found
        LDFLAGS += ['-Wl,--dynamic-list', '-Wl,%s/media/libav/libav_dynamic_list' % TOPSRCDIR]

Is that acceptable over -Bsymbolic, since it provides granularity over just marking all of the symbols?

    
    

    
  


  
Updating patch 1 to remove build files.

        Attachment #8625013 -
        Attachment is obsolete: true

    
    

    
  


  
Updated patch with review comments addressed.

        Attachment #8625384 -
        Attachment is obsolete: true

        Attachment #8625384 -
        Flags: review?(padenot)

        Attachment #8625967 -
        Flags: review?(mh+mozilla)

    
    

    
  
Comment on attachment 8625967 [details] [diff] [review]
Patch 2 (v5) - Build files and config headers for libav fft

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

::: config/external/lgpllibs/Makefile.in
@@ +1,4 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +

Turns out you do need the config.mk include here in the end (for LD_VERSION_SCRIPT)

@@ +9,5 @@
> +
> +# Convert to the format we need for ld.
> +$(LD_VERSION_SCRIPT): $(topsrcdir)/config/external/lgpllibs/lgpllibs.def
> +	@$(call py_action,convert_def_file, \
> +	  $(DEFINES) $(ACDEFINES) $(XULPPFLAGS) -o $@ $^)

Since you don't have any preprocessing in that def file, you don't need $(DEFINES) $(ACDEFINES) $(XULPPFLAGS)

::: config/external/lgpllibs/moz.build
@@ +13,5 @@
> +    SHARED_LIBRARY_NAME = 'lgpllibs'
> +    DEFFILE = SRCDIR + '/lgpllibs.def'
> +    if CONFIG['OS_ARCH'] == 'Linux':
> +        # Need ff_cos_32
> +        LDFLAGS += ['-Wl,--dynamic-list', '-Wl,%s/media/libav/libav_dynamic_list' % TOPSRCDIR]

Turns out you only need this because you don't have a LD_VERSION_SCRIPT set alongside DEFFILE. So adding LD_VERSION_SCRIPT in this file and a config.mk include in the corresponding Makefile makes it build without --dynamic-list for me.

@@ +17,5 @@
> +        LDFLAGS += ['-Wl,--dynamic-list', '-Wl,%s/media/libav/libav_dynamic_list' % TOPSRCDIR]
> +    # Darwin 32-bit has problems with relocs in the libav FFT code.
> +    # See http://stackoverflow.com/questions/6650178/illegal-text-reloc-to-non-lazy-ptr-error-while-building-in-xcode-4-with-libav-l
> +    if CONFIG['MOZ_LIBAV_FFT'] and CONFIG['OS_ARCH'] == 'Darwin' and CONFIG['CPU_ARCH'] == 'x86':
> +        LDFLAGS += ['-read_only_relocs suppress']

And I guess this is a similar story on mac. Now, looking a bit around, I'd say the following should work:
- remove -read_only_relocs suppress
- remove NO_VISIBILITY_FLAGS = True for libav files
- replace the "avfft.h" include in avfft.c with "libavcodec/avfft.h". Ideally this would be upstreamed (and be done for all headers).

Note that with the above, you don't really need LD_VERSION_SCRIPT (except a few more symbols are exported).

::: configure.in
@@ +6280,5 @@
> +dnl Turn on libav-fft for x86 based platforms by default. Don't build libav on
> +dnl 32-bit linux, won't compile yet. Also, don't build libav fft on x86 android,
> +dnl it's not anything we'll probably use and it's missing some system headers.
> +
> +if test "${CPU_ARCH}" = "x86" -o "${CPU_ARCH}" = "x86_64" ; then

You could still have used a case.

::: media/libav/Makefile.in
@@ +5,5 @@
> +
> +AS=$(LIBAV_FFT_AS)
> +ASM_SUFFIX=asm
> +
> +include $(topsrcdir)/config/rules.mk

You don't need this include.

::: media/libav/libavcommon.mozbuild
@@ +4,5 @@
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# Couldn't find a good define that would tell us if we're 32-bit x86, so making
> +# one here.

Since the code is x86/x86-64 only you could get away with it with HAVE_64BIT_BUILD. Otherwise, we just tend to use compiler macros (__i386__ for gcc, _M_IX86 for msvc, but I don't know if yasm has those)

@@ +13,5 @@
> +# Fix inline symbols and math defines for windows.
> +if CONFIG['OS_ARCH'] == 'WINNT':
> +    DEFINES['_USE_MATH_DEFINES'] = True
> +    DEFINES['inline'] = "__inline"
> +    DEFINES['snprintf'] = "_snprintf"

If you use the bundled snprintf, why do you need this define?

::: media/libav/libavutil/x86/Makefile.in
@@ +2,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +AS=$(LIBJPEG_TURBO_AS)

libjpeg?

::: media/libav/moz.build
@@ +45,5 @@
> +]
> +
> +# VS2013 would cover us for snprintf and strtod, but unfortunately libav
> +# includes linking to these for anything that exposes MSCVER, so we're stuck
> +# using their implementation for now.

Do you mean there's a

#ifdef _MSC_VER // Any version

around uses of plain snprintf/strtod?

::: toolkit/library/moz.build
@@ +112,5 @@
> +# link to lgpllibs everywhere we don't use MOZ_FOLD_LIBS.
> +if CONFIG['MOZ_LIBAV_FFT']:
> +    USE_LIBS += [
> +        'lgpllibs',
> +    ]

Not that it matters much, but if you want to avoid the if here now, you can declare an empty lgpllibs Library in the non-MOZ_LIBAV_FFT case in config/external/lgpllibs/moz.build.

        Attachment #8625967 -
        Flags: review?(mh+mozilla) → review-

    
    

    
  
> Note that with the above, you don't really need LD_VERSION_SCRIPT (except a
> few more symbols are exported).

You're right, making the header change works and gets rid of a ton of the complexity. I'll put a patch in the tree for right now to show our changes, and see about getting it upstreamed.

Since we're changing code now, should I go ahead and prefix exported symbols for windows in the header file instead of going the def file route?

> Since the code is x86/x86-64 only you could get away with it with
> HAVE_64BIT_BUILD. Otherwise, we just tend to use compiler macros (__i386__
> for gcc, _M_IX86 for msvc, but I don't know if yasm has those)

Actually, I just noticed we have -D__x86_64__ in our AS_FLAGS in configure.in, so between that and the gcc/msvc/etc compiler flags, we don't need MOZ_X86.

> Do you mean there's a
> 
> #ifdef _MSC_VER // Any version
> 
> around uses of plain snprintf/strtod?

Sorta, yes. https://github.com/qdot/gecko-hg/blob/1157768-libav-fft/media/libav/libavutil/internal.h#L137

> Not that it matters much, but if you want to avoid the if here now, you can
> declare an empty lgpllibs Library in the non-MOZ_LIBAV_FFT case in
> config/external/lgpllibs/moz.build.

Gonna go ahead and do this, since I was just going to have to undo it in bug 1173600 anyways.

    
    

    
  
Whoops, forgot to ni? glandium on last comment. See Comment #54 about def file.

    
    

    
  
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #54)
> > Note that with the above, you don't really need LD_VERSION_SCRIPT (except a
> > few more symbols are exported).
> 
> You're right, making the header change works and gets rid of a ton of the
> complexity. I'll put a patch in the tree for right now to show our changes,
> and see about getting it upstreamed.
> 
> Since we're changing code now, should I go ahead and prefix exported symbols
> for windows in the header file instead of going the def file route?

I think we should only patch for things that have a remote chance of being upstreamed, and I don't think that does.

> > Do you mean there's a
> > 
> > #ifdef _MSC_VER // Any version
> > 
> > around uses of plain snprintf/strtod?
> 
> Sorta, yes.
> https://github.com/qdot/gecko-hg/blob/1157768-libav-fft/media/libav/
> libavutil/internal.h#L137

So, another way we could deal with that is to patch that ifdef to also check for the value of MSC_VER, and upstream it (although, upstream, that might require a bit more changes not to build snprintf/strtod).

Something like:

#if defined(_MSC_VER) && _MSC_VER < 1900
Flags: needinfo?(mh+mozilla)

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #53)
> And I guess this is a similar story on mac. Now, looking a bit around, I'd
> say the following should work:
> - remove -read_only_relocs suppress
> - remove NO_VISIBILITY_FLAGS = True for libav files
> - replace the "avfft.h" include in avfft.c with "libavcodec/avfft.h".
> Ideally this would be upstreamed (and be done for all headers).

Talked to glandium about this on IRC, but just to keep the bug up to date...

While adding the include to the header at least cleaned up things on linux, if I remove -read_only_relocs suppress, I still get the relocs error in 32-bit OS X.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42ea13a939ce

Readding that argument gets things slightly farther, but then I'm running into visibility issues after removing NO_VISIBILITY_FLAGS = True on 64-bit OS X.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e15589f2f556

What's our mechanism for symbol visibility of OS X, since afaict in configure.in, it doesn't look like we use system headers there?
Flags: needinfo?(mh+mozilla)

    
    

    
  
I think it's time to try to use the same thing on osx and linux (wrappers). But that a) needs careful consideration and testing and b) would not be a good candidate for uplifing.

So in the meanwhile, here's a hackish idea:
create a header that essentially does:
  #pragma GCC visibility push(default)
  #include "avfft.h"
  #pragma GCC visibility pop

And add ['-include', 'that-header.h'] to CXXFLAGS.

(that could even work on linux instead of patching + system-headers)

But that's only for the visibility issue. I'm still looking at the 32-bits OSX failure.
Flags: needinfo?(mh+mozilla)

    
    

    
  
Unrelated note: in comment 53, I did mean an empty "Library", not an empty "SharedLibrary".

    
    

    
  
And the reason this fails to build on 32-bits OSX is the exact same that made you disable MOZ_LIBAV_FFT on 32-bits Linux: the x86 assembly ends up with text relocations, and we don't want those.

    
    

    
  
What "-read_only_relocs suppress" does is to allow text relocations, and that's not really desirable.

Essentially, the problem is that many instructions in fft.asm are accessing variables from the same file in 
SSE or AVX instructions. On x86-64, those end up being direct reads with PC-relative addresses, which is fine, but on x86, PC-relative addresses are not possible. The assembly would need to be modified to be position independent on x86. Or left alone and disabled.

    
    

    
  
Ok, so I'll disable this on 32-bit linux and OS X, and add 32-bit OS X to our followup on this, since we may try adding Accelerate/VecLib support at some point which might fix that there.

As for the includes... First off, I talked to people again yesterday (sorry, forgot to loop you in on the email), and we're no longer aiming for uplift currently. However, how long do you think the wrapper implementation/testing on OS X would take? I would still like to get this landed soon just so we can start running it through more widespread perf testing, but if getting headers right only adds, say, a couple of days, I'm fine filing a followup and waiting for that fix before landing this.
Flags: needinfo?(mh+mozilla)

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #61)
> What "-read_only_relocs suppress" does is to allow text relocations, and
> that's not really desirable.
> 
> Essentially, the problem is that many instructions in fft.asm are accessing
> variables from the same file in 
> SSE or AVX instructions. On x86-64, those end up being direct reads with
> PC-relative addresses, which is fine, but on x86, PC-relative addresses are
> not possible. The assembly would need to be modified to be position
> independent on x86. Or left alone and disabled.

Also, just out of curiosity, why does this still compile/work on x86 windows then?

    
    

    
  
> However, how long do you think the wrapper implementation/testing on OS X would take?

I'll give it a spin on try today, if things go smoothly, that can be done quickly.

> Also, just out of curiosity, why does this still compile/work on x86 windows then?

Because windows only does text relocations.
Flags: needinfo?(mh+mozilla)

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #64)
> > However, how long do you think the wrapper implementation/testing on OS X would take?
> 
> I'll give it a spin on try today, if things go smoothly, that can be done
> quickly.

Considering I haven't got a green try build yet despite having built successfully locally, I'm inclined to think this is likely to a) take more time than expected b) be backed out because it breaks local builds for various developers.
So I wouldn't hold my breath waiting for this.

    
    

    
  
Ok, well, thanks for trying. Will go with permission header hack for the time being.

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #59)
> Unrelated note: in comment 53, I did mean an empty "Library", not an empty
> "SharedLibrary".

Er, well, this runs us into another problem. If I use an empty static library, that means my library sets differ between 32-bit and 64-bit OS X, which means I get dependentlib list generation failures on universal builds.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d156fa194ed

Would an empty dynamic lib on 32-bit linux/darwin be ok for now? libsoundtouch should be moving into lgpllibs on all platforms hopefully quickly after this lands, so it won't be empty for long. If we don't want it empty ever, I can just plan on landing this and bug 1176300 together.
Flags: needinfo?(mh+mozilla)

    
    

    
  
As long as we don't end up shipping an empty liblgplglibs, that's fine. Alternatively, you could get 1176300 in first.
Flags: needinfo?(mh+mozilla)
No longer blocks: 1176300
Depends on: 1176300

    
    

    
  
Dependency order change. Now integrating libsoundtouch first, so we build lgpllibs everywhere, then adding libav in.

    
    

    
  


  
Ok, all of the lgpllibs initial setup happens in bug 1176300, so this is now just libav additions.

        Attachment #8628629 -
        Attachment is obsolete: true

        Attachment #8628630 -
        Flags: review?(mh+mozilla)

    
    

    
  
Comment on attachment 8628630 [details] [diff] [review]
Patch 2 (v7) - Build files and config headers for libav fft

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

::: configure.in
@@ +6288,5 @@
> +  x86|x86_64)
> +    if test "${OS_TARGET}" != "Linux" -o "${CPU_ARCH}" != "x86" ; then
> +       if test "${OS_TARGET}" != "Darwin" -o "${CPU_ARCH}" != "x86" ; then
> +          if test "${OS_TARGET}" != "Android" ; then
> +             MOZ_LIBAV_FFT=1

This would all be much simpler if you just said you build it on Windows x86 and non-android x86-64.

::: media/libav/README_MOZILLA
@@ +11,5 @@
> +files to reduce repeated entries. config_common.h contains package and
> +architecture configuration information, and are used on all platforms.
> +Platform specific config files (config_win.h/asm, config_unix.h/asm,
> +etc) contain platform specific header information. .asm files should
> +match their .h counterparts.

Note that is if you used a script to do that, it would be better to include it.

::: media/libav/config.asm
@@ +17,5 @@
> +  %include "config_win.asm"
> +%elifdef MACHO
> +	%include "config_darwin.asm"
> +%elifdef ELF
> +	%include "config_unix.asm"

Note that you could just add a conditional  -P in moz.build instead of creating a config.asm file just for that kind of dispatch.

::: media/libav/libavcommon.mozbuild
@@ +9,5 @@
> +    DEFINES['_USE_MATH_DEFINES'] = True
> +    DEFINES['inline'] = "__inline"
> +    # libav requires us to compile in its strtod/snprintf via /EXPORT pragmas,
> +    # but still tries to use VS2013 afterward for some reason. This fixes the
> +    # resulting link errors.

I'm having a hard time parsing this paragraph. Could you rephrase?

::: media/libav/moz.build
@@ +48,5 @@
> +# the lgpl shared library, since it does not yet use system headers. This is
> +# also used on linux for the time being, to avoid having to patch libav code.
> +#
> +# TODO: Remove header and patch libav once OS X supports system headers
> +if CONFIG['OS_ARCH'] == 'Darwin' or CONFIG['OS_ARCH'] == 'Linux':

if CONFIG['OS_ARCH'] != 'WINNT' would be shorter.

@@ +58,5 @@
> +if CONFIG['OS_ARCH'] == 'WINNT':
> +    SOURCES += [
> +        'compat/msvcrt/snprintf.c',
> +        'compat/strtod.c'
> +    ]

The suggestion in comment 56 doesn't work?

        Attachment #8628630 -
        Flags: review?(mh+mozilla) → feedback+

    
    

    
  


  
Removing windows compat files

        Attachment #8625965 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #72)

> ::: media/libav/README_MOZILLA
> @@ +11,5 @@
> > +files to reduce repeated entries. config_common.h contains package and
> > +architecture configuration information, and are used on all platforms.
> > +Platform specific config files (config_win.h/asm, config_unix.h/asm,
> > +etc) contain platform specific header information. .asm files should
> > +match their .h counterparts.
> 
> Note that is if you used a script to do that, it would be better to include
> it.

The config change was by hand, but I don't think it'll need to be done often, if ever again. We'll just need to possibly flip on whatever new parts of libav we might be building.

> @@ +58,5 @@
> > +if CONFIG['OS_ARCH'] == 'WINNT':
> > +    SOURCES += [
> > +        'compat/msvcrt/snprintf.c',
> > +        'compat/strtod.c'
> > +    ]
> 
> The suggestion in comment 56 doesn't work?

I was trying to stay out of changing code and putting up patches, but I made this change and it worked, so I've attached the diff to patch 2.

    
    

    
  
Comment on attachment 8630885 [details] [diff] [review]
Patch 2 (v9) - Build files and config headers for libav fft

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

::: media/libav/moz.build
@@ +48,5 @@
> +# the lgpl shared library, since it does not yet use system headers. This is
> +# also used on linux for the time being, to avoid having to patch libav code.
> +#
> +# TODO: Remove header and patch libav once OS X supports system headers
> +if CONFIG['OS_ARCH'] == 'Darwin' or CONFIG['OS_ARCH'] == 'Linux':

As mentioned in previous review, "if CONFIG['OS_ARCH'] != 'WINNT'" would be shorter. Come to think of it, it would also be more correct, since we enable this code on e.g. BSDs.

        Attachment #8630885 -
        Flags: review?(mh+mozilla) → review+

    
    

    
  


  
Fixed platform check for perms header.

        Attachment #8630714 -
        Attachment is obsolete: true

        Attachment #8630885 -
        Attachment is obsolete: true

    
    

    
  
This blows up because PGO builds with no optimization to start, meaning that the 'if' branches that depend on platform preprocessor defines in fft_templace.c try to build even though they don't exist. This would also break anyone trying to build with --disable-optimization.

Solution is to just add dummy functions to fill in the function bodies and at least make sure everything links. Since we never build for the architectures it's having a problem with, we should be ok.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(kyle)

    
    

    
  


  
Turns out that spelling OS names correctly in configure.in is pretty important.

        Attachment #8631074 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8631878 [details] [diff] [review]
Patch 4 (v1) - libav FFT dummy functions for Windows PGO/NoOpt builds

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

::: media/libav/moz.build
@@ +44,4 @@
>      'libavutil/x86/cpuid.asm'
>  ]
>  
> +if CONFIG['OS_ARCH'] == 'WINNT' and (CONFIG["MOZ_PGO"] or not CONFIG["MOZ_OPTIMIZE"]):

It's an MSVC thing more than a Windows thing, so this should be CONFIG['_MSC_VER'] instead of CONFIG['OS_ARCH'] == 'WINNT'. It's also probably safer not to be fancy with the condition and to drop the MOZ_PGO/MOZ_OPTIMIZE check. The dummy functions will be dropped from the final binary anyways.

        Attachment #8631878 -
        Flags: review?(mh+mozilla) → review+
Depends on: 1220037
Depends on: 1223520

    
  
Depends on: 1228230
Depends on: 1481745
See Also:  → 1503361

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: