[FFmpeg-devel] ABI break in 4.3

[James Almer]

On 7/5/2020 5:43 AM, Jan Engelhardt wrote:
> 
> On Sunday 2020-07-05 01:04, James Almer wrote:
>> On 7/4/2020 7:54 PM, Jan Engelhardt wrote:
>>> @@ -67,6 +67,10 @@ typedef struct AVDCT {
>>> +
>>> +    void (*get_pixels_unaligned)(int16_t *block /* align 16 */,
>>> +                       const uint8_t *pixels,
>>> +                       ptrdiff_t line_size);
>>>  } AVDCT;
>>
>> Neither of these are breaks as sizeof(AVCodecContext) and sizeof(AVDCT)
>> are not part of the ABI. Both structs are meant to be allocated using
>> avcodec_alloc_context3() and avcodec_dct_alloc() respectively, and not
>> stored on stack or allocated with av_malloc(sizeof()).
> 
> There is sometimes a disconnect between how an API is meant to be 
> used, and how 3rd party programs actually use it. In the spirit of 
> making it "hard(er) to misuse", perhaps the struct definition should be 
> removed from the public header and instead be written as
> 
> 	struct AVDCT;
> 	typedef struct AVDCT AVDCT;
> 
> so as to rule out av_malloc(sizeof(AVDCT)).
> 
> 
>> [[the header file says:
>> * You can use AVOptions (av_opt* / av_set/get*()) to access these fields from user
>> * applications.]]
> 
> A "can" can be read as "need not".

And you'd be correct. Direct access is allowed and offsets are
guaranteed within a soname version no matter the release.

> Perhaps that is what we are seeing with
> blender which seems to access av fields directly, and has no noticable 
> av_set_ calls.
> 
> writeffmpeg.c:  if ((of->oformat->flags & AVFMT_GLOBALHEADER)) {
> writeffmpeg.c:  if (of->oformat->flags & AVFMT_GLOBALHEADER) {
> writeffmpeg.c:  of->oformat = fmt;
> writeffmpeg.c:    of->packet_size = rd->ffcodecdata.mux_packet_size;
> writeffmpeg.c:  of->max_delay = (int)(0.7 * AV_TIME_BASE);
> writeffmpeg.c:  BLI_strncpy(of->filename, name, sizeof(of->filename));
> writeffmpeg.c:    if (avio_open(&of->pb, name, AVIO_FLAG_WRITE) < 0) {
> writeffmpeg.c:        &of->metadata, context->stamp_data, ffmpeg_add_metadata_callback, false);
> writeffmpeg.c:  if (of->pb) {
> writeffmpeg.c:    avio_close(of->pb);

As Thomas mentioned, this looks like valid usage of the API, so the
issue in Blender must be something else. All those AVFormatContext
fields can be accessed directly.

Unrelated to this, but they should stop using of->filename, for that
matter, and use of->url instead. The former has been deprecated for two
years and a half, and will be removed in an upcoming soname bump.

> 
> Or, summarized: A program may have been built with 4.3 but is combined
> with 4.2.3 at runtime, then this can happen:
> 
> 	a = avcodec_dct_alloc(); // allocates 896
> #ifdef HAVE_STRUCT_AVDCT_GET_PIXELS_UNALIGNED
> 	a->get_pixels_unaligned = ffunc; // boom accessing byte ~952
> #endif
> 
> If the API should not be used this way, it should not offer this way.

But it isn't offered that way. I don't know what part of avdct.h made
you think it could be used that way.

First, you're setting a pointer you're not meant to set yourself.
Nowhere in the doxy it says you can do that. It's not an user settable
callback. You're asked to initialize the struct with avcodec_dct_init().
Was it the best design choice? Probably not. Something like pixelutils.h
in libavutil may be a better approach, and AVDCT could be changed into
something like it.

And second, you're running a program built against 4.3 with a lavc 4.2.x
at runtime. That is not supported for obvious reasons and the source of
your "boom". You can (or should be able to) use 4.3 at runtime on a
program built against an ffmpeg release as old as 4.0, but no the
opposite way.

Your suggestion to change the way we generate the .ver files to outright
refuse to run in the above scenario is interesting, but may be quite a
lot of work considering the amount of public functions we export.