[FFmpeg-devel] [PATCH] Allow linking to an external libpostproc

Tue Oct 29 02:38:13 EET 2024

On Mon, Oct 28, 2024 at 04:40:25PM -0300, James Almer wrote:
> On 10/28/2024 3:28 PM, Michael Niedermayer wrote:
> > Hi everyone
> > 
> > On Mon, Oct 28, 2024 at 01:24:48AM +0100, Michael Niedermayer wrote:
> > > Sponsored-by: Sovereign Tech Fund
> > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > ---
> > >   Makefile             |  3 +--
> > >   configure            | 18 ++++++++----------
> > >   fftools/ffprobe.c    |  1 -
> > >   fftools/opt_common.c |  1 -
> > >   4 files changed, 9 insertions(+), 14 deletions(-)
> > 
> > the other side of this is here:
> > 
> > https://github.com/michaelni/libpostproc
> > 
> > it should build and install and link
> > but expect bits from ffmpeg remaining in it (most not intentional, some maybe are)
> > 
> > It contains libavutil because after decades of API rewrites, we still
> > are not capable to write working and usable APIs.
> > This is not a issue of libpostproc its an issue of libavutil API
> > which is not treated like a library with a public API, but like a
> > random subdirectory that randomly gets used by other bits of code.
> > 
> > Ranting a little more, i do have to mention that libavutil and
> > libavcore or what it was called, did split a general library and the
> > multimedia parts apart. That was still a better design than now.
> > where i need to leave a copy of libavutil in the split out libpostproc
> > and no the system installed libavutil doesnt even build
> > as it tries to include headers that are not even installed with
> > HAVE_AV_CONFIG_H
> > 
> > ok one last rant bit,
> > a library has a public API
> > and NOTHING outside the library should use anything else
> > there should not be headers or features enabled in ffmpegs tree
> > that can be used by other libs. That is wrong.
> > If its usefull to us it is usefull to others, it should be a
> > proper public API or it should not be there at all
> > 
> > rant end
> > 
> > Ahh and forks and pull requests of libpostproc are very welcome!
> > 
> > and if you want to improve libpostproc, up to the point of making
> > it a full replacement of a full filter framework, thats IS welcome
> > 
> > thx
> > 
> > [...]
> 
> I didn't look too in depth, but a quick grep in git head gives me
> 
> > ~/FFmpeg/libpostproc
> > $ git grep avutil
> > Makefile:FFLIBS = avutil
> > postprocess.c:#include "libavutil/common.h"
> > postprocess.c:#include "libavutil/cpu.h"
> > postprocess.c:#include "libavutil/intreadwrite.h"
> > postprocess.c:#include "libavutil/mem.h"
> > postprocess.c:#include "libavutil/avstring.h"
> > postprocess.c:#include "libavutil/ppc/util_altivec.h"
> > postprocess_altivec_template.c:#include "libavutil/avutil.h"
> > postprocess_altivec_template.c:#include "libavutil/mem_internal.h"
> > postprocess_internal.h:#include "libavutil/avutil.h"
> > postprocess_internal.h:#include "libavutil/intmath.h"
> > postprocess_internal.h:#include "libavutil/log.h"
> > postprocess_internal.h:#include "libavutil/mem_internal.h"
> > postprocess_template.c:#include "libavutil/mem_internal.h"
> > postprocess_template.c:#include "libavutil/x86/asm.h"
> > postprocres.rc:#include "libavutil/ffversion.h"
> > version.c:#include "libavutil/ffversion.h"
> > version.h:#include "libavutil/version.h"
> 
> Of those, mem_internal.h, intmath.h, x86/asm.h, ppc/util_altivec.h and
> ffversion.h are not installed.

mem_internal you say ?
as in

