Support ffmpeg on Windows

RESOLVED FIXED in Firefox 46

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: kentuckyfriedtakahe, Assigned: jya)

Tracking

(Depends on: 3 bugs)

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(8 attachments, 6 obsolete attachments)

11.25 KB, patch
Details | Diff | Splinter Review
3.34 MB, patch
kentuckyfriedtakahe
: review+
Details | Diff | Splinter Review
890 bytes, patch
gerv
: review+
Details | Diff | Splinter Review
4.12 KB, patch
kentuckyfriedtakahe
: review+
glandium
: feedback+
Details | Diff | Splinter Review
2.49 KB, patch
glandium
: review-
Details | Diff | Splinter Review
12.95 KB, patch
jya
: review+
Details | Diff | Splinter Review
587.14 KB, patch
glandium
: review+
Details | Diff | Splinter Review
1.79 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
Add support for ffmpeg on Windows so we can use ffvp9.
Created attachment 8673421 [details] [diff] [review]
WIP patch; ffmpeg support on Windows
Assignee: nobody → ajones
Status: NEW → ASSIGNED
Priority: -- → P2
Blocks: 1210219
Depends on: 1205746
Blocks: 1223270
(Assignee)

Updated

3 years ago
Depends on: 1223333
(Assignee)

Comment 6

3 years ago
Created attachment 8687742 [details] [diff] [review]
P1. Add ffvpx source code.

Code was extracted from FFmpeg n2.8.2

Files needed found using the command ./configure --disable-everything --disable-protocols --disable-demuxers --disable-muxers --disable-filters --disable-programs --disable-doc --disable-parsers --enable-parser=vp8 --enable-parser=vp9 --enable-decoder=vp8 --enable-decoder=vp9 --disable-static --enable-shared --disable-debug --disable-sdl --disable-libxcb --disable-securetransport --disable-iconv --disable-swresample --disable-swscale --disable-avdevice --disable-avfilter --disable-avformat --disable-d3d11va --disable-dxva2 --disable-vaapi --disable-vda --disable-vdpau --disable-videotoolbox --enable-asm --enable-yasm
Attachment #8687742 - Flags: review?(ajones)
(Assignee)

Comment 7

3 years ago
Created attachment 8687743 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx.
Attachment #8687743 - Flags: review?(mh+mozilla)
(Assignee)

Comment 8

3 years ago
Created attachment 8687744 [details] [diff] [review]
P3. Add FFmpeg license's link to about:license.
Attachment #8687744 - Flags: review?(gerv)
(Assignee)

Comment 9

3 years ago
Created attachment 8687745 [details] [diff] [review]
P4. Add Windows support for the FFmpeg PDM.
Attachment #8687745 - Flags: review?(ajones)
(Assignee)

Comment 10

3 years ago
Created attachment 8687746 [details] [diff] [review]
P5. Enable ffvpx compilation and use.
Attachment #8687746 - Flags: review?(mh+mozilla)
(Assignee)

Comment 11

3 years ago
Created attachment 8687747 [details] [diff] [review]
P6. Fix ffvpx compilation on android x86 and FreeBSD.
Attachment #8687747 - Flags: review?(mh+mozilla)
Comment on attachment 8687745 [details] [diff] [review]
P4. Add Windows support for the FFmpeg PDM.

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

Approved with the 0 and -1 sentinels cleaned up.

::: dom/media/platforms/ffmpeg/FFmpegFunctionList.h
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// A value of 0 indicates that the entry is located in libavcodec.dll
> +// A negative value indicates that the entry is to always be loaded from
> +// libavutil.dll, regardless of the version.

You should use something like:

#define AV_FUNC_AVCODEC   0
#define AV_FUNC_LIBAVUTIL (-1)
Attachment #8687745 - Flags: review?(ajones) → review+
Comment on attachment 8687745 [details] [diff] [review]
P4. Add Windows support for the FFmpeg PDM.

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

::: dom/media/platforms/ffmpeg/FFmpegLibs.h
@@ +37,1 @@
>  #define AV_FUNC(func, ver) extern typeof(func)* func;

You should be able to use `decltype` without checking __GNUC__ if you #include "mozilla/Types.h" because the header #defines decltype to typeof for the appropriate gcc versions.

::: dom/media/platforms/ffmpeg/FFmpegRuntimeLinker.cpp
@@ +12,5 @@
>  
> +#if defined(XP_WIN)
> +#include "libavcodec/avcodec.h"
> +#include "libavutil/avutil.h"
> +#include <libavutil/imgutils.h>

Why '<' instead of '"' here?

@@ +131,5 @@
>  
>  #define LIBAVCODEC_ALLVERSION
>  #define AV_FUNC(func, ver)                                                     \
> +  if (ver <= 0 || ver == major) {                                              \
> +    if (!(func = (typeof(func))PR_FindSymbol(ver != 0 ? sLinkedUtilLib : sLinkedLib, #func))) { \

You should be able to use decltype instead of typeof macro if you #include "mozilla/Types.h". You can then remove the __GNUC__ macro gunk on lines 55–63 above.
Depends on: 1224408
Comment on attachment 8687744 [details] [diff] [review]
P3. Add FFmpeg license's link to about:license.

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

r=gerv. Good catch.

Gerv
Attachment #8687744 - Flags: review?(gerv) → review+
(Assignee)

Comment 16

3 years ago
(In reply to Chris Peterson [:cpeterson] from comment #14)

> You should be able to use decltype instead of typeof macro if you #include
> "mozilla/Types.h". You can then remove the __GNUC__ macro gunk on lines
> 55–63 above.

unfortunately not.
Because on windows we must set the type of the function pointer to what it really is, while on other platforms they are void*

But point taken for the other areas.

Updated

3 years ago
Depends on: 1225325
Comment on attachment 8687743 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx.

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

::: media/ffvpx/config.h
@@ +12,5 @@
> +#if defined(ARCH_X86_64)
> +#include "config_darwin64.h"
> +#else
> +#include "config_darwin32.h"
> +#endif

I suspect the 32 and 64 variants are largely identical. It would be better if they were just one file with a few ifdefs. That's actually probably true across platforms, where the whole thing is probably identical with very few differences across platform/bitsize.

::: media/ffvpx/configure.run
@@ +1,1 @@
> +./configure --disable-everything --disable-protocols --disable-demuxers --disable-muxers --disable-filters --disable-programs --disable-doc --disable-parsers --enable-parser=vp8 --enable-parser=vp9 --enable-decoder=vp8 --enable-decoder=vp9 --disable-static --enable-shared --disable-debug --disable-sdl --disable-libxcb --disable-securetransport --disable-iconv --disable-swresample --disable-swscale --disable-avdevice --disable-avfilter --disable-avformat --disable-d3d11va --disable-dxva2 --disable-vaapi --disable-vda --disable-vdpau --disable-videotoolbox --enable-asm --enable-yasm --disable-avx2

I'd rather see a README explaining how this is updated than a random "configure.run" file that doesn't say much.

::: media/ffvpx/ffvpxcommon.mozbuild
@@ +20,5 @@
> +        # 32-bit windows need to prefix symbols with an underscore.
> +        if CONFIG['CPU_ARCH'] == 'x86':
> +            ASFLAGS += ['-DPREFIX']
> +        # To avoid attempting to link on removed code, use optimised compilation.
> +        CFLAGS += ['-O1']

Please do like libav and add dummy functions instead of relying on a forced optimization level. Technically, you're also probably disabling PGO/LTO with this here.

@@ +23,5 @@
> +        # To avoid attempting to link on removed code, use optimised compilation.
> +        CFLAGS += ['-O1']
> +    elif CONFIG['OS_ARCH'] == 'Darwin':
> +        # 32/64-bit macosx assemblers need to prefix symbols with an underscore.
> +        ASFLAGS += ['-Pconfig_darwin64.asm', '-DPREFIX']

ASFLAGS += [
    '-Pconfig_darwin64.asm',
    '-DPREFIX',
]

@@ +29,5 @@
> +        # Default to unix, similar to how ASFLAGS setup works in configure.in
> +        ASFLAGS += ['-Pconfig_unix64.asm']
> +
> +if CONFIG['CPU_ARCH'] == 'x86_64':
> +    CFLAGS += [ '-DARCH_X86_64']

DEFINES['ARCH_X86_64'] = True

That said, if you only need ARCH_X86_64 for the test in config.h, then you can change that test to use HAVE_64BIT_BUILD instead.

@@ +74,5 @@
> +        # from FFmpeg configure
> +        '-wd4244', '-wd4127', '-wd4018', '-wd4389', '-wd4146', '-wd4701',
> +        '-wd4057', '-wd4204', '-wd4706', '-wd4305', '-wd4152', '-wd4324',
> +        '-we4013', '-wd4100', '-wd4214', '-wd4307', '-wd4273', '-wd4554',
> +        #'-WX', # Force warning as error, use this to simulate try build

Please remove this line.

@@ +77,5 @@
> +        '-we4013', '-wd4100', '-wd4214', '-wd4307', '-wd4273', '-wd4554',
> +        #'-WX', # Force warning as error, use this to simulate try build
> +    ]
> +
> +CFLAGS += ['-DHAVE_AV_CONFIG_H']

DEFINES[...] = True

::: media/ffvpx/libavcodec/moz.build
@@ +53,5 @@
> +ALLOW_COMPILER_WARNINGS = True
> +
> +DEFFILE = SRCDIR + '/avcodec-56.def'
> +
> +NO_VISIBILITY_FLAGS = True

Would be better to limit this to the platforms where it's strictly necessary (which I guess is mac)

@@ +55,5 @@
> +DEFFILE = SRCDIR + '/avcodec-56.def'
> +
> +NO_VISIBILITY_FLAGS = True
> +
> +USE_LIBS += [ 'mozavutil' ]

USE_LIBS += [
   'mozavutil',
]

::: media/ffvpx/libavcodec/x86/moz.build
@@ +22,5 @@
> +    'vp9mc.asm'
> +
> +]
> +
> +FINAL_LIBRARY='mozavcodec'

spaces around the =

::: media/ffvpx/libavutil/internal.h
@@ -236,5 @@
>                             const char *msg, ...) av_printf_format(2, 3);
>  
>  #if HAVE_LIBC_MSVCRT
> -#include <crtversion.h>
> -#if defined(_VC_CRT_MAJOR_VERSION) && _VC_CRT_MAJOR_VERSION < 14

Why do you need to change from _VC_CRT_MAJOR_VERSION to _MSC_VER?

::: media/ffvpx/libavutil/x86/moz.build
@@ +13,5 @@
> +    'lls.asm',
> +    'lls_init.c'
> +]
> +
> +FINAL_LIBRARY='mozavutil'

spaces around =

::: media/ffvpx/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/.
> +
> +DIRS += ['libavutil']
> +DIRS += ['libavcodec']

DIRS += [
    'libavcodec',
    'libavutil',
]

Order doesn't matter.
Attachment #8687743 - Flags: review?(mh+mozilla)
Comment on attachment 8687746 [details] [diff] [review]
P5. Enable ffvpx compilation and use.

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

::: configure.in
@@ +3706,5 @@
>    MOZ_FMP4=1
>  else
>    MOZ_FMP4=
>  fi
> +MOZ_FFMPEG=1

The implication of doing this needs to be reviewed by some media peer.

@@ +6279,5 @@
> +case "$CPU_ARCH" in
> +  x86)
> +      MOZ_FFVPX=1
> +  ;;
> +  x86_64)

x86|x86_64)
Attachment #8687746 - Flags: review?(mh+mozilla) → feedback+
Comment on attachment 8687747 [details] [diff] [review]
P6. Fix ffvpx compilation on android x86 and FreeBSD.

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

Err, you're disabling stuff that exist on linux to please android/bsd? that doesn't seem right.
Attachment #8687747 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 20

3 years ago
(In reply to Mike Hommey [:glandium] from comment #17)
> Comment on attachment 8687743 [details] [diff] [review]

> I suspect the 32 and 64 variants are largely identical. It would be better
> if they were just one file with a few ifdefs. That's actually probably true
> across platforms, where the whole thing is probably identical with very few
> differences across platform/bitsize.

I much prefer to have a single file per platform, because as those are generated by the ffmpeg's configure script, it's just a matter of copying the result, rather than manually editing the result.
(Assignee)

Comment 21

3 years ago
(In reply to Mike Hommey [:glandium] from comment #19)
> Comment on attachment 8687747 [details] [diff] [review]
> P6. Fix ffvpx compilation on android x86 and FreeBSD.
> 
> Review of attachment 8687747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Err, you're disabling stuff that exist on linux to please android/bsd? that
> doesn't seem right.

the code is unused on x86 platform as it's plain-C.

For the x86_64 platform, the code is also unused.

It just allows to compile a #include that wouldn't otherwise exits.
(Assignee)

Comment 22

3 years ago
(In reply to Mike Hommey [:glandium] from comment #17)
> Comment on attachment 8687743 [details] [diff] [review]
> P2. Add mozilla build files to build ffvpx.
> 
> Review of attachment 8687743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/ffvpx/config.h
> @@ +12,5 @@
> > +#if defined(ARCH_X86_64)
> > +#include "config_darwin64.h"
> > +#else
> > +#include "config_darwin32.h"
> > +#endif
> 
> I suspect the 32 and 64 variants are largely identical. It would be better
> if they were just one file with a few ifdefs. That's actually probably true
> across platforms, where the whole thing is probably identical with very few
> differences across platform/bitsize.
> 
> ::: media/ffvpx/configure.run
> @@ +1,1 @@
> > +./configure --disable-everything --disable-protocols --disable-demuxers --disable-muxers --disable-filters --disable-programs --disable-doc --disable-parsers --enable-parser=vp8 --enable-parser=vp9 --enable-decoder=vp8 --enable-decoder=vp9 --disable-static --enable-shared --disable-debug --disable-sdl --disable-libxcb --disable-securetransport --disable-iconv --disable-swresample --disable-swscale --disable-avdevice --disable-avfilter --disable-avformat --disable-d3d11va --disable-dxva2 --disable-vaapi --disable-vda --disable-vdpau --disable-videotoolbox --enable-asm --enable-yasm --disable-avx2
> 
> I'd rather see a README explaining how this is updated than a random
> "configure.run" file that doesn't say much.
> 
> ::: media/ffvpx/ffvpxcommon.mozbuild
> @@ +20,5 @@
> > +        # 32-bit windows need to prefix symbols with an underscore.
> > +        if CONFIG['CPU_ARCH'] == 'x86':
> > +            ASFLAGS += ['-DPREFIX']
> > +        # To avoid attempting to link on removed code, use optimised compilation.
> > +        CFLAGS += ['-O1']
> 
> Please do like libav and add dummy functions instead of relying on a forced
> optimization level. Technically, you're also probably disabling PGO/LTO with
> this here.

libav can afford to do this as its tiny. 
There are over 1200 of those unused functions left in libavcodec. By doing so you also greatly increase the future cost to maintain will be high. They are all private, internal API whose definitions change often (and I'm talking by experience having maintained a ffmpeg fork)

So this is precisely why I don't want to take that approach. 
I have discussed this with Anthony already. 

> @@ +29,5 @@
> > +        # Default to unix, similar to how ASFLAGS setup works in configure
> DEFINES['ARCH_X86_64'] = True
> 
> That said, if you only need ARCH_X86_64 for the test in config.h, then you
> can change that test to use HAVE_64BIT_BUILD instead.
> 

Cool. I thought there was something available for this. 

> ::: media/ffvpx/libavutil/internal.h
> @@ -236,5 @@
> >                             const char *msg, ...) av_printf_format(2, 3);
> >  
> >  #if HAVE_LIBC_MSVCRT
> > -#include <crtversion.h>
> > -#if defined(_VC_CRT_MAJOR_VERSION) && _VC_CRT_MAJOR_VERSION < 14
> 
> Why do you need to change from _VC_CRT_MAJOR_VERSION to _MSC_VER?
> 

Otherwise I get link failures on win64 due to those two entries not being found.
http://archive.mozilla.org/pub/firefox/try-builds/jyavenard@mozilla.com-a7d5b5bbd8e1f3517344798b624fc25b4ccc79a1/try-win64/try-win64-bm78-try1-build313.txt.gz

As this is something they have fixed in libav, I took their fix.
(In reply to Jean-Yves Avenard [:jya] from comment #22)
> (In reply to Mike Hommey [:glandium] from comment #17)
> > Comment on attachment 8687743 [details] [diff] [review]
> > P2. Add mozilla build files to build ffvpx.
> > 
> > Review of attachment 8687743 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/ffvpx/config.h
> > @@ +12,5 @@
> > > +#if defined(ARCH_X86_64)
> > > +#include "config_darwin64.h"
> > > +#else
> > > +#include "config_darwin32.h"
> > > +#endif
> > 
> > I suspect the 32 and 64 variants are largely identical. It would be better
> > if they were just one file with a few ifdefs. That's actually probably true
> > across platforms, where the whole thing is probably identical with very few
> > differences across platform/bitsize.
> > 
> > ::: media/ffvpx/configure.run
> > @@ +1,1 @@
> > > +./configure --disable-everything --disable-protocols --disable-demuxers --disable-muxers --disable-filters --disable-programs --disable-doc --disable-parsers --enable-parser=vp8 --enable-parser=vp9 --enable-decoder=vp8 --enable-decoder=vp9 --disable-static --enable-shared --disable-debug --disable-sdl --disable-libxcb --disable-securetransport --disable-iconv --disable-swresample --disable-swscale --disable-avdevice --disable-avfilter --disable-avformat --disable-d3d11va --disable-dxva2 --disable-vaapi --disable-vda --disable-vdpau --disable-videotoolbox --enable-asm --enable-yasm --disable-avx2
> > 
> > I'd rather see a README explaining how this is updated than a random
> > "configure.run" file that doesn't say much.
> > 
> > ::: media/ffvpx/ffvpxcommon.mozbuild
> > @@ +20,5 @@
> > > +        # 32-bit windows need to prefix symbols with an underscore.
> > > +        if CONFIG['CPU_ARCH'] == 'x86':
> > > +            ASFLAGS += ['-DPREFIX']
> > > +        # To avoid attempting to link on removed code, use optimised compilation.
> > > +        CFLAGS += ['-O1']
> > 
> > Please do like libav and add dummy functions instead of relying on a forced
> > optimization level. Technically, you're also probably disabling PGO/LTO with
> > this here.
> 
> libav can afford to do this as its tiny. 
> There are over 1200 of those unused functions left in libavcodec. By doing
> so you also greatly increase the future cost to maintain will be high. They
> are all private, internal API whose definitions change often (and I'm
> talking by experience having maintained a ffmpeg fork)
> 
> So this is precisely why I don't want to take that approach. 
> I have discussed this with Anthony already. 

Then at the very least make it dependent on the fact that optimization is not enabled in the first place, because you're essentially downgrading optimization when it's enabled.

> > ::: media/ffvpx/libavutil/internal.h
> > @@ -236,5 @@
> > >                             const char *msg, ...) av_printf_format(2, 3);
> > >  
> > >  #if HAVE_LIBC_MSVCRT
> > > -#include <crtversion.h>
> > > -#if defined(_VC_CRT_MAJOR_VERSION) && _VC_CRT_MAJOR_VERSION < 14
> > 
> > Why do you need to change from _VC_CRT_MAJOR_VERSION to _MSC_VER?
> > 
> 
> Otherwise I get link failures on win64 due to those two entries not being
> found.
> http://archive.mozilla.org/pub/firefox/try-builds/jyavenard@mozilla.com-
> a7d5b5bbd8e1f3517344798b624fc25b4ccc79a1/try-win64/try-win64-bm78-try1-
> build313.txt.gz
> 
> As this is something they have fixed in libav, I took their fix.

I'm asking why you're changing the variable used to check the version, not why you need to change the version check.
(Assignee)

Comment 24

3 years ago
(In reply to Mike Hommey [:glandium] from comment #23)

> Then at the very least make it dependent on the fact that optimization is
> not enabled in the first place, because you're essentially downgrading
> optimization when it's enabled.

okay.. do you have a suggestion on how to do this?

When I had /Ox (which is all optimisations turned on); all the logs on windows opt showed that I was overriding /O1 with /Ox. (warning was: 23:50:18     INFO -  cl : Command line warning D9025 : overriding '/O1' with '/Ox')

/O1 appears to be the optimisation value used for all windows opt builds (that part of the moz.build is restricted to windows)

> I'm asking why you're changing the variable used to check the version, not
> why you need to change the version check.

I couldn't reproduce it with my local win64 build ; so I have no idea what _VC_CRT_MAJOR_VERSION actually is on the build bot.

Seeing that the same code in libav modified the check to use _MSC_VER and provided that it worked (it took me a fair amount of blind try runs to finally got something to work).
(Assignee)

Comment 25

3 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #21)
> (In reply to Mike Hommey [:glandium] from comment #19)
> > Comment on attachment 8687747 [details] [diff] [review]
> > P6. Fix ffvpx compilation on android x86 and FreeBSD.
> > 
> > Review of attachment 8687747 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Err, you're disabling stuff that exist on linux to please android/bsd? that
> > doesn't seem right.
> For the x86_64 platform, the code is also unused.
> 
> It just allows to compile a #include that wouldn't otherwise exits.

further to this (I was leaving for lunch and didn't take to post the details).

The change is mostly on config_unix32.h ; which is used whenever we're not on windows or mac.
Furthermore, when using config_unix32.h we have disabled all assembly code and are in pure C.

HAVE_SYSCTL is used to determine the number of processors on linux. We do not use this API as we explicitly set the number of threads to be used for decoding in the FFmpeg PDM (value retrieved using PR_GetNumberOfProcessors())

POSIX_MEMALIGN is the other I disabled. This is only use when allocating memory should assembly is enabled (as, as you know SSE required aligned memory).
We do not reach that code when using the plain C code, and even so it reaches the option of HAVE_MEMALIGN which is just as efficient.

I took the approach that config_unix* was for "unix" system, not just linux in particular. This allows to use the C code on whichever platform we want ; as it turned out ffvp9 C code is still significantly faster than libvpx SSE optimised one!
(Assignee)

Comment 26

3 years ago
Comment on attachment 8687746 [details] [diff] [review]
P5. Enable ffvpx compilation and use.

I'm guessing you'll be okay with that change seeing that this is something I took mostly from your own patch :)

This change enable ffmpeg by default on all platforms.
Attachment #8687746 - Flags: review?(ajones)
(Assignee)

Comment 27

3 years ago
Created attachment 8688254 [details] [diff] [review]
P4. Add Windows support for the FFmpeg PDM.

making change, carrying r+
Attachment #8688254 - Flags: review+
(Assignee)

Comment 28

3 years ago
(In reply to Mike Hommey [:glandium] from comment #17)

> > +NO_VISIBILITY_FLAGS = True
> 
> Would be better to limit this to the platforms where it's strictly necessary
> (which I guess is mac)

Unfortunately, it appears to be related to the use of the clang compiler rather than the platform as I get linkage error on the asan linux build as well as the some android/b2g build.

As I don't see clang related configuration being export, I'm afraid it has to be enabled on all platforms.
(Assignee)

Comment 29

3 years ago
Created attachment 8688302 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx.

Making all the required change.
Limiting change of optimisation flag on non-optimised builds.
Simplify the amount of changes to the original source.
Indicating in README_MOZILLA how the config files were generated and notes about AVX2 being disabled.
Attachment #8688302 - Flags: review?(mh+mozilla)
Depends on: 1230265
(Assignee)

Updated

3 years ago
Depends on: 1232268
(Assignee)

Updated

3 years ago
Assignee: ajones → jyavenard
(Assignee)

Comment 31

3 years ago
Mike, could you have a look at the review? I answered all your concerns either in patch or in the comments above.

We would now like to go ahead and merge that in.

thank you
Flags: needinfo?(mh+mozilla)
Attachment #8687742 - Flags: review?(ajones) → review+
Attachment #8687746 - Flags: review?(ajones) → review+
Comment on attachment 8688302 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx.

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

::: media/ffvpx/README_MOZILLA
@@ +5,5 @@
>  Currently, we only use the vp8 and vp9 portion of the library, and only on x86
>  based platforms. If this changes, configuration files will most likely
>  need to be updated.
> +
> +The configuration file were generated using FFmpeg's configure file using the

The configuration file*s* were generated using FFmpeg's configure *script*...

@@ +7,5 @@
>  need to be updated.
> +
> +The configuration file were generated using FFmpeg's configure file using the
> +command:
> +# ./configure --disable-everything --disable-protocols --disable-demuxers --disable-muxers --disable-filters --disable-programs --disable-doc --disable-parsers --enable-parser=vp8 --enable-parser=vp9 --enable-decoder=vp8 --enable-decoder=vp9 --disable-static --enable-shared --disable-debug --disable-sdl --disable-libxcb --disable-securetransport --disable-iconv --disable-swresample --disable-swscale --disable-avdevice --disable-avfilter --disable-avformat --disable-d3d11va --disable-dxva2 --disable-vaapi --disable-vda --disable-vdpau --disable-videotoolbox --enable-asm --enable-yasm --disable-avx2

it's funny how --disable-everything still needs multiple other --disable.

::: media/ffvpx/ffvpxcommon.mozbuild
@@ +17,5 @@
> +        # 32-bit windows need to prefix symbols with an underscore.
> +        if CONFIG['CPU_ARCH'] == 'x86':
> +            ASFLAGS += ['-DPREFIX']
> +        # To avoid attempting to link on removed code, use optimised compilation.
> +        if not CONFIG['MOZ_OPTMIZE']:

MOZ_OPTIMIZE

You should probably do a try push with an explicit --disable-optimize to check this actually works ;)

::: media/ffvpx/libavcodec/moz.build
@@ +49,5 @@
> +    'xiph.c'
> +]
> +
> +# We allow warnings for third-party code that can be updated from upstream.
> +ALLOW_COMPILER_WARNINGS = True

Why not put that in the common.mozbuild file, next to all the -W flags you add?

@@ +53,5 @@
> +ALLOW_COMPILER_WARNINGS = True
> +
> +DEFFILE = SRCDIR + '/avcodec-56.def'
> +
> +NO_VISIBILITY_FLAGS = True

This still will export way too many symbols from libmozavcodec on both linux and mac. Same is true for libmozavutil. BTW, other than the fact that it's generally not desirable, on linux it may or may not cause problems now or in the future, because of the overlapping symbols in both libraries, which for now, are ff_reverse and ff_log2_tab. This /could/ be worked around by using a LD_VERSION_SCRIPT (and using convert_def_file to create one from the windows .def files), but that would only address it on linux, not mac.
Attachment #8688302 - Flags: review?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 33

3 years ago
(In reply to Mike Hommey [:glandium] from comment #32)

> @@ +53,5 @@
> > +ALLOW_COMPILER_WARNINGS = True
> > +
> > +DEFFILE = SRCDIR + '/avcodec-56.def'
> > +
> > +NO_VISIBILITY_FLAGS = True
> 
> This still will export way too many symbols from libmozavcodec on both linux
> and mac. Same is true for libmozavutil. BTW, other than the fact that it's
> generally not desirable, on linux it may or may not cause problems now or in
> the future, because of the overlapping symbols in both libraries, which for
> now, are ff_reverse and ff_log2_tab. This /could/ be worked around by using
> a LD_VERSION_SCRIPT (and using convert_def_file to create one from the
> windows .def files), but that would only address it on linux, not mac.

I'm not sure I follow what you want instead that doesn't make us fall back in the trap mentioned in comment 22.

What do you suggest we do then, when both libavcodec and libavutil call each others private's symbols.

Is this something you could take care of? I'm feeling that I will never achieve what you want in regards to packaging those two libraries. Those issues are things we find upstream.. Narrowing the symbols exported is only going to exponentially increase maintenance time.
Flags: needinfo?(mh+mozilla)
You're already maintaining a list of exported symbols for Windows. Why would that add maintenance time when that's something you'll already have to update?
Flags: needinfo?(mh+mozilla)
I'll note that upstream doesn't export those symbols we do export:
$ readelf -sW /usr/lib/x86_64-linux-gnu/libavutil.so.53 | grep ff_log2_tab
$ readelf -sW /usr/lib/x86_64-linux-gnu/libavcodec.so.55 | grep ff_log2_tab
(Assignee)

Comment 36

3 years ago
(In reply to Mike Hommey [:glandium] from comment #34)
> You're already maintaining a list of exported symbols for Windows. Why would
> that add maintenance time when that's something you'll already have to
> update?

So NO_VISIBILITY_FLAGS = True actually make all symbols visible (sorry, it's been so long now that I have already forgotten what those flags are doing). Name is counter intuitive imho.

What's the mac equivalent of DEFFILE then ? or is there even one ?
how else could you restrict what symbols are exported?
(In reply to Jean-Yves Avenard [:jya] from comment #36)
> So NO_VISIBILITY_FLAGS = True actually make all symbols visible (sorry, it's
> been so long now that I have already forgotten what those flags are doing).
> Name is counter intuitive imho.

It's also not really supposed to be used...

> What's the mac equivalent of DEFFILE then ? or is there even one ?

There isn't.

> how else could you restrict what symbols are exported?

By not using NO_VISIBILITY_FLAGS... which is sometimes not possible. But maybe it is possible in our case.
(Assignee)

Comment 38

3 years ago
(In reply to Mike Hommey [:glandium] from comment #32)
> a LD_VERSION_SCRIPT (and using convert_def_file to create one from the
> windows .def files), but that would only address it on linux, not mac.

convert_def_file appears to only be used from within a makefile, where you have far greater flexibility than within a moz.build.
How do you suggest I could convert_def_file within a moz.build.

As an alternative, could I use instead a manually crafted .def file that I will pass on linux with -Bsymbolic (ffmpeg already has one that contains
LIBAVCODEC_56 {
        global: av*;
                #deprecated, remove after next bump
                audio_resample;
                audio_resample_close;
        local:  *;
};
)

and one for mac have one used in combination with -exported_symbols_list?

I will create a follow up bug to automate the creation of the export list from the single .def file.
(Assignee)

Comment 39

3 years ago
(In reply to Mike Hommey [:glandium] from comment #34)
> You're already maintaining a list of exported symbols for Windows. Why would
> that add maintenance time when that's something you'll already have to
> update?

I was discussing the issue with Anthony. Those two libraries are linked using dlopen and only a narrow set of symbols are actually resolved using dlsym 
https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/ffmpeg/FFmpegFunctionList.h

As such, it really doesn't matter what symbols are exported.

Do we really need to address this issue ? can this be postponed to later?
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 40

3 years ago
Created attachment 8701313 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx.

Carrying on changes but the visibility changes.
Attachment #8701313 - Flags: review?(mh+mozilla)
(Assignee)

Updated

3 years ago
Attachment #8687743 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8688302 - Attachment is obsolete: true
I'm not following the logic that gets you from "only a narrow set of symbols are used" to "it doesn't matter what symbols are exported".
Flags: needinfo?(mh+mozilla) → needinfo?(jyavenard)
(Assignee)

Comment 42

3 years ago
We aren't linking the libraries against xul, nothing link to those libs but the ffmpeg PDM which only dlsym a set subset of symbols.

So there can't be an issue having too many symbols exported, it has no effect on the rest of our code.
Flags: needinfo?(jyavenard)
That doesn't make it any better. And now that there are patches to the newly filed bug 1235132, limiting what you export from those libraries should be effortless. Just convert that .def to the new format (essentially, remove LIBRARY and EXPORTS, as well as leading whitespaces and you're done), rename to .symbols, and change moz.builds to use SYMBOLS_FILE instead of DEFFILE, and that should do it, provided the symbol list is good for all platforms.
Attachment #8701313 - Flags: review?(mh+mozilla)
(Assignee)

Comment 44

3 years ago
thank you for creating bug 1235132... now have to wait until that one goes in.

(I do think dlsym each symbols really make it irrelevant on what symbols are actually exported)
(Assignee)

Updated

3 years ago
Depends on: 1235132
(Assignee)

Comment 45

3 years ago
Created attachment 8702026 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx.

Use SYMBOL_FILES.

Note that this doesn't work on mac, I get a warning for each entries found in the .symbols file
like:
 1:07.14 ld: warning: cannot export hidden symbol _av_buffer_allocz from buffer.o
 1:07.14 ld: warning: cannot export hidden symbol _av_buffer_ref from buffer.o
 1:07.14 ld: warning: cannot export hidden symbol _av_buffer_unref from buffer.o

which then failed to be resolved in libavcodec.
 1:07.87 Undefined symbols for architecture x86_64:
 1:07.88   "_av_buffer_ref", referenced from:
 1:07.88       _copy_packet_data in avpacket.o
 1:07.88       _av_packet_ref in avpacket.o
 1:07.88       _get_buffer_internal in utils.o
 1:07.88       _ff_thread_ref_frame in utils.o
 1:07.88       _vp8_ref_frame in vp8.o
 1:07.88       _vp9_ref_frame in vp9.o
 1:07.88   "_av_buffer_unref", referenced from:
 1:07.88       _av_free_packet in avpacket.o
 1:07.88       _av_packet_unref in avpacket.o
 1:07.88       _thread_get_buffer_internal in pthread_frame.o
 1:07.88       _ff_thread_release_buffer in pthread_frame.o
 1:07.88       _get_buffer_internal in utils.o
 1:07.88       _compat_release_buffer in utils.o
 1:07.88       _vp8_release_frame in vp8.o
 1:07.88       ...


I see that you're going away until January 11th so I'm guessing nothing will happen until then.
This bug should have got in weeks ago. And is now being delayed for what I believe is a non-problem :(
Attachment #8702026 - Flags: review?(mh+mozilla)
(Assignee)

Comment 46

3 years ago
Anthony, if someone can take over this bug.
Assignee: jyavenard → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ajones)
(Assignee)

Updated

3 years ago
Attachment #8701313 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8687745 - Attachment is obsolete: true
(Assignee)

Comment 47

3 years ago
Comment on attachment 8702026 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx.

didn't realise .symbols could have #ifdef
Attachment #8702026 - Attachment is obsolete: true
Attachment #8702026 - Flags: review?(mh+mozilla)
(Assignee)

Comment 48

3 years ago
Created attachment 8702027 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx.

still doesn't compile...
You need NO_VISIBILITY_FLAGS = True
(Assignee)

Comment 50

3 years ago
I thought this exported all symbols?
On the contrary, it hides all symbols not listed, but if the symbols are hidden in the first place, it can't export them. Except on Windows.
(Assignee)

Comment 52

3 years ago
(In reply to Mike Hommey [:glandium] (VAC: 31 Dec - 11 Jan) from comment #51)
> On the contrary, it hides all symbols not listed, but if the symbols are
> hidden in the first place, it can't export them. Except on Windows.

So all symbols are visible and exported with it, but SYMBOL_FILES restrict those then?

The names are counter intuitive really.
Except on Windows, where all the symbols are hidden except if marked otherwise in the source, and SYMBOL_FILES makes them exported. There is no way to explain this that is cross-platform.
(Assignee)

Comment 54

3 years ago
Created attachment 8702085 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ca22791983
Flags: needinfo?(ajones)
Attachment #8702085 - Flags: review?(mh+mozilla)
(Assignee)

Updated

3 years ago
Attachment #8702027 - Attachment is obsolete: true
Comment on attachment 8702085 [details] [diff] [review]
P2. Add mozilla build files to build ffvpx

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

::: media/ffvpx/README_MOZILLA
@@ +5,5 @@
>  Currently, we only use the vp8 and vp9 portion of the library, and only on x86
>  based platforms. If this changes, configuration files will most likely
>  need to be updated.
> +
> +The configuration file*s* were generated using FFmpeg's configure file using the

The * in comment 32 was there as an emphasis on what's different, not meant to be put verbatim :)
s/configure file/configure script/ too.
Attachment #8702085 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 56

3 years ago
\o/

I wasn't going to suffer another r- :) so I took everything verbatim !
(Assignee)

Updated

3 years ago
Depends on: 1235959
(Assignee)

Updated

3 years ago
Depends on: 1236120
(Assignee)

Comment 60

3 years ago
Chris,
In https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f466bb4d8f0


It shows that Windows XP canPlayType errors due to it returning ""

But that is the proper behaviour; there's no h264 or AAC decoder on Windows. 
Why would it ever return maybe?

Also I wonder if that's what causing bug 1235222

It seems that having a PDM now causes canPlayType to return the proper value. Just surprising that the mochitest ever expected maybe there.
Flags: needinfo?(cpearce)
(Assignee)

Comment 61

3 years ago
Same for android api 9. Do we even have a way to play MP4/h264 (on try machines that is. I would say no, so returning "" here seems correct too).
Flags: needinfo?(snorp)
(In reply to Jean-Yves Avenard [:jya] from comment #60)
> Chris,
> In https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f466bb4d8f0
> 
> 
> It shows that Windows XP canPlayType errors due to it returning ""
> 
> But that is the proper behaviour; there's no h264 or AAC decoder on Windows. 
> Why would it ever return maybe?

We shouldn't have AAC or H.264 decoders on XP in mochitests unless the BlankDecoderModule is enabled, and I don't think it should be in this test.

Are your patches causing the pref media.ffmpeg.enabled to be true? That would cause the test to assume that H.264/AAC are supported:

http://mxr.mozilla.org/mozilla-central/source/dom/media/test/test_can_play_type_mpeg.html?force=1#131
Flags: needinfo?(cpearce)
(Assignee)

Comment 63

3 years ago
No no. The issue is *without* the changes. 
With the changes in, I now get a failure that canPlayType is returning "" when "maybe" was expected.
What I'm saying is wrong is that "maybe" was expected in the first place
(Assignee)

Comment 64

3 years ago
Oh sorry. I misread your message. I see, just having the pref on change the expected behaviour of the mochitest.
Flags: needinfo?(snorp)
(Assignee)

Comment 65

3 years ago
Created attachment 8703298 [details] [diff] [review]
P7. Do not assume having ffmpeg enabled provides h264 or mp3 decoding.

FFmpeg on Windows or Mac only provides VP9 and VP8 decoding
Attachment #8703298 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Depends on: 1236167
(Assignee)

Updated

3 years ago
Depends on: 1236384

Comment 66

3 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #65)
> Created attachment 8703298 [details] [diff] [review]
> P7. Do not assume having ffmpeg enabled provides h264 or mp3 decoding.
> 
> FFmpeg on Windows or Mac only provides VP9 and VP8 decoding


Just wondering, Why not allow more codec decoding? We wouldn't need the VLC FF plugin and we'll be finally able to play google music natively without flash.
(Assignee)

Comment 67

3 years ago
They aren't free, unencumbered codecs.

We have h264, aac and mp3 decoder using the OS decoders. Unless you're on Windows XP there should be no need for flash to play google music. You should take the issue with google on why they use flash. Lack of decoders in Firefox is unlikely the reason.
(Assignee)

Updated

3 years ago
Depends on: 1236746
Attachment #8703298 - Flags: review?(cpearce) → review+
(Assignee)

Updated

3 years ago
Depends on: 1237210
(Assignee)

Updated

3 years ago
Assignee: nobody → jyavenard

Updated

3 years ago
Depends on: 1239550
(In reply to Gervase Markham [:gerv] from comment #15)
> Comment on attachment 8687744 [details] [diff] [review]
> P3. Add FFmpeg license's link to about:license.
> 
> Review of attachment 8687744 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=gerv. Good catch.

Gerv, Jean-Yves added a link to ffmpeg.org under the "GNU Lesser General Public License 2.1" section of the about:license page. Do we also need to mention which source directories contain the ffmpeg code? For example, the "jemalloc License" section says:

> This license applies to files in the directories memory/mozjemalloc/ and memory/jemalloc/.
Flags: needinfo?(gerv)
No, that's fine. The "source directories" lines are informational rather than mandatory, and it seems like we aren't bothering for the LGPL section.

Gerv
Flags: needinfo?(gerv)

Updated

2 years ago
Depends on: 1244773

Updated

2 years ago
Depends on: 1260362
Depends on: 1262727
You need to log in before you can comment on or make changes to this bug.