[FFmpeg-devel] [PATCH] lavc: always build dnxhddata

[Michael Niedermayer]

On Sun, Jul 24, 2016 at 06:52:50PM -0300, James Almer wrote:
> On 7/24/2016 6:14 PM, Michael Niedermayer wrote:
> > On Sun, Jul 24, 2016 at 06:04:18PM -0300, James Almer wrote:
> >> On 7/24/2016 5:59 PM, Matthieu Bouron wrote:
> >>> On Sun, Jul 24, 2016 at 10:53:51PM +0200, Michael Niedermayer wrote:
> >>>> On Sun, Jul 24, 2016 at 10:40:58PM +0200, Michael Niedermayer wrote:
> >>>>> On Sun, Jul 24, 2016 at 10:30:35PM +0200, Matthieu Bouron wrote:
> >>>>>> From: Matthieu Bouron <matthieu.bouron at stupeflix.com>
> >>>>>>
> >>>>>> lavc/movenc rely on avpriv_dnxhd_parse_header_prefix declared by
> >>>>>> dnxhddata.h since e47981dab7fb7c9499b959cb0125b7281301969a.
> >>>>>>
> >>>>>> Fixes a missing symbol error in lavc/movenc if the dnxhd encoder is not
> >>>>>> enabled.
> >>>>>> ---
> >>>>>>  libavcodec/Makefile | 5 +++--
> >>>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/libavcodec/Makefile b/libavcodec/Makefile
> >>>>>> index abef19e..4fc4b09 100644
> >>>>>> --- a/libavcodec/Makefile
> >>>>>> +++ b/libavcodec/Makefile
> >>>>>> @@ -32,6 +32,7 @@ OBJS = allcodecs.o                                                      \
> >>>>>>         codec_desc.o                                                     \
> >>>>>>         d3d11va.o                                                        \
> >>>>>>         dirac.o                                                          \
> >>>>>> +       dnxhddata.o                                                      \
> >>>>>>         dv_profile.o                                                     \
> >>>>>>         imgconvert.o                                                     \
> >>>>>>         jni.o                                                            \
> >>>>>> @@ -241,8 +242,8 @@ OBJS-$(CONFIG_DIRAC_DECODER)           += diracdec.o dirac.o diracdsp.o diractab
> >>>>>>                                            dirac_arith.o mpeg12data.o dirac_dwt.o \
> >>>>>>                                            dirac_vlc.o
> >>>>>>  OBJS-$(CONFIG_DFA_DECODER)             += dfa.o
> >>>>>> -OBJS-$(CONFIG_DNXHD_DECODER)           += dnxhddec.o dnxhddata.o
> >>>>>> -OBJS-$(CONFIG_DNXHD_ENCODER)           += dnxhdenc.o dnxhddata.o
> >>>>>> +OBJS-$(CONFIG_DNXHD_DECODER)           += dnxhddec.o
> >>>>>> +OBJS-$(CONFIG_DNXHD_ENCODER)           += dnxhdenc.o
> >>>>>
> >>>>> why dont you add dnxhddata.o to the demuxer needing it ?
> >>>>
> >>>> patch ok, i missed that this was a dependancy between the 2 libs
> >>>> thought it was within a lib
> >>>
> >>> Pushed. Thanks.
> >>>
> >>> Matthieu
> >>
> >> Please revert this change and instead add a 
> >> "OBJS-$(CONFIG_MOV_MUXER) += dnxhddata.o" line to the libavformat
> >> dependencies section.
> > 
> > doesnt this break the ABI of libavcodec ?
> 
> No more than it already is, assuming this can be considered a
> mistake in how avpriv functions are made available.

yes


> 
> dnxhddata.c contains like three avpriv functions, the oldest
> from 2013, and the file has never been built unconditionally.
> Any ffmpeg build without dnxhd de/encoder, mxf muxer or dnxhd
> demuxer from the past few years hasn't built it.
> There are other files, like mpeg4audio.c, that are also not
> built unconditionally and also have avpriv functions.

yes
but tradition doesnt make it correct
i think this hasnt hit anyone because its a problem for mainstream
distros that care about ABI and they dont disable alot of stuff
People disabling alot of stuff are probably building for a
specifici use case, some embeded system some debuging case whatever
they likely arent affected or concerned by the build libs all
changing ABI together


> 
> > either avpriv_dnxhd_parse_header_prefix is part of the ABI of
> > libavcodec or its not
> 

> If avpriv functions are meant to be part of the ABI and their
> presence not depend on configure time options, then that's
> something that will have to be changed across the codebase
> in general.

yes

They have to be part of the ABI though because only the ABI can be
used from outside and they are used from outside the lib

if you want an hypothetical example where the current code breaks
1. a distro maintainer rebuilding the libs with some things disabled
2. a user installing a new package that pulls in a new libavcodec
   (could be a video player) but not libavformat
3. everything using libavformat breaks due to missing symbbols

removing a function from the ABI requires a major version and soname
bump

we could document that changing the configure options affect the ABI
but that would be very unfriendly to distro maintainers

maybe we can just enable the conditional parts when shared libs
are enabled. This without much thinking seems a possible solution
that would if its enough be better than fully unconditional.
But if we go this way someone should think about if this is really
enough first ...


> 
> Looking at this function closely now, it could have been added
> to dnxhddata.h instead. No need for avpriv_ prefix and to
> forcefully build an object file full of data tables that it
> doesn't need.

agree, this would be best in this case


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"    - "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160725/0eadb962/attachment.sig>