libavcodec/4xm.c:#include "libavutil/mem_internal.h"
libavcodec/aac/aacdec.h:#include "libavutil/mem_internal.h"
libavcodec/aacenc.h:#include "libavutil/mem_internal.h"
libavcodec/aacps.c:#include "libavutil/mem_internal.h"
libavcodec/aacps.h:#include "libavutil/mem_internal.h"
libavcodec/aacps_fixed_tablegen.h:#include "libavutil/mem_internal.h"
libavcodec/aacps_tablegen.h:#include "libavutil/mem_internal.h"
libavcodec/aacsbr.c:#include "libavutil/mem_internal.h"
libavcodec/aacsbrdata.h:#include "libavutil/mem_internal.h"
libavcodec/aactab.c:#include "libavutil/mem_internal.h"
libavcodec/aactab.h:#include "libavutil/mem_internal.h"
libavcodec/aarch64/vp9dsp_init_16bpp_aarch64_template.c:#include "libavutil/mem_internal.h"
libavcodec/aarch64/vp9dsp_init_aarch64.c:#include "libavutil/mem_internal.h"
libavcodec/ac3dec.h:#include "libavutil/mem_internal.h"
libavcodec/ac3dsp.c:#include "libavutil/mem_internal.h"
libavcodec/ac3enc.c:#include "libavutil/mem_internal.h"
libavcodec/ac3enc.h:#include "libavutil/mem_internal.h"
libavcodec/ac3enc_template.c:#include "libavutil/mem_internal.h"
libavcodec/agm.c:#include "libavutil/mem_internal.h"
libavcodec/aic.c:#include "libavutil/mem_internal.h"
libavcodec/arm/sbcdsp_init_arm.c:#include "libavutil/mem_internal.h"
libavcodec/arm/vp9dsp_init_16bpp_arm_template.c:#include "libavutil/mem_internal.h"
libavcodec/arm/vp9dsp_init_arm.c:#include "libavutil/mem_internal.h"
libavcodec/asvdec.c:#include "libavutil/mem_internal.h"
libavcodec/asvenc.c:#include "libavutil/mem_internal.h"
libavcodec/atrac1.c:#include "libavutil/mem_internal.h"
libavcodec/atrac3.c:#include "libavutil/mem_internal.h"
libavcodec/atrac3plus.h:#include "libavutil/mem_internal.h"
libavcodec/atrac3plusdec.c:#include "libavutil/mem_internal.h"
libavcodec/atrac3plusdsp.c:#include "libavutil/mem_internal.h"
libavcodec/atrac9dec.c:#include "libavutil/mem_internal.h"

thats just the files starting with "a" and just libavcodec

then intmath:

git grep libavutil/intmath.h
libavcodec/ac3dec.c:#include "libavutil/intmath.h"
libavcodec/ac3dsp.c:#include "libavutil/intmath.h"
libavcodec/av1_parse.h:#include "libavutil/intmath.h"
libavcodec/bsf/vp9_raw_reorder.c:#include "libavutil/intmath.h"
libavcodec/cabac_functions.h:#include "libavutil/intmath.h"
libavcodec/celp_math.c:#include "libavutil/intmath.h"
libavcodec/dcamath.h:#include "libavutil/intmath.h"
libavcodec/flacenc.c:#include "libavutil/intmath.h"
libavcodec/g723_1.c:#include "libavutil/intmath.h"
libavcodec/g729postfilter.c:#include "libavutil/intmath.h"
libavcodec/h2645_parse.c:#include "libavutil/intmath.h"
libavcodec/jpegls.c:#include "libavutil/intmath.h"
libavcodec/jpegxl_parser.c:#include "libavutil/intmath.h"
libavcodec/mlpenc.c:#include "libavutil/intmath.h"
libavcodec/mpegvideo_enc.c:#include "libavutil/intmath.h"
libavcodec/nellymoser.c:#include "libavutil/intmath.h"
libavcodec/opus/enc.h:#include "libavutil/intmath.h"
libavcodec/pixlet.c:#include "libavutil/intmath.h"
libavcodec/rangecoder.h:#include "libavutil/intmath.h"
libavcodec/sbcdsp.c:#include "libavutil/intmath.h"
libavcodec/snowdec.c:#include "libavutil/intmath.h"
libavcodec/snowenc.c:#include "libavutil/intmath.h"
libavcodec/wavpack.h:#include "libavutil/intmath.h"
libavfilter/af_headphone.c:#include "libavutil/intmath.h"
libavfilter/af_sofalizer.c:#include "libavutil/intmath.h"
libavformat/options.c:#include "libavutil/intmath.h"

next you list x86/asm.h

