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

[Ivan Kalvachev]

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.