[FFmpeg-devel] [PATCH] lavc: always build dnxhddata
James Almer
jamrial at gmail.com
Mon Jul 25 05:43:41 EEST 2016
On 7/24/2016 8:13 PM, Michael Niedermayer wrote:
> 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
A change in configure options can affect even public API right now.
See avutil's opencl and lzo modules.
We should probably change them to behave like avutil's pixelutils,
but in any case documenting that a change in configure options *may*
result in altered ABI is a good idea.
>
> 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 ...
I'm not sure if the theoretical benefits are worth the trouble and
extra complexity. Distros never really disable native modules and
only change configure options to add some new external library
dependency introduced with new ffmpeg releases.
Some applications do disable native modules often, especially on
Windows, but in those cases they always ship their own libav* set.
But in any case, this is something that should be dealt with next
time we bump major, if at all.
>
>>
>> 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
Are you ok with me reverting this commit and moving the function
to the header? The symbol will of course not be removed until next
major bump since it's already in a release.
Alternatively apply "[PATCH] lavf/movenc: add missing dependency
on dnxhddata" but IMO it's better to just move the thing to the
header.
>
>
> [...]
>
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list