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

[Ivan Kalvachev]

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.