Bug 2051008

Taking this review.

As this will require Fedora Legal review, blocking this on that while I proceed with the review.

Initial spec review:

* Is the ffmpeg_clean_sources.sh script safe to run on an already cleaned tarball? If it is, it'd be good to run against the sources in %prep, like we do for OpenSSL.
* fedora-review complains about the Source0 mismatch. Looking at other packages with cleaned sources (chromium, openssl), the Source0 doesn't have a URL when using cleaned sources. Please make a similar adjustment here.
* This package is missing a BR on gcc and make, which are required for this package to build.


> CFLAGS="%{optflags}" \
> %if %{with lto}
> LDFLAGS="%{_lto_cflags}" \
> %endif

This can be simplified to "%set_build_flags", which handles this logic properly for you (as you disable LTO further up in the spec properly)

>     --optflags="%{optflags}" \
>     --extra-cflags="%{optflags}" \

Is this actually needed? If so, it probably makes sense to use "%{build_cflags}" for --extra-cflags, though.

> %{_libdir}/libavcodec.so.%{av_codec_soversion}*
> %{_libdir}/libavdevice.so.%{av_device_soversion}*
> %{_libdir}/libavfilter.so.%{av_filter_soversion}*
> %{_libdir}/libavformat.so.%{av_format_soversion}*
> %{_libdir}/libavutil.so.%{av_util_soversion}*
> %{_libdir}/libpostproc.so.%{postproc_soversion}*
> %{_libdir}/libswresample.so.%{swresample_soversion}*
> %{_libdir}/libswscale.so.%{swscale_soversion}*

It would be better to use "{,.*}" instead of "*" here, since that ensures that we don't get odd matches for soversions (it's more explicit).

The ffmpeg_clean_source.sh currently needs a build.log from a build with full tarball to gather the files getting compiled. This should make it easier to detect new files which need to be compiled. So you can't run it on a cleaned source ...

The Source0 currently allows that the ffmpeg_clean_source.sh can download the file if needed. Also it indicates from which tarball we created the clean tarball. I would prefer to leave it like this.

Will fix the others.

Thanks for the review.

(In reply to Andreas Schneider from comment #4)
> The ffmpeg_clean_source.sh currently needs a build.log from a build with
> full tarball to gather the files getting compiled. This should make it
> easier to detect new files which need to be compiled. So you can't run it on
> a cleaned source ...
> 
> The Source0 currently allows that the ffmpeg_clean_source.sh can download
> the file if needed. Also it indicates from which tarball we created the
> clean tarball. I would prefer to leave it like this.
> 

Please make a comment noting this then, it's a deliberate mismatch and it should be noted.

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-2.fc35.src.rpm

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-3.fc35.src.rpm

## Creating the 'clean' tarball

1. Update the version in the spec file
2. Do a full build locally: `fedpkg mockbuild --with full_build`
3. Create the 'clean' tarball: `./ffmpeg_clean_sources.sh path/to/build.log`

## Creating the 'clean' tarball

1. Update the `Version` in the spec file
2. Set the `Release` to 0
2. Do a full build locally: `fedpkg mockbuild --with full_build`
3. Create the 'clean' tarball: `./ffmpeg_clean_sources.sh results_ffmpeg/5.0/0.fc35/build.log`
4. Set the `Release` to 1

Please use ffmpeg-free

> make alltools V=1

This should be "%make_build alltools" so it does parallel make.

(In reply to Nicolas Chauvet (kwizart) from comment #10)
> Please use ffmpeg-free

Why? Ideally we'd be able to expand our ffmpeg over time...

> %if %{with upstream_tarball}
> gpgv2 --quiet --keyring %{SOURCE2} %{SOURCE1} %{SOURCE0}
> %endif

This should be the first thing in %prep, rather than in %build

It's still missing BRs on gcc and make.

(In reply to Neal Gompa from comment #12)
> (In reply to Nicolas Chauvet (kwizart) from comment #10)
> > Please use ffmpeg-free
> 
> Why? Ideally we'd be able to expand our ffmpeg over time...

Also, we generally want our packages to be named what the source project is named.

Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_general_naming

(In reply to Neal Gompa from comment #14)
.
> > Why? Ideally we'd be able to expand our ffmpeg over time...


Not what is concern disabled software as there are enough cases of fedora

We (as rpmfusion) do not want to complement this package as this will break many software expecting full (upstream, non disabled) ffmpeg at runtime to not crash.

Also, we don't want to back the remote work of making this package compatible to the non-replacement policy of rpmfusion.
So because of that, I'm going to suspend the non replacement policy if ever this unbacked package is accepted as ffmpeg.

I'm personally emotionally exhausted by this kind of felony toward our project (rpmfusion) sustainability.

(In reply to Andreas Schneider from comment #0)

> 
> This is a stripped down version of ffmpeg only including royalty free
> codecs! The idea is that we have the same package in rpmfusion and you would
> be able to replace it with ffmpeg including more codecs.

So you want the current rpmfusion ffmpeg package with an epoch to replace the fedora version once the rpmfusion repo is enabled?

You can't enable fdk without making the ffmpeg package unredistributable, fdk-acc-free doesn't mitigate it.

EXTERNAL_LIBRARY_NONFREE_LIST="
    decklink
    libfdk_aac
    libtls


  --enable-nonfree         allow use of nonfree code, the resulting libs
                           and binaries will be unredistributable [no]

(In reply to Nicolas Chauvet (kwizart) from comment #15)
> (In reply to Neal Gompa from comment #14)
> .
> > > Why? Ideally we'd be able to expand our ffmpeg over time...
> 
> 
> Not what is concern disabled software as there are enough cases of fedora
> 
> We (as rpmfusion) do not want to complement this package as this will break
> many software expecting full (upstream, non disabled) ffmpeg at runtime to
> not crash.
> 
> Also, we don't want to back the remote work of making this package
> compatible to the non-replacement policy of rpmfusion.
> So because of that, I'm going to suspend the non replacement policy if ever
> this unbacked package is accepted as ffmpeg.
> 
> I'm personally emotionally exhausted by this kind of felony toward our
> project (rpmfusion) sustainability.

Rather than getting angry at us about this, let's ask Dominik which way he prefers. He's the RPM Fusion maintainer for ffmpeg anyway, so it's his call.

Dominik, what would you prefer?

1. "ffmpeg" in Fedora and "ffmpeg-freeworld" in RPM Fusion
2. "ffmpeg-free" in Fedora and "ffmpeg" in RPM Fusion

There are trade-offs for each choice:

Option 1 means that packages that explicitly request the ffmpeg package by name will be able to have their dependencies satisfied by the Fedora one. This means that there may be some short-term pain by applications that expect more codecs from our ffmpeg than are available at this time. However, this will force us to find avenues/mechanisms to dynamically expand codec availability sooner rather than later.

Option 2 means that packages that explicitly request the ffmpeg package by name will not work without RPM Fusion. This avoids the short-term pain I mentioned in option 1, but eliminates our incentive to work on dynamically expanding codec availability in the package in Fedora.

Note that neither choice requires Epoch bumps or any such EVR mangling, since the names would be different.

(In reply to leigh scott from comment #16)
> (In reply to Andreas Schneider from comment #0)
> 
> > 
> > This is a stripped down version of ffmpeg only including royalty free
> > codecs! The idea is that we have the same package in rpmfusion and you would
> > be able to replace it with ffmpeg including more codecs.
> 
> So you want the current rpmfusion ffmpeg package with an epoch to replace
> the fedora version once the rpmfusion repo is enabled?
> 
> You can't enable fdk without making the ffmpeg package unredistributable,
> fdk-acc-free doesn't mitigate it.
> 
> EXTERNAL_LIBRARY_NONFREE_LIST="
>     decklink
>     libfdk_aac
>     libtls
> 
> 
>   --enable-nonfree         allow use of nonfree code, the resulting libs
>                            and binaries will be unredistributable [no]

Per bug 1501522 comment 112, Red Hat Legal has declared that the clause that led to FFmpeg upstream putting it in the nonfree section[1] is a no-op, and fdk-aac-free does not have a GPL incompatibility in practice. Fedora's ffmpeg package, which explicitly builds against fdk-aac-free, could patch to move it out of the nonfree list. I would prefer to have it linked so that users can choose to swap implementations freely as they desire.

That said, since this BZ already is blocked on FE-Legal, we can also ask Fedora Legal to check and see if we can enable the built-in AAC codec in FFmpeg, as I believe it supports the same algorithms that our fdk-aac-free package does. If they are okay with that codec, we can enable that and put the question around linking to fdk-aac-free off for later.

[1]: https://github.com/FFmpeg/FFmpeg/commit/87246953d8424b52aeb975f22c18f9ee690751ba

(In reply to Nicolas Chauvet (kwizart) from comment #15)
> (In reply to Neal Gompa from comment #14)
> .
> > > Why? Ideally we'd be able to expand our ffmpeg over time...
> 
> 
> Not what is concern disabled software as there are enough cases of fedora
> 
> We (as rpmfusion) do not want to complement this package as this will break
> many software expecting full (upstream, non disabled) ffmpeg at runtime to
> not crash.

This works just fine for openSUSE/Packman since 2015 ...

(In reply to leigh scott from comment #16)
> (In reply to Andreas Schneider from comment #0)
> 
> > 
> > This is a stripped down version of ffmpeg only including royalty free
> > codecs! The idea is that we have the same package in rpmfusion and you would
> > be able to replace it with ffmpeg including more codecs.
> 
> So you want the current rpmfusion ffmpeg package with an epoch to replace
> the fedora version once the rpmfusion repo is enabled?

There is no need for an epoch, you just need a higher release number set.

> You can't enable fdk without making the ffmpeg package unredistributabe,
> fdk-acc-free doesn't mitigate it.
> 
> EXTERNAL_LIBRARY_NONFREE_LIST="
>     decklink
>     libfdk_aac
>     libtls
> 
> 
>   --enable-nonfree         allow use of nonfree code, the resulting libs
>                            and binaries will be unredistributable [no]

We could also dlopen() it. Would that work?

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-4.fc35.src.rpm

(In reply to Neal Gompa from comment #17)
> Dominik, what would you prefer?
> 
> 1. "ffmpeg" in Fedora and "ffmpeg-freeworld" in RPM Fusion
> 2. "ffmpeg-free" in Fedora and "ffmpeg" in RPM Fusion
> 
> There are trade-offs for each choice:
> 
> Option 1 means that packages that explicitly request the ffmpeg package by
> name will be able to have their dependencies satisfied by the Fedora one.
> This means that there may be some short-term pain by applications that
> expect more codecs from our ffmpeg than are available at this time. However,
> this will force us to find avenues/mechanisms to dynamically expand codec
> availability sooner rather than later.

We can only wait for patents to expire, so there's not much we can do, no
matter the incentives.

> Option 2 means that packages that explicitly request the ffmpeg package by
> name will not work without RPM Fusion. This avoids the short-term pain I
> mentioned in option 1, but eliminates our incentive to work on dynamically
> expanding codec availability in the package in Fedora.

I'd go with option 2. ffmpeg in RPM Fusion was "first" and is the fully
featured package. I'd be happy to co-maintain ffmpeg-free in Fedora, too.

(In reply to Andreas Schneider from comment #20)
> (In reply to leigh scott from comment #16)
[...]
> > You can't enable fdk without making the ffmpeg package unredistributabe,
> > fdk-acc-free doesn't mitigate it.
> > 
> > EXTERNAL_LIBRARY_NONFREE_LIST="
> >     decklink
> >     libfdk_aac
> >     libtls
> > 
> > 
> >   --enable-nonfree         allow use of nonfree code, the resulting libs
> >                            and binaries will be unredistributable [no]

That needs to be patched considering RH legal approved it:
diff -up ffmpeg-5.0/configure.orig ffmpeg-5.0/configure
--- ffmpeg-5.0/configure.orig   2022-01-17 14:41:41.369212587 +0100
+++ ffmpeg-5.0/configure        2022-01-20 23:43:27.370353154 +0100
@@ -1783,7 +1783,6 @@ EXTERNAL_LIBRARY_GPL_LIST="

 EXTERNAL_LIBRARY_NONFREE_LIST="
     decklink
-    libfdk_aac
     libtls
 "

@@ -1822,6 +1821,7 @@ EXTERNAL_LIBRARY_LIST="
     libdav1d
     libdc1394
     libdrm
+    libfdk_aac
     libflite
     libfontconfig
     libfreetype

> We could also dlopen() it. Would that work?

There's no legal difference as far as I understand (IANAL) and I don't like carrying non-upstream patches forever.

I'll post more comments shortly.

Note that when linking with fdk-aac-free, you need to disable all GPL codecs as FDK-AAC license is GPL-incompatible (but LGPL-compatible).

Here's the list:
%global _without_amr      1
%global _without_cdio     1
%global _without_frei0r   1
%global _without_gmp      1
%global _without_gpl      1
%global _without_lensfun  1
%global _without_rubberband 1
%global _without_smb      1
%global _without_vidstab  1
%global _without_vmaf     1
%global _without_x264     1
%global _without_x265     1
%global _without_xvid     1


Now, this part:
    --disable-parser="h263,h264,hevc,vc1" \
    --enable-muxers \
    --disable-muxer="mp4,h263,h264,hevc,vc1" \
    --enable-demuxers \
    --disable-demuxer="mp4,h263,h264,hevc,vc1" \
    --disable-encoders \
    --disable-decoders \
    --disable-decoder="mpeg4,h263,h264,hevc,vc1" \

I think this will break all hardware en/decoders. You need to keep parsers and (de)muxers enabled. Considering mkvtoolnix and other tools which can parse and demux the above are in Fedora already, this should be legal to do.

Also, there's no point in disabling the specific set of decoders if you have --disable-decoders in the line above.

# Paranoia check
%if %{without all_codecs}
for i in MPEG4 H263 H264 HEVC VC1; do
    grep -q "#define CONFIG_${i}_DECODER 0" config.h
done
%endif

You're forgetting about encoders. ;)

(In reply to Dominik 'Rathann' Mierzejewski from comment #23)
> Note that when linking with fdk-aac-free, you need to disable all GPL codecs
> as FDK-AAC license is GPL-incompatible (but LGPL-compatible).

If we don't have h264 support, why should we enable aac then? Also there is https://github.com/knik0/faac which is LGPL. Looks like it ffmpeg has support for that.

> Here's the list:
> %global _without_amr      1
> %global _without_cdio     1
> %global _without_frei0r   1
> %global _without_gmp      1
> %global _without_gpl      1
> %global _without_lensfun  1
> %global _without_rubberband 1
> %global _without_smb      1
> %global _without_vidstab  1
> %global _without_vmaf     1
> %global _without_x264     1
> %global _without_x265     1
> %global _without_xvid     1
> 
> 
> Now, this part:
>     --disable-parser="h263,h264,hevc,vc1" \
>     --enable-muxers \
>     --disable-muxer="mp4,h263,h264,hevc,vc1" \
>     --enable-demuxers \
>     --disable-demuxer="mp4,h263,h264,hevc,vc1" \
>     --disable-encoders \
>     --disable-decoders \
>     --disable-decoder="mpeg4,h263,h264,hevc,vc1" \
> 
> I think this will break all hardware en/decoders. You need to keep parsers
> and (de)muxers enabled. Considering mkvtoolnix and other tools which can
> parse and demux the above are in Fedora already, this should be legal to do.

Without --disable-decoder="mpeg4,h263,h264,hevc,vc1" the paranoia check triggers! I will remove --disable-parsers and --disable-muxers stuff.

> > Option 2 means that packages that explicitly request the ffmpeg package by
> > name will not work without RPM Fusion. This avoids the short-term pain I
> > mentioned in option 1, but eliminates our incentive to work on dynamically
> > expanding codec availability in the package in Fedora.
> 
> I'd go with option 2. ffmpeg in RPM Fusion was "first" and is the fully
> featured package. I'd be happy to co-maintain ffmpeg-free in Fedora, too.

Why not have the same name in Fedora and rpmfusion? It works fine for openSUSE/Packman since 2015.

(In reply to Andreas Schneider from comment #25)
> > > Option 2 means that packages that explicitly request the ffmpeg package by
> > > name will not work without RPM Fusion. This avoids the short-term pain I
> > > mentioned in option 1, but eliminates our incentive to work on dynamically
> > > expanding codec availability in the package in Fedora.
> > 
> > I'd go with option 2. ffmpeg in RPM Fusion was "first" and is the fully
> > featured package. I'd be happy to co-maintain ffmpeg-free in Fedora, too.
> 
> Why not have the same name in Fedora and rpmfusion? It works fine for
> openSUSE/Packman since 2015.

It works in openSUSE because they have sticky vendors on by default. While we have the feature in DNF, we do not enable it by default yet.

The sticky vendors feature means that if a package name is provided by one vendor vs another, the package manager won't auto-switch unless the user specifically requests the switch.

Maybe one day we could make a Fedora Change to turn it on, but for now, we lack that feature.

(In reply to Neal Gompa from comment #26)
> Maybe one day we could make a Fedora Change to turn it on, but for now, we
> lack that feature.

I cannot imagine why we don't have that turned on if it already exists. Can we please enable that?

(In reply to Michael Catanzaro from comment #27)
> (In reply to Neal Gompa from comment #26)
> > Maybe one day we could make a Fedora Change to turn it on, but for now, we
> > lack that feature.
> 
> I cannot imagine why we don't have that turned on if it already exists. Can
> we please enable that?

Well, I guess I can talk to the DNF people about making a F37 Change for it.

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-5.fc35.src.rpm

changelog
* Mon Feb 07 2022 Andreas Schneider <asn> - 5.0-5
- Improved paranoia checks
- Renamed binary packages to ffmpeg-free
- Enabled all parser, muxer and demuxer

> %global pkg_name ffmpeg-free
> 
> # If you want to do a build with the upstream source tarball, then set the
> # clean_suffix to %%nil. We can't handle this with a conditional, as srpm
> # generation would not take it into account.
> %global clean_suffix -clean

Couldn't we unify this like so?

> # If you want to do a build with the upstream source tarball, then set the
> # clean_suffix to %%nil. We can't handle this with a conditional, as srpm
> # generation would not take it into account.
> %global clean_suffix -free
> 
> %global pkg_name %{name}%{?clean_suffix}

That way, we could also make the controls simpler for doing flavor builds.

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-6.fc35.src.rpm

changelog
* Tue Feb 08 2022 Andreas Schneider <asn> - 5.0-6
- Spec file improvements

> Requires:       %{name}-libs%{?_isa} = %{version}-%{release}

This needs to be conditioned or adjusted accordingly.

> Requires:       libavdevice%{?flavor}%{_isa} = %{version}-%{release}

Where is this package from? I don't see it produced in here, I think you mean to drop this.

Also, all "%{_isa}" should be "%{?_isa}".

> %autosetup -p1 -n %{name}-%{version}

This can just be "%autosetup -p1".

(In reply to Neal Gompa from comment #33)
> > Requires:       libavdevice%{?flavor}%{_isa} = %{version}-%{release}
> 
> Where is this package from? I don't see it produced in here, I think you
> mean to drop this.

It is present in RPM Fusion package. I split libavdevice to a separate package based on user request:

https://bugzilla.rpmfusion.org/show_bug.cgi?id=3075

> Also, all "%{_isa}" should be "%{?_isa}".

No, they shouldn't. _isa is always defined as long as rpm is installed:

$ grep _isa /usr/lib/rpm/macros
%_isa			%{?__isa:(%{__isa})}%{!?__isa:%{nil}}
$ rpm -qf /usr/lib/rpm/macros
rpm-4.17.0-4.fc35.x86_64

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-7.fc35.src.rpm

changelog
* Tue Feb 08 2022 Andreas Schneider <asn> - 5.0-7
- Spec file fixes

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-8.fc35.src.rpm

changelog
* Wed Feb 09 2022 Andreas Schneider <asn> - 5.0-8
- Build against fdk-aac-free-devel

As I understand it, this was actually okayed in 2020. Removing FE Legal blocker.

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated


Issues:
=======
- Sources used to build the package match the upstream source, as provided
  in the spec URL.
  Note: Upstream MD5sum check error, diff is in
  /home/ngompa/2051008-ffmpeg/diff.txt
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig not called in %post and %postun for Fedora 28 and later.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "GNU General Public License, Version
     2", "GNU Lesser General Public License, Version 2.1", "GNU Lesser
     General Public License, Version 3", "*No copyright* GNU General Public
     License v2.0 or later", "*No copyright* GNU General Public License",
     "GNU Lesser General Public License v2.1 or later", "*No copyright* GNU
     Lesser General Public License v2.1 or later", "MIT License", "BSD (3
     clause)", "GNU General Public License v3.0 or later", "*No copyright*
     [generated file]", "GNU General Public License v2.0 or later", "*No
     copyright* MIT License", "ISC License", "BSD (3 clause) GNU Lesser
     General Public License v2.1 or later", "BSD (2 clause)", "MIT License
     GNU Lesser General Public License v2.1 or later", "BSD (2 clause) GNU
     Lesser General Public License v2.1 or later", "zlib/libpng license",
     "Boost Software License 1.0 GNU Lesser General Public License v2.1 or
     later", "*No copyright* Public domain". 256 files have unknown
     license. Detailed output of licensecheck in
     /home/ngompa/2051008-ffmpeg/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Only use %_sourcedir in very specific situations.
     Note: %_sourcedir/$RPM_SOURCE_DIR is used.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 563200 bytes in 46 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in ffmpeg-
     free , ffmpeg-free-libs , ffmpeg-free-devel
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[!]: Spec file according to URL is the same as in SRPM.
     Note: Bad spec filename: /home/ngompa/2051008-ffmpeg/srpm-
     unpacked/ffmpeg.spec
     See: (this test has no URL)
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
     Note: Arch-ed rpms have a total of 2293760 bytes in /usr/share
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).


Rpmlint
-------
Checking: ffmpeg-free-5.0-8.fc36.x86_64.rpm
          ffmpeg-free-libs-5.0-8.fc36.x86_64.rpm
          ffmpeg-free-devel-5.0-8.fc36.x86_64.rpm
          ffmpeg-debuginfo-5.0-8.fc36.x86_64.rpm
          ffmpeg-debugsource-5.0-8.fc36.x86_64.rpm
          ffmpeg-5.0-8.fc36.src.rpm
ffmpeg-free.x86_64: W: spelling-error %description -l en_US transcode -> trans code, trans-code, transcendent
ffmpeg-free.x86_64: W: spelling-error %description -l en_US mux -> mix, mu, mus
ffmpeg-free.x86_64: W: spelling-error %description -l en_US demux -> demur
ffmpeg-free-libs.x86_64: W: spelling-error %description -l en_US transcode -> trans code, trans-code, transcendent
ffmpeg-free-libs.x86_64: W: spelling-error %description -l en_US mux -> mix, mu, mus
ffmpeg-free-libs.x86_64: W: spelling-error %description -l en_US demux -> demur
ffmpeg-free-libs.x86_64: W: crypto-policy-non-compliance-gnutls-1 /usr/lib64/libavformat.so.59.16.100 gnutls_priority_set_direct
ffmpeg-free-devel.x86_64: W: spelling-error %description -l en_US transcode -> trans code, trans-code, transcendent
ffmpeg-free-devel.x86_64: W: spelling-error %description -l en_US mux -> mix, mu, mus
ffmpeg-free-devel.x86_64: W: spelling-error %description -l en_US demux -> demur
ffmpeg.src: W: spelling-error %description -l en_US transcode -> trans code, trans-code, transcendent
ffmpeg.src: W: spelling-error %description -l en_US mux -> mix, mu, mus
ffmpeg.src: W: spelling-error %description -l en_US demux -> demur
ffmpeg.src: W: strange-permission ffmpeg_clean_sources.sh 755
ffmpeg.src:351: E: use-of-RPM_SOURCE_DIR
ffmpeg.src:474: W: macro-in-%changelog %prep
ffmpeg.src: W: file-size-mismatch ffmpeg.keyring = 1203, https://ffmpeg.org/ffmpeg-devel.asc#/ffmpeg.keyring = 1709
ffmpeg.src: W: invalid-url Source0: ffmpeg-free-5.0.tar.xz
6 packages and 0 specfiles checked; 1 errors, 17 warnings.




Rpmlint (debuginfo)
-------------------
Checking: ffmpeg-free-libs-debuginfo-5.0-8.fc36.x86_64.rpm
          ffmpeg-debuginfo-5.0-8.fc36.x86_64.rpm
          ffmpeg-free-debuginfo-5.0-8.fc36.x86_64.rpm
3 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Source checksums
----------------
https://ffmpeg.org/ffmpeg-devel.asc#/ffmpeg.keyring :
  CHECKSUM(SHA256) this package     : fd543a75d3bba61b759ca25b135e6f91b34aa37ae6987316e5c636bf3c78ae8d
  CHECKSUM(SHA256) upstream package : 397b3becedcd5a98769967ff1ff8501ddc89f8368b8f766e4701377d7dbaabe5
https://ffmpeg.org/releases/ffmpeg-5.0.tar.xz.asc :
  CHECKSUM(SHA256) this package     : 25fc8a9de93e3ff086451bf5780704368675612e39c8209c31802f057281baed
  CHECKSUM(SHA256) upstream package : 25fc8a9de93e3ff086451bf5780704368675612e39c8209c31802f057281baed
diff -r also reports differences


Requires
--------
ffmpeg-free (rpmlib, GLIBC filtered):
    ffmpeg-free-libs(x86-64)
    glibc
    libSDL2-2.0.so.0()(64bit)
    libavcodec.so.59.18()(64bit)
    libavcodec.so.59.18(LIBAVCODEC_59)(64bit)
    libavdevice.so.59.4()(64bit)
    libavdevice.so.59.4(LIBAVDEVICE_59)(64bit)
    libavfilter.so.8.24()(64bit)
    libavfilter.so.8.24(LIBAVFILTER_8)(64bit)
    libavformat.so.59.16()(64bit)
    libavformat.so.59.16(LIBAVFORMAT_59)(64bit)
    libavutil.so.57.17()(64bit)
    libavutil.so.57.17(LIBAVUTIL_57)(64bit)
    libc.so.6()(64bit)
    libm.so.6()(64bit)
    libpostproc.so.56.3()(64bit)
    libpostproc.so.56.3(LIBPOSTPROC_56)(64bit)
    libswresample.so.4.3()(64bit)
    libswresample.so.4.3(LIBSWRESAMPLE_4)(64bit)
    libswscale.so.6.4()(64bit)
    libswscale.so.6.4(LIBSWSCALE_6)(64bit)
    rtld(GNU_HASH)

ffmpeg-free-libs (rpmlib, GLIBC filtered):
    glibc
    libSDL2-2.0.so.0()(64bit)
    libSvtAv1Enc.so.0()(64bit)
    libX11.so.6()(64bit)
    libaom.so.3()(64bit)
    libasound.so.2()(64bit)
    libasound.so.2(ALSA_0.9)(64bit)
    libasound.so.2(ALSA_0.9.0rc4)(64bit)
    libass.so.9()(64bit)
    libavcodec.so.59.18()(64bit)
    libavcodec.so.59.18(LIBAVCODEC_59)(64bit)
    libavfilter.so.8.24()(64bit)
    libavfilter.so.8.24(LIBAVFILTER_8)(64bit)
    libavformat.so.59.16()(64bit)
    libavformat.so.59.16(LIBAVFORMAT_59)(64bit)
    libavutil.so.57.17()(64bit)
    libavutil.so.57.17(LIBAVUTIL_57)(64bit)
    libbluray.so.2()(64bit)
    libbs2b.so.0()(64bit)
    libbz2.so.1()(64bit)
    libc.so.6()(64bit)
    libcdio_cdda.so.2()(64bit)
    libcdio_cdda.so.2(CDIO_CDDA_2)(64bit)
    libcdio_paranoia.so.2()(64bit)
    libcdio_paranoia.so.2(CDIO_PARANOIA_2)(64bit)
    libdav1d.so.5()(64bit)
    libdc1394.so.25()(64bit)
    libdrm.so.2()(64bit)
    libfontconfig.so.1()(64bit)
    libfreetype.so.6()(64bit)
    libfribidi.so.0()(64bit)
    libgnutls.so.30()(64bit)
    libgnutls.so.30(GNUTLS_3_4)(64bit)
    libgsm.so.1()(64bit)
    libjack.so.0()(64bit)
    liblzma.so.5()(64bit)
    liblzma.so.5(XZ_5.0)(64bit)
    libm.so.6()(64bit)
    libmp3lame.so.0()(64bit)
    libmysofa.so.1()(64bit)
    libopenjp2.so.7()(64bit)
    libopenmpt.so.0()(64bit)
    libopus.so.0()(64bit)
    libpostproc.so.56.3()(64bit)
    libpostproc.so.56.3(LIBPOSTPROC_56)(64bit)
    libpulse.so.0()(64bit)
    libpulse.so.0(PULSE_0)(64bit)
    librav1e.so.0()(64bit)
    librubberband.so.2()(64bit)
    libsmbclient.so.0()(64bit)
    libsmbclient.so.0(SMBCLIENT_0.1.0)(64bit)
    libsoxr.so.0()(64bit)
    libspeex.so.1()(64bit)
    libsrt.so.1.4()(64bit)
    libssh.so.4()(64bit)
    libssh.so.4(LIBSSH_4_5_0)(64bit)
    libswresample.so.4.3()(64bit)
    libswresample.so.4.3(LIBSWRESAMPLE_4)(64bit)
    libswscale.so.6.4()(64bit)
    libswscale.so.6.4(LIBSWSCALE_6)(64bit)
    libtheoradec.so.1()(64bit)
    libtheoradec.so.1(libtheoradec_1.0)(64bit)
    libtheoraenc.so.1()(64bit)
    libtheoraenc.so.1(libtheoraenc_1.0)(64bit)
    libtwolame.so.0()(64bit)
    libv4l2.so.0()(64bit)
    libva-drm.so.2()(64bit)
    libva-x11.so.2()(64bit)
    libva.so.2()(64bit)
    libva.so.2(VA_API_0.33.0)(64bit)
    libvdpau.so.1()(64bit)
    libvidstab.so.1.2()(64bit)
    libvmaf.so.1()(64bit)
    libvorbis.so.0()(64bit)
    libvorbisenc.so.2()(64bit)
    libvpx.so.7()(64bit)
    libwebp.so.7()(64bit)
    libwebpmux.so.3()(64bit)
    libxcb-shape.so.0()(64bit)
    libxcb-shm.so.0()(64bit)
    libxcb-xfixes.so.0()(64bit)
    libxcb.so.1()(64bit)
    libxml2.so.2()(64bit)
    libxml2.so.2(LIBXML2_2.4.30)(64bit)
    libxml2.so.2(LIBXML2_2.5.2)(64bit)
    libxml2.so.2(LIBXML2_2.6.0)(64bit)
    libxml2.so.2(LIBXML2_2.7.3)(64bit)
    libz.so.1()(64bit)
    libz.so.1(ZLIB_1.2.0)(64bit)
    libz.so.1(ZLIB_1.2.0.2)(64bit)
    libzimg.so.2()(64bit)
    rtld(GNU_HASH)

ffmpeg-free-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    ffmpeg-free-libs(x86-64)
    libavcodec.so.59.18()(64bit)
    libavdevice.so.59.4()(64bit)
    libavfilter.so.8.24()(64bit)
    libavformat.so.59.16()(64bit)
    libavutil.so.57.17()(64bit)
    libpostproc.so.56.3()(64bit)
    libswresample.so.4.3()(64bit)
    libswscale.so.6.4()(64bit)
    pkgconfig
    pkgconfig(libavcodec)
    pkgconfig(libavfilter)
    pkgconfig(libavformat)
    pkgconfig(libavutil)
    pkgconfig(libpostproc)
    pkgconfig(libswresample)
    pkgconfig(libswscale)

ffmpeg-debuginfo (rpmlib, GLIBC filtered):

ffmpeg-debugsource (rpmlib, GLIBC filtered):



Provides
--------
ffmpeg-free:
    ffmpeg-free
    ffmpeg-free(x86-64)

ffmpeg-free-libs:
    ffmpeg-free-libs
    ffmpeg-free-libs(x86-64)
    libavcodec.so.59.18()(64bit)
    libavcodec.so.59.18(LIBAVCODEC_59)(64bit)
    libavdevice.so.59.4()(64bit)
    libavdevice.so.59.4(LIBAVDEVICE_59)(64bit)
    libavfilter.so.8.24()(64bit)
    libavfilter.so.8.24(LIBAVFILTER_8)(64bit)
    libavformat.so.59.16()(64bit)
    libavformat.so.59.16(LIBAVFORMAT_59)(64bit)
    libavutil.so.57.17()(64bit)
    libavutil.so.57.17(LIBAVUTIL_57)(64bit)
    libpostproc.so.56.3()(64bit)
    libpostproc.so.56.3(LIBPOSTPROC_56)(64bit)
    libswresample.so.4.3()(64bit)
    libswresample.so.4.3(LIBSWRESAMPLE_4)(64bit)
    libswscale.so.6.4()(64bit)
    libswscale.so.6.4(LIBSWSCALE_6)(64bit)

ffmpeg-free-devel:
    ffmpeg-free-devel
    ffmpeg-free-devel(x86-64)
    pkgconfig(libavcodec)
    pkgconfig(libavdevice)
    pkgconfig(libavfilter)
    pkgconfig(libavformat)
    pkgconfig(libavutil)
    pkgconfig(libpostproc)
    pkgconfig(libswresample)
    pkgconfig(libswscale)

ffmpeg-debuginfo:
    ffmpeg-debuginfo
    ffmpeg-debuginfo(x86-64)

ffmpeg-debugsource:
    ffmpeg-debugsource
    ffmpeg-debugsource(x86-64)



Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10
Command line :/usr/bin/fedora-review -b 2051008 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: C/C++, Generic, Shell-api
Disabled plugins: PHP, R, SugarActivity, Java, fonts, Ocaml, Python, Perl, Haskell
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Fedora Review showed all the expected errors for this particular package, but the rest of it looks good, so...

PACKAGE APPROVED.

Huh, some of those rpmlint warnings sound useful ...

> ffmpeg-free-libs.x86_64: W: crypto-policy-non-compliance-gnutls-1 /usr/lib64/libavformat.so.59.16.100 gnutls_priority_set_direct

Is this expected?

> ffmpeg.src: W: strange-permission ffmpeg_clean_sources.sh 755

Does it maybe expect 775 instead? Or does it complain about it being an executable?

> ffmpeg.src:351: E: use-of-RPM_SOURCE_DIR

I guess you could copy those files into the local build directory first. Would be "cleaner".

> ffmpeg.src:474: W: macro-in-%changelog %prep

That unescaped "%prep" needs to be removed from the changelog, or escaped properly.

> ffmpeg.src: W: file-size-mismatch ffmpeg.keyring = 1203, https://ffmpeg.org/ffmpeg-devel.asc#/ffmpeg.keyring = 1709

What's this about? Why doesn't the keyring file match?
This is probably also the reason for the MD5sum mismatch error.

(In reply to Fabio Valentini from comment #41)
> Huh, some of those rpmlint warnings sound useful ...
> 
> > ffmpeg-free-libs.x86_64: W: crypto-policy-non-compliance-gnutls-1 /usr/lib64/libavformat.so.59.16.100 gnutls_priority_set_direct
> 
> Is this expected?
> 

It's an old policy from attempts to unify crypto libraries. We never completed it and I believe those checks were removed in rpmlint 2.x.

> > ffmpeg.src: W: strange-permission ffmpeg_clean_sources.sh 755
> 
> Does it maybe expect 775 instead? Or does it complain about it being an
> executable?
> 

This is some rpmlint-1.x goofiness. It should be 755, and that's what Git will store it as.

> > ffmpeg.src:351: E: use-of-RPM_SOURCE_DIR
> 
> I guess you could copy those files into the local build directory first.
> Would be "cleaner".
> 
> > ffmpeg.src:474: W: macro-in-%changelog %prep
> 
> That unescaped "%prep" needs to be removed from the changelog, or escaped
> properly.
> 

Hmm, yeah, that's true. I glossed over this one in the sea of the others.

> > ffmpeg.src: W: file-size-mismatch ffmpeg.keyring = 1203, https://ffmpeg.org/ffmpeg-devel.asc#/ffmpeg.keyring = 1709
> 
> What's this about? Why doesn't the keyring file match?
> This is probably also the reason for the MD5sum mismatch error.

Looks like the keyring file got updated.

Andreas, when you get this imported, please add "ngompa" and "rathann" as admins so we can co-maintain this. :)

(In reply to Neal Gompa from comment #42)
> (In reply to Fabio Valentini from comment #41)
> > Huh, some of those rpmlint warnings sound useful ...
> > 
> > > ffmpeg-free-libs.x86_64: W: crypto-policy-non-compliance-gnutls-1 /usr/lib64/libavformat.so.59.16.100 gnutls_priority_set_direct
> > 
> > Is this expected?
> > 
> 
> It's an old policy from attempts to unify crypto libraries. We never
> completed it and I believe those checks were removed in rpmlint 2.x.

No, that's not old, that's really the current crypto policy. gnutls_priority_set_direct() is suspicious because it almost always ignores the system policy, so it is rarely ever OK to use that function in Fedora. We probably need to patch the code to use either gnutls_set_default_priority() or gnutls_set_default_priority_append() instead. See https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/ for guidance.

In certain circumstances, it might be OK to use gnutls_priority_set_direct() if it's gated behind manual user configuration and there is a good reason to ignore system policy, but this is rare.

In this particular case, see: https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/tls_gnutls.c#L195

The code calls:

gnutls_priority_set_direct(p->session, "NORMAL", NULL);

Which means: "disable Fedora crypto policy, and use upstream default crypto policy instead." That's not OK and needs to be fixed to use gnutls_set_default_priority() instead. That change should be accepted upstream too, since if there is no system policy, it's going to set the NORMAL priority

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-8.fc35.src.rpm

changelog
* Thu Feb 10 2022 Andreas Schneider <asn> - 5.0-9
- Create a subpackage for each library

I will fix the rest tomorrow.

> Provides:       libavcodec%{?pkg_suffix}-devel = %{version}-%{release}
> Obsoletes:      libavcodec%{?pkg_suffix}-devel < %{version}-%{release}

This is not needed.

> Provides:       libavdevice%{?pkg_suffix}-devel = %{version}-%{release}
> Obsoletes:      libavdevice%{?pkg_suffix}-devel < %{version}-%{release}

Not needed.

> Provides:       libavfilter-devel = %{version}-%{release}
> Obsoletes:      libavfilter-devel < %{version}-%{release}

Wrong and not needed.

> Provides:       libavformat%{?pkg_suffix}-devel = %{version}-%{release}
> Obsoletes:      libavformat%{?pkg_suffix}-devel < %{version}-%{release}

Not needed.

> Provides:       libavutil%{?pkg_suffix}-devel = %{version}-%{release}
> Obsoletes:      libavutil%{?pkg_suffix}-devel < %{version}-%{release}

Not needed.

> Provides:       libpostproc%{?pkg_suffix}-devel = %{version}-%{release}
> Obsoletes:      libpostproc%{?pkg_suffix}-devel < %{version}-%{release}

Not needed.

> Provides:       libswresample%{?pkg_suffix}-devel = %{version}-%{release}
> Obsoletes:      libswresample%{?pkg_suffix}-devel < %{version}-%{release}

Not needed.

> Provides:       libswscale%{?pkg_suffix}-devel = %{version}-%{release}
> Conflicts:      libswscale%{?pkg_suffix}-devel < %{version}-%{release}

Not needed.

> %ldconfig_scriptlets  libavcodec%{?pkg_suffix}
> %ldconfig_scriptlets  libavdevice%{?pkg_suffix}
> %ldconfig_scriptlets  libavfilter%{?pkg_suffix}
> %ldconfig_scriptlets  libavformat%{?pkg_suffix}
> %ldconfig_scriptlets  libavutil%{?pkg_suffix}
> %ldconfig_scriptlets  libpostproc%{?pkg_suffix}
> %ldconfig_scriptlets  libswresample%{?pkg_suffix}
> %ldconfig_scriptlets  libswscle%{?pkg_suffix}

This is missing "-n", so it should be like so:

%ldconfig_scriptlets -n libavcodec%{?pkg_suffix}
%ldconfig_scriptlets -n libavdevice%{?pkg_suffix}
%ldconfig_scriptlets -n libavfilter%{?pkg_suffix}
%ldconfig_scriptlets -n libavformat%{?pkg_suffix}
%ldconfig_scriptlets -n libavutil%{?pkg_suffix}
%ldconfig_scriptlets -n libpostproc%{?pkg_suffix}
%ldconfig_scriptlets -n libswresample%{?pkg_suffix}
%ldconfig_scriptlets -n libswscle%{?pkg_suffix}

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-10.fc35.src.rpm

changelog
* Fri Feb 11 2022 Andreas Schneider <asn> - 5.0-10
- Fixed gnutls to use default policy
- Don't use URL for keyring
- Fix use-of-RPM_SOURCE_DIR warning
- Remove provides/obsolets
- Fix scriptlets

(In reply to Neal Gompa from comment #26)
> (In reply to Andreas Schneider from comment #25)
...
> > Why not have the same name in Fedora and rpmfusion? It works fine for
> > openSUSE/Packman since 2015.
> 
> It works in openSUSE because they have sticky vendors on by default. While
> we have the feature in DNF, we do not enable it by default yet.

Fedora/RPM Fusion share a "non-replacement package policy", should one pick a ffmpeg-free package that fits one very limited use-case, we(rpmfusion) will not replace it.

There is no "library replacement policy" which leave a room for a "freeworld" kind of package, but there is a miss-understanding here. We cannot assume that for ffmpeg, as many softwares assume a full featured ffmpeg behavior.
Also as mandatory for a freeworld kind of package, it "MUST" be made from the same spec file (and maintained with the same team I would say) with a very limited set of options turned on/off. That to avoid ABI incompatibility.

Having ffmpeg of two kind in two repositories leaves us in the pre RPM Fusion area where different libraries were competing. 

As I'm concerned, I'm not going to back this package nor any induced charge in our project to adapt in a sustainable way, so we will take countermeasure for this package not to interfere with our project.

I'm also going to escalate to the board to prevent such conflict with lame packager overriding ours packages.

As for the packagers involved in our project.

There is not much written guidelines, but as written here concerning decision making: https://rpmfusion.org/SteeringCommittee
"* Issue are raised on the list, etc"
This is to prevent unilateral one packager decision without seeking for a consensus.

There are many cases where fdk-aac-free and ffmpeg-free was discussed in our mailing list. So this isn't a new topic.

Violating our guideline for our project leave a question of some members. They might need to be coherent.

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-11.fc35.src.rpm

changelog
* Fri Feb 11 2022 Andreas Schneider <asn> - 5.0-11
- Add patch for chromium support

As further reading the spec here are more comment to avoid interfering with our build.

The ffmpeg configure script has a mean to build libraries with a dedicated suffix such as libavcodec-free.so.58.0.0 so that any package built with that version will not clash with our.
The set of packages using this incomplete (for the least) library needs to be curated to not end with a package crashing by expecting the full featured versions while using the disabled version.
As I understand, you want this ffmpeg-free for chromium and not for general purpose consumption or any other package (blender ?)

Also it seems like this package is aimed to be introduced in el7 in this really the case ?

Also do you really have tested ffmpeg binaries ? Does it really worth it ?

$ youtube-dl NmCCQxVBfyM
$ ffmpeg -i "Original Tetris theme (Tetris Soundtrack)-NmCCQxVBfyM.webm" tetris.aac
$ ffplay tetris.aac

I'm listening to the tetris theme now. You can also try it yourself, if you're brave enough ;-)

(In reply to Andreas Schneider from comment #55)
...
> you're brave enough ;-)
Please stop been insolent !

Once again many scripts and program will assume full featured ffmpeg binaries.

Hold on. There's a ton of patches that are not explained in spec file comments or submitted upstream:

Patch0:         ffmpeg-soversion.patch
Patch1:         fix-vmaf-model-path.patch
Patch2:         ffmpeg-fix-exif-include.patch
Patch3:         ffmpeg-codec-choice.patch
Patch4:         ffmpeg-new-coder-errors.patch
# See https://bugzilla.redhat.com/show_bug.cgi?id=1501522#c112
Patch5:         ffmpeg-allow-fdk-aac-free.patch
Patch6:         ffmpeg-fix-gnutls-priority.patch
Patch7:         ffmpeg-support-chromium.patch

Without any mean to acknowledge that a particularly crippled ffmpeg binaries is used (by renaming to ffmpeg-free or using a dedicated path in libexec), you will break expectation here.

At least ffmpeg-soversion.patch will make this build ABI incompatible with RPM Fusion, so I'd say it must
be dropped here unless you can achieve a consensus with RPM Fusion maintainers to apply it.

ffmpeg-support-chromium.patch has the same issue. Chromium using this new API will break if the package is
swapped with the RPM Fusion one.

In my opinion, a package with the two above applied should not be allowed in Fedora.

ffmpeg-codec-choice.patch should be submitted upstream first and is not really necessary if you enable
hardware codecs (as I was told is legal to do).

ffmpeg-new-coder-errors.patch should also be upstreamed first.

The codec lists (enable_decoders and enable_encoders) are missing hardware codecs and have some other issues. In particular:
enable_decoders:
aac -> must be disabled, internal AAC encoder was not cleared, only libfdk_aac was (which is missing here)

Hardware codecs are missing:
h264_v4l2m2m # hardware
h264_qsv # hardware

jpeg2000 is missing
pgx is missing

enable_encoders:
aac -> must be disabled, internal AAC encoder was not cleared, only libfdk_aac was (which is missing here)
ac3 is missing

Hardware codecs are missing:
h264_qsv # hardware
h264_v4l2m2m # hardware
h264_vaapi # hardware
hevc_qsv # hardware
hevc_v4l2m2m # hardware

jpeg2000 is missing

And in both cases, are you sure mjpeg can be enabled?

$ ffmpeg -i "Original Tetris theme (Tetris Soundtrack)-NmCCQxVBfyM.webm" tetris.mp4
Automatic encoder selection failed for output stream #0:0. Default encoder for format mp4 (codec mpeg4) is probably disabled or this build of ffmpeg does not include that codec. Please choose an encoder manually.

@

(In reply to Andreas Schneider from comment #61)
> $ ffmpeg -i "Original Tetris theme (Tetris Soundtrack)-NmCCQxVBfyM.webm"
> tetris.mp4
> Automatic encoder selection failed for output stream #0:0. Default encoder
> for format mp4 (codec mpeg4) is probably disabled or this build of ffmpeg
> does not include that codec. Please choose an encoder manually.

Your comments lacks context. Perhaps you submitted it incomplete, like I did above?

After consideration, I guess I'm fine with modifying the default codec choice
to prefer free codecs over proprietary ones if not specified on the command line.
Just make sure they can be stored in .mp4.

(In reply to Andreas Schneider from comment #53)
> Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
> SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-11.fc35.src.rpm
> 
> changelog
> * Fri Feb 11 2022 Andreas Schneider <asn> - 5.0-11
> - Add patch for chromium support

FWIW this patch was rejected by FFmpeg upstream:
https://ffmpeg.org/pipermail/ffmpeg-devel/2021-September/285401.html

A solution to avoid accessing internal structures was shown as well,
so Chromium should just stop carrying this patch and follow upstream advice.

I'm against carrying this patch both here and in RPM Fusion.

Oh, and another important thing which you missed again:

License:        GPLv3+

fdk-aac-free is not compatible with GPLv3 or v2. You must disable all GPL codecs and build LGPL-licensed ffmpeg like I said in comment #23 .

    --disable-decoders \
    --disable-decoder="mpeg4,h263,h264,hevc,vc1" \

This is still redundant...

I think you missed this in comment #23 , too.

(In reply to Dominik 'Rathann' Mierzejewski from comment #65)
> Oh, and another important thing which you missed again:
> 
> License:        GPLv3+
> 
> fdk-aac-free is not compatible with GPLv3 or v2. You must disable all GPL
> codecs and build LGPL-licensed ffmpeg like I said in comment #23 .

Red Hat Legal has judged otherwise, that's why ffmpeg is patched as such.

(In reply to Dominik 'Rathann' Mierzejewski from comment #60)
> The codec lists (enable_decoders and enable_encoders) are missing hardware
> codecs and have some other issues. In particular:
> enable_decoders:
> aac -> must be disabled, internal AAC encoder was not cleared, only
> libfdk_aac was (which is missing here)
> 
> Hardware codecs are missing:
> h264_v4l2m2m # hardware
> h264_qsv # hardware
> 
> jpeg2000 is missing
> pgx is missing
> 
> enable_encoders:
> aac -> must be disabled, internal AAC encoder was not cleared, only
> libfdk_aac was (which is missing here)
> ac3 is missing
> 
> Hardware codecs are missing:
> h264_qsv # hardware
> h264_v4l2m2m # hardware
> h264_vaapi # hardware
> hevc_qsv # hardware
> hevc_v4l2m2m # hardware
> 
> jpeg2000 is missing
> 
> And in both cases, are you sure mjpeg can be enabled?

mjpeg can be enabled, we have other implementations already.

(In reply to Dominik 'Rathann' Mierzejewski from comment #65)
> Oh, and another important thing which you missed again:
> 
> License:        GPLv3+
> 
> fdk-aac-free is not compatible with GPLv3 or v2. You must disable all GPL
> codecs and build LGPL-licensed ffmpeg like I said in comment #23 .

Ok, I missed the "For this (and only this) specific implementation of fdk-aac,
we believe that the FDK AAC license is GPL compatible." part in
https://bugzilla.redhat.com/show_bug.cgi?id=1501522#c112 .
So, for this particular case, GPLv3+ build is fine. Cool.

(In reply to Nicolas Chauvet (kwizart) from comment #51)
> (In reply to Neal Gompa from comment #26)
> > (In reply to Andreas Schneider from comment #25)
> ...
> > > Why not have the same name in Fedora and rpmfusion? It works fine for
> > > openSUSE/Packman since 2015.
> > 
> > It works in openSUSE because they have sticky vendors on by default. While
> > we have the feature in DNF, we do not enable it by default yet.
> 
> Fedora/RPM Fusion share a "non-replacement package policy", should one pick
> a ffmpeg-free package that fits one very limited use-case, we(rpmfusion)
> will not replace it.
> 
> There is no "library replacement policy" which leave a room for a
> "freeworld" kind of package, but there is a miss-understanding here. We
> cannot assume that for ffmpeg, as many softwares assume a full featured
> ffmpeg behavior.

While that's true, many programs also probe FFmpeg for codecs before doing anything and handle it gracefully too. They do this because FFmpeg's codec list changes over time and applications can get stuff for free by simply doing the right thing here.

> Also as mandatory for a freeworld kind of package, it "MUST" be made from
> the same spec file (and maintained with the same team I would say) with a
> very limited set of options turned on/off. That to avoid ABI incompatibility.
> 
> Having ffmpeg of two kind in two repositories leaves us in the pre RPM
> Fusion area where different libraries were competing. 
> 
> As I'm concerned, I'm not going to back this package nor any induced charge
> in our project to adapt in a sustainable way, so we will take countermeasure
> for this package not to interfere with our project.
> 
> I'm also going to escalate to the board to prevent such conflict with lame
> packager overriding ours packages.

(In reply to Nicolas Chauvet (kwizart) from comment #56)
> (In reply to Andreas Schneider from comment #55)
> ...
> > you're brave enough ;-)
> Please stop been insolent !
> 


Please stop being rude and assuming maliciousness from people. We're all trying to increase enablement and availability of software in mainline Fedora. None of us are trying to break anyone. Within the Fedora Project, we all should be assuming positive intent and working together to make better solutions.

> Once again many scripts and program will assume full featured ffmpeg
> binaries.

If it's open source software, then we should contribute to make these applications probe for codecs correctly. FFmpeg's function interface isn't changing, just the codecs that it provides in its capabilities that you query via API.

(In reply to Dominik 'Rathann' Mierzejewski from comment #59)
> At least ffmpeg-soversion.patch will make this build ABI incompatible with
> RPM Fusion, so I'd say it must
> be dropped here unless you can achieve a consensus with RPM Fusion
> maintainers to apply it.

It also looks like this patch was dropped by openSUSE for their ffmpeg-5 package, so we probably don't need this anymore.

(In reply to Dominik 'Rathann' Mierzejewski from comment #59)
> 
> ffmpeg-support-chromium.patch has the same issue. Chromium using this new
> API will break if the package is
> swapped with the RPM Fusion one.
> 

FFmpeg in RPM Fusion has this patch applied already: https://pkgs.rpmfusion.org/cgit/free/ffmpeg.git/commit/ffmpeg.spec?id=afe251ab706f9a81a446ffb28dac43d8091f3a31

(In reply to Neal Gompa from comment #72)
> (In reply to Dominik 'Rathann' Mierzejewski from comment #59)
> > 
> > ffmpeg-support-chromium.patch has the same issue. Chromium using this new
> > API will break if the package is
> > swapped with the RPM Fusion one.
> > 
> 
> FFmpeg in RPM Fusion has this patch applied already:
> https://pkgs.rpmfusion.org/cgit/free/ffmpeg.git/commit/ffmpeg.
> spec?id=afe251ab706f9a81a446ffb28dac43d8091f3a31

I know, but Leigh said has was not opposed to reverting, which
I'm considering, given it was submitted and rejected upstream
with alternative solution suggested.

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-12.fc35.src.rpm

changelog                 
* Fri Feb 11 2022 Andreas Schneider <asn> - 5.0-12
- Drop ffmpeg-soversion.patch                                                         
- Update encoders and decoders list
  * enabled hevc/h264 hardware encoders/decoders

Spec URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg.spec
SRPM URL: https://xor.cryptomilk.org/rpm/ffmpeg/ffmpeg-5.0-13.fc35.src.rpm

changelog
* Fri Feb 11 2022 Andreas Schneider <asn> - 5.0-13
- Build with intel-mediasdk-devel for h264/hevc qsv

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/ffmpeg

FEDORA-2022-78a5cbdceb has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-78a5cbdceb

FEDORA-2022-a3794271c6 has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2022-a3794271c6

FEDORA-2022-78a5cbdceb has been pushed to the Fedora 36 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2022-a3794271c6 has been pushed to the Fedora 37 stable repository.
If problem still persists, please make note of it in this bug report.