git grep x86/asm.h
libavcodec/dirac_arith.h:#include "libavutil/x86/asm.h"
libavcodec/msmpeg4.c:#include "libavutil/x86/asm.h"
libavcodec/x86/ac3dsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/cabac.h:#include "libavutil/x86/asm.h"
libavcodec/x86/cavsdsp.c:#include "libavutil/x86/asm.h"
libavcodec/x86/constants.c:#include "libavutil/x86/asm.h" // for xmm_reg
libavcodec/x86/constants.h:#include "libavutil/x86/asm.h"
libavcodec/x86/dirac_dwt_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/fdct.c:#include "libavutil/x86/asm.h"
libavcodec/x86/h264_qpel.c:#include "libavutil/x86/asm.h"
libavcodec/x86/h264dsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/h26x/h2656dsp.h:#include "libavutil/x86/asm.h"
libavcodec/x86/hevcdsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/huffyuvdsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/lossless_videoencdsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/lpc_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/mathops.h:#include "libavutil/x86/asm.h"
libavcodec/x86/me_cmp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/mlpdsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/mpegaudiodsp.c:#include "libavutil/x86/asm.h"
libavcodec/x86/mpegvideo.c:#include "libavutil/x86/asm.h"
libavcodec/x86/mpegvideoenc.c:#include "libavutil/x86/asm.h"
libavcodec/x86/mpegvideoenc_qns_template.c:#include "libavutil/x86/asm.h"
libavcodec/x86/mpegvideoenc_template.c:#include "libavutil/x86/asm.h"
libavcodec/x86/snowdsp.c:#include "libavutil/x86/asm.h"
libavcodec/x86/utvideodsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/vc1dsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/vc1dsp_mmx.c:#include "libavutil/x86/asm.h"
libavcodec/x86/videodsp_init.c:#include "libavutil/x86/asm.h"
libavcodec/x86/vpx_arith.h:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_atadenoise_init.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_bwdif_init.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_eq_init.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_idet_init.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_maskedclamp_init.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_noise.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_pullup_init.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_spp.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_tinterlace_init.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_transpose_init.c:#include "libavutil/x86/asm.h"
libavfilter/x86/vf_w3fdif_init.c:#include "libavutil/x86/asm.h"
libavutil/x86/cpu.c:#include "libavutil/x86/asm.h"
libpostproc/postprocess_template.c:#include "libavutil/x86/asm.h"
libswscale/utils.c:#include "libavutil/x86/asm.h"
libswscale/x86/hscale_fast_bilinear_simd.c:#include "libavutil/x86/asm.h"
libswscale/x86/rgb2rgb.c:#include "libavutil/x86/asm.h"
libswscale/x86/swscale_template.c:#include "libavutil/x86/asm.h"
libswscale/x86/yuv2rgb.c:#include "libavutil/x86/asm.h"

lets stop here, but we could easily continue.

this is broken and its not libs using libavutil that should
"copy" random bits from libavutil.

Thats not how an API works.
We also dont copy random bits of libc and random bits of libssl
to use them.
And next we then have to maintain bugfixes, updates, and security
for these libssl bits we copied.

Maybe my words before where too polite but this is not how an API
is done. Especially there is no reason for this. Unless one tries
to create a unusable lib intentionally

If randomly copying these bits works and is recommanded to people
then they can also be cleanly exported with proper AV_ names

> 
> mem_internal.h is all defines, which can be trivially copied to an
> internally header in postproc.
> intmath.h seems to have a lot of inline functions, so it's a matter of also
> copying the header and removing the arch includes (or copying them too).
> x86/asm.h and ppc/util_altivec.h are defines and inline functions, so also
> possible to copy.

after 20 years of API design the solution is "copy our code"
and thats ok, FFmpeg is free software, what makes me a bit angry is that
for decades we have endless API redesigns and API improvments and then
when one once actually tries to use the API its
"copy this and copy that and lets move on"
instead of
"oops, let us fix this and export that properly"

and yes iam a little verbaly abrasive here. But i think its needed because
this mindset that the team as a whole seems to have is bad for FFmpeg.
I hope you agree that "copy it" isnt the API design to aim at

> ffversion.h is used postproc_ffversion, which should IMO be removed from the
> standalone library.

This includes the git version into the object. Which is as usefull as it is
in ffmpeg
Maybe it shouldnt have a "ffmpeg" string in it

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20241029/624cd229/attachment.sig>