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

[James Almer]

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.