Closed Bug 1476231 Opened 5 years ago Closed 5 years ago

Add ffmpeg FFT functions to ffvpx and switch web audio from libav to ffvpx

Categories

(Core :: Web Audio, enhancement, P3)

Product:
Core
Component:
Web Audio
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(6 files)

bug 1476231 remove unnecessary x86/cpuid.s, which does not exist in libav-11.3
59 bytes, text/x-review-board-request
jya
: review+
Details
bug 1476231 add ffmpeg floating point real FFT functions to ffvpx when MOZ_LIBAV_FFT is configured
59 bytes, text/x-review-board-request
jya
: review+
Details
bug 1476231 link ffmpeg real DFT functions
59 bytes, text/x-review-board-request
jya
: review+
Details
bug 1476231 add accessor for ffvpx real DFT functions
59 bytes, text/x-review-board-request
jya
: review+
Details
bug 1476231 switch FFTBlock from libav to ffvpx
59 bytes, text/x-review-board-request
jya
: review+
Details
bug 1476231 remove now-unused libav from lgpllibs
59 bytes, text/x-review-board-request
jya
: review+
Details 
This results in a speed improvement of about 6% on the "Convolution reverb"
webaudio-benchmark (measured with Linux x86_64 build on Skylake), and should mean less code to ship.
Comment on attachment 8992577 [details]
bug 1476231 remove unnecessary x86/cpuid.s, which does not exist in libav-11.3

https://reviewboard.mozilla.org/r/257438/#review264362
Attachment #8992577 - Flags: review?(jyavenard) → review+
Comment on attachment 8992578 [details]
bug 1476231 add ffmpeg floating point real FFT functions to ffvpx when MOZ_LIBAV_FFT is configured

https://reviewboard.mozilla.org/r/257440/#review264364
Attachment #8992578 - Flags: review?(jyavenard) → review+
Comment on attachment 8992580 [details]
bug 1476231 add accessor for ffvpx real DFT functions

https://reviewboard.mozilla.org/r/257444/#review264368

::: dom/media/platforms/ffmpeg/FFmpegLibWrapper.h:10
(Diff revision 1)
>  #ifndef __FFmpegLibWrapper_h__
>  #define __FFmpegLibWrapper_h__
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/Types.h"
> +#include "FFVPXRuntimeLinker.h" // for enum RDFTransformType

not sure I like the way it's organised in there... and the apparent cycle in header loading.

Can we use a separate header for those two? that would be included as required.

::: dom/media/platforms/ffmpeg/ffvpx/FFVPXRuntimeLinker.cpp:125
(Diff revision 1)
>    }
>    return FFmpegDecoderModule<FFVPX_VERSION>::Create(&sFFVPXLib);
>  }
>  
> +/* static */ void
> +FFVPXRuntimeLinker::GetRDFTFuncs(FFVPXRuntimeLinker::RDFTFuncs* aOutFuncs)

FFVPXRuntimeLinker::GetRDFTFuncs(RDFTFuncs* aOutFuncs)
Attachment #8992580 - Flags: review?(jyavenard) → review+
Comment on attachment 8992581 [details]
bug 1476231 switch FFTBlock from libav to ffvpx

https://reviewboard.mozilla.org/r/257446/#review264370

