[FFmpeg-devel] [PATCH] all: avoid data imports across DLL boundaries

James Almer jamrial at gmail.com
Fri Aug 25 04:28:28 EEST 2017


On 8/24/2017 9:36 PM, Ivan Kalvachev wrote:
> On 8/25/17, James Almer <jamrial at gmail.com> wrote:
>> On 8/24/2017 8:26 PM, Ivan Kalvachev wrote:
>>> On 8/24/17, James Almer <jamrial at gmail.com> wrote:
>>>> On 8/24/2017 12:01 PM, wm4 wrote:
>>>>> On Thu, 24 Aug 2017 11:20:17 +0200
>>>>> Michael Niedermayer <michael at niedermayer.cc> wrote:
>>>>>
>>>>>> On Thu, Aug 24, 2017 at 10:52:55AM +0200, wm4 wrote:
>>>>>>> On Wed, 23 Aug 2017 19:23:12 -0300
>>>>>>> James Almer <jamrial at gmail.com> wrote:
>>>>>>>
>>>>>>>> On 8/21/2017 2:51 PM, wm4 wrote:
>>>>>>>>> From: Pedro Pombeiro <pedropombeiro at gmail.com>
>>>>>>>>>
>>>>>>>>> DLL data imports cause problems on Windows. They normally require
>>>>>>>>> each variable to be correctly marked with dllimport/dllexport
>>>>>>>>> declspecs. For MSVC, we define av_export to dllimport declspec. This
>>>>>>>>> is not entirely correct - depending on which sub-lib is built, they
>>>>>>>>> should be marked to dllexport instead. It happens to work with MSVC,
>>>>>>>>> because it supports exports incorrectly marked as imports. Trying to
>>>>>>>>> use this breaks on MinGW and results in linker errors.
>>>>>>>>>
>>>>>>>>> On MinGW, not using any import/export specifiers happens to work,
>>>>>>>>> because binutils and the MinGW runtime provide "pseudo relocations",
>>>>>>>>> which manually fix these imports at load time. This does not work
>>>>>>>>> with
>>>>>>>>> MinGW WinRT builds: the relocations cannot be performed because this
>>>>>>>>> would require writing to the code section, which is not allowed.
>>>>>>>>>
>>>>>>>>> Get rid of all these issues by not using data exports/imports. The
>>>>>>>>> public API is already free of them, but avpriv_ symbols make
>>>>>>>>> extensive
>>>>>>>>> use of them. Replace them all with getters.
>>>>>>>>
>>>>>>>> Should be good i think, but it can't be applied as is until the next
>>>>>>>> major bump as it breaks ABI (Tons of avpriv_ symbols are removed).
>>>>>>>
>>>>>>> Well, I call bullshit. We've never taken ABI compatibility between
>>>>>>> FFmpeg libs seriously, especially not if it was about avpriv
>>>>>>> functions.
>>>>>>>
>>>>>>
>>>>>> We did take ABI compatibility between FFmpeg libs serious.
>>>>>> And we must to be shiped by distributions that package the libs based
>>>>>> on our ABI versions.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> And guess what? You didn't either. Commit 7c9d2ad45f4e46ad2c3b2 is
>>>>>>> authored and committed by you, and changes an avpriv function in an
>>>>>>> incompatible way, without even minor version bumps.
>>>>>>
>>>>>> This commit did not break ABI
>>>>>> no release contains the removed symbol:
>>>>>>
>>>>>>  git grep avpriv_dca_parse_core_frame_header release/3.3
>>>>>>  git grep avpriv_dca_parse_core_frame_header release/3.2
>>>>>>  git grep avpriv_dca_parse_core_frame_header release/3.1
>>>>>>  git grep avpriv_dca_parse_core_frame_header release/3.0
>>>>>>  git grep avpriv_dca_parse_core_frame_header release/2.4
>>>>>>  git grep avpriv_dca_parse_core_frame_header release/2.8
>>>>>>
>>>>>> The symbol was added 9 days before it was removed, it was in no
>>>>>> release
>>>>>>
>>>>>> commit 7c9d2ad45f4e46ad2c3b2e93051efbe1e0d0529e
>>>>>> Author: James Almer <jamrial at gmail.com>
>>>>>> Date:   Wed Jul 19 01:53:22 2017 -0300
>>>>>>
>>>>>>     avcodec/dca: remove GetBitContext usage from
>>>>>> avpriv_dca_parse_core_frame_header()
>>>>>>
>>>>>> commit 2123ddb4251bf39bde8b38a1307a0f6154d260e6
>>>>>> Author: foo86 <foobaz86 at gmail.com>
>>>>>> Date:   Mon Jul 10 17:11:33 2017 +0300
>>>>>>
>>>>>>     avcodec: add avpriv_dca_parse_core_frame_header()
>>>>>> [...]
>>>>>
>>>>> We keep ABI stability even within git master, not just within releases.
>>>>> Otherwise, our lives would be so much easier whenever ABI problems come
>>>>> up.
>>>>
>>>> Yes, that's why the fix was committed two or three days after the symbol
>>>> was introduced, and not weeks after (Ignore the above dates, those are
>>>> authoring dates).
>>>>
>>>> It was in any case a change that could have waited until the major bump.
>>>> I hurried to replace it because i thought it was becoming the only
>>>> GetBitContext usage in a libavcodec exported function, thus effectively
>>>> making it part of the ABI, something that plays against any attempt to
>>>> replace it with the new bitstream reader.
>>>> Turns out that no, there were other avpriv_ symbols alredy using it, so
>>>> a major bump is nonetheless required to fix GetBitContext's ABI
>>>> dependency in other existing avpriv_ symbols.
>>>
>>> Digging the past won't help with the current issues.
>>>
>>> Can we keep the avpriv_* export of constants, (aka not remove them)
>>> but still apply the portion where avpriv_get_*() are used?
>>>
>>> This should keep the ABI and should not cause problems
>>> unless libraries from different builds are mixed.
>>>
>>> Probably a minor version bump for the getters
>>> and major for removing the exports.
>>>
>>> Since the symbols to constants would no longer be used
>>> by the ffmpeg libraries, the mingw linking problem should
>>> be solved.
>>>
>>> Best Regards.
>>
>> Martin sent a patchset that deals with wm4's issue in a different way,
>> so this patch might not be necessary after all.
> 
> I don't see it on the maillist.

It's of course on the libav devel mailing list. And the point is, since
there's an alternative solution for this issue with dll boundaries, wm4
as the author of this patch will probably (for the time being) keep it
on hold and until that solution is confirmed to work or not.

If it works, then he'll probably cherry pick it and send it here.


More information about the ffmpeg-devel mailing list