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

James Almer jamrial at gmail.com
Thu Aug 24 18:11:00 EEST 2017

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.

More information about the ffmpeg-devel mailing list