::: dom/media/webaudio/FFTBlock.h:305
(Diff revision 1)
>  #endif
>  
>    void Clear()
>    {
>  #if defined(MOZ_LIBAV_FFT)
> -    av_rdft_end(mAvRDFT);
> +    if (av_rdft.end) {

testing if (mAVRFDT || mAVIRDFT) seems more obvious to me, av_rfdt.end can then be in MOZ_ASSERT instead

::: dom/media/webaudio/FFTBlock.h:325
(Diff revision 1)
>    }
>    void AddConstantGroupDelay(double sampleFrameDelay);
>    void InterpolateFrequencyComponents(const FFTBlock& block0,
>                                        const FFTBlock& block1, double interp);
>  #if defined(MOZ_LIBAV_FFT)
> +  static FFVPXRuntimeLinker::RDFTFuncs av_rdft;

Can we use a mozilla-style name here?

makes it confusing reading the code ,as you don't know if we're accessing a ffmpeg function or what...

maybe mAvRfdtLib
Attachment #8992581 - Flags: review?(jyavenard) → review+
Comment on attachment 8992582 [details]
bug 1476231 remove now-unused libav from lgpllibs

https://reviewboard.mozilla.org/r/257448/#review264372

\o/
Attachment #8992582 - Flags: review?(jyavenard) → review+
Comment on attachment 8992579 [details]
bug 1476231 link ffmpeg real DFT functions

https://reviewboard.mozilla.org/r/257442/#review264374

::: dom/media/platforms/ffmpeg/FFmpegLibWrapper.h:19
(Diff revision 1)
>  struct AVPacket;
>  struct AVDictionary;
>  struct AVCodecParserContext;
>  struct PRLibrary;
>  
> +struct RDFTContext;

see comment in patch 4
Attachment #8992579 - Flags: review?(jyavenard) → review+
Comment on attachment 8992580 [details]
bug 1476231 add accessor for ffvpx real DFT functions

https://reviewboard.mozilla.org/r/257444/#review264368

> FFVPXRuntimeLinker::GetRDFTFuncs(RDFTFuncs* aOutFuncs)

This is different now with included as required.
Comment on attachment 8992581 [details]
bug 1476231 switch FFTBlock from libav to ffvpx

https://reviewboard.mozilla.org/r/257446/#review264370

> testing if (mAVRFDT || mAVIRDFT) seems more obvious to me, av_rfdt.end can then be in MOZ_ASSERT instead

Switched to testing mAVRFDT or mAVIRDFT as appropriate.  No need to fill around the code with asserts, because we'll know if the
function pointer is null.

> Can we use a mozilla-style name here?
> 
> makes it confusing reading the code ,as you don't know if we're accessing a ffmpeg function or what...
> 
> maybe mAvRfdtLib

It's now sRDFTFuncs.
Blocks: 1259445
Comment on attachment 8992580 [details]
bug 1476231 add accessor for ffvpx real DFT functions

https://reviewboard.mozilla.org/r/257444/#review267936

::: dom/media/platforms/moz.build:51
(Diff revision 3)
> +    EXPORTS += [
> +        'ffmpeg/FFmpegRDFTTypes.h',
> +    ]

dom/media/platforms/ffmpeg/moz.build is used only when MOZ_FFMPEG is defined,
and MOZ_FFMPEG is not defined on NT, so the export is moved to the same
moz.build file as associated code.
Comment on attachment 8992581 [details]
bug 1476231 switch FFTBlock from libav to ffvpx

https://reviewboard.mozilla.org/r/257446/#review267940

::: testing/web-platform/meta/webaudio/the-audio-api/the-convolvernode-interface/convolver-response-1-chan.html.ini:2
(Diff revision 3)
>  [convolver-response-1-chan.html]
> -  [X 1: Channel 1: Expected 0 for all values but found 1280 unexpected values: \n\tIndex\tActual\n\t[0\]\t-1.4901161193847656e-7\n\t[1\]\t-8.940696716308594e-8\n\t[2\]\t0.3311062455177307\n\t[3\]\t0.6248594522476196\n\t...and 1276 more errors.]
> +  [X 1: Channel 1: Expected 0 for all values but found 1280 unexpected values: \n\tIndex\tActual\n\t[0\]\t-1.1920928955078125e-7\n\t[1\]\t-4.470348358154297e-8\n\t[2\]\t0.3311062455177307\n\t[3\]\t0.6248593926429749\n\t...and 1276 more errors.]

Numerical differences show up here due to bug 1474222 and
https://github.com/web-platform-tests/wpt/issues/10201.

::: testing/web-platform/meta/webaudio/the-audio-api/the-convolvernode-interface/convolver-response-2-chan.html.ini:17
(Diff revision 3)
>      expected: FAIL
>  
>    [# AUDIT TASK RUNNER FINISHED: 2 out of 6 tasks were failed.]
>      expected: FAIL
>  
> +  [X 1: Channel 0 does not equal [0,0,0.9458408951759338,0.8448333740234375,0.8210252523422241,0.8620985746383667,0.8430315852165222,0.855602502822876,0.7933436632156372,0.9865825176239014,0.3972480297088623,-0.7786127924919128,-0.9223549962043762,-0.7896472215652466,-0.8727429509162903,-0.8325281143188477...\] with an element-wise tolerance of {"absoluteThreshold":3.5763e-7,"relativeThreshold":0}.\n\tIndex\tActual\t\t\tExpected\t\tAbsError\t\tRelError\t\tTest threshold\n\t[267\]\t8.6412906646728516e-1\t8.6412948369979858e-1\t4.1723251342773438e-7\t4.8283564129919487e-7\t3.5763000000000001e-7\n\t[779\]\t-8.6412906646728516e-1\t-8.6412948369979858e-1\t4.1723251342773438e-7\t4.8283564129919487e-7\t3.5763000000000001e-7\n\tMax AbsError of 4.1723251342773438e-7 at index of 267.\n\tMax RelError of 4.8283564129919487e-7 at index of 267.\n]

The multiple tolerances in this test were tightly tuned for Firefox with
libav.  On linux32, where libav was not used, the subtests where numerical
errors were the largest differed, and so there are already expected failures.
The switch to ffmpeg similarly changes the particular subtests were numerical
errors are largest, and so some more subtests are classed as failing.
This is bug 1475158, the fix for which is waiting on review.

::: testing/web-platform/meta/webaudio/the-audio-api/the-convolvernode-interface/convolver-response-4-chan.html.ini:48
(Diff revision 3)
>      expected: FAIL
>  
>    [< [5.1-channel input\] 2 out of 2 assertions were failed.]
>      expected: FAIL
>  
> -  [X 1: Channel 0 expected to be equal to the array [0,0,0.9458408951759338,0.8448333740234375,1.7668662071228027,1.7069319486618042,1.6640567779541016,1.7177010774612427,1.6363751888275146,1.8421850204467773,1.1905916929244995,0.20796972513198853,-0.5251069664955139,-1.5682599544525146,-1.7950979471206665,-1.6221753358840942...\] but differs in 965 places:\n\tIndex\tActual\t\t\tExpected\n\t[0\]\t-2.9802322387695313e-8\t0.0000000000000000e+0\n\t[1\]\t-7.4505805969238281e-8\t0.0000000000000000e+0\n\t[2\]\t9.4584065675735474e-1\t9.4584089517593384e-1\n\t[4\]\t1.7668659687042236e+0\t1.7668662071228027e+0\n\t...and 961 more errors.]
> +  [X 1: Channel 0 expected to be equal to the array [0,0,0.9458408951759338,0.8448333740234375,1.7668662071228027,1.7069319486618042,1.6640567779541016,1.7177010774612427,1.6363751888275146,1.8421850204467773,1.1905916929244995,0.20796972513198853,-0.5251069664955139,-1.5682599544525146,-1.7950979471206665,-1.6221753358840942...\] but differs in 969 places:\n\tIndex\tActual\t\t\tExpected\n\t[0\]\t1.1920928955078125e-7\t0.0000000000000000e+0\n\t[1\]\t-7.4505805969238281e-8\t0.0000000000000000e+0\n\t[2\]\t9.4584071636199951e-1\t9.4584089517593384e-1\n\t[3\]\t8.4483325481414795e-1\t8.4483337402343750e-1\n\t...and 965 more errors.]

This is also bug 1475158, the fix for which is waiting on review.
Pushed by ktomlinson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/056ccdf7f059
remove unnecessary x86/cpuid.s, which does not exist in libav-11.3 r=jya
https://hg.mozilla.org/integration/autoland/rev/08a8351aca6f
add ffmpeg floating point real FFT functions to ffvpx when MOZ_LIBAV_FFT is configured r=jya
https://hg.mozilla.org/integration/autoland/rev/fefcc2674976
link ffmpeg real DFT functions r=jya
https://hg.mozilla.org/integration/autoland/rev/c988d5a322d3
add accessor for ffvpx real DFT functions r=jya
https://hg.mozilla.org/integration/autoland/rev/0fbbfd857bb8
switch FFTBlock from libav to ffvpx r=jya
https://hg.mozilla.org/integration/autoland/rev/beddb157da49
remove now-unused libav from lgpllibs r=jya
Regressions: 1656063
You need to log in before you can comment on or make changes to this bug.