diff mbox series

[FFmpeg-devel] ABI break in 4.3

Message ID nycvar.YFH.7.77.849.2007050047290.32693@n3.vanv.qr
State Not Applicable
Headers show
Series [FFmpeg-devel] ABI break in 4.3
Related show

Checks

Context Check Description
andriy/default pending
andriy/configure warning Failed to apply patch

Commit Message

Jan Engelhardt July 4, 2020, 10:54 p.m. UTC
Greetings.


Between ffmpeg-4.2.3 and ffmpeg-4.3, struct AVCodecContext,
publicly exposed through /usr/include, has been changed thus:



The abidiff(1) utility confirms this situation such:

$ abidiff b42/usr/lib/debug/usr/lib64/libavcodec.so.58.54.100-4.2.3-1.1.x86_64.debug \
          b43/usr/lib/debug/usr/lib64/libavcodec.so.58.91.100-4.3-136.2.x86_64.debug

  [...] in pointed to type 'struct AVCodecContext' at avcodec.h:526:1:
                     type size changed from 8448 to 8576 (in bits)
  [...]
        underlying type 'struct AVDCT' at avdct.h:29:1 changed:
          type size changed from 896 to 960 (in bits)


These struct changes constitute an ABI break, and ABI breaks require SO version
bumps, but which was not done for 4.3. This is bad.

A user has summarily reported crashes through {not a proper bug reporting
medium} already.
https://build.opensuse.org/package/show/multimedia:libs/ffmpeg-4

Comments

James Almer July 4, 2020, 11:04 p.m. UTC | #1
On 7/4/2020 7:54 PM, Jan Engelhardt wrote:
> Greetings.
> 
> 
> Between ffmpeg-4.2.3 and ffmpeg-4.3, struct AVCodecContext,
> publicly exposed through /usr/include, has been changed thus:
> 
> --- avcodec.h 2020-06-11 11:45:16.000000000 +0200
> +++ avcodec.h 2020-07-01 03:45:24.000000000 +0200
> @@ -3370,6 +2334,24 @@ typedef struct AVCodecContext {
>       * - encoding: unused
>       */
>      int discard_damaged_percentage;
> +
> +    /**
> +     * The number of samples per frame to maximally accept.
> +     *
> +     * - decoding: set by user
> +     * - encoding: set by user
> +     */
> +    int64_t max_samples;
> +
> +    /**
> +     * Bit set of AV_CODEC_EXPORT_DATA_* flags, which affects the kind of
> +     * metadata exported in frame, packet, or coded stream side data by
> +     * decoders and encoders.
> +     *
> +     * - decoding: set by user
> +     * - encoding: set by user
> +     */
> +    int export_side_data;
>  } AVCodecContext;
>  
>  #if FF_API_CODEC_GET_SET
> 
> 
> Second, struct AVDCT, which is publicly exposed through /usr/include, has been
> changed:
> 
> --- avdct.h   2020-06-11 11:45:16.000000000 +0200
> +++ avdct.h   2020-07-01 03:45:24.000000000 +0200
> @@ -67,6 +67,10 @@ typedef struct AVDCT {
>                         ptrdiff_t line_size);
>  
>      int bits_per_sample;
> +
> +    void (*get_pixels_unaligned)(int16_t *block /* align 16 */,
> +                       const uint8_t *pixels,
> +                       ptrdiff_t line_size);
>  } AVDCT;
>  
>  /**
> 
> 
> The abidiff(1) utility confirms this situation such:
> 
> $ abidiff b42/usr/lib/debug/usr/lib64/libavcodec.so.58.54.100-4.2.3-1.1.x86_64.debug \
>           b43/usr/lib/debug/usr/lib64/libavcodec.so.58.91.100-4.3-136.2.x86_64.debug
> 
>   [...] in pointed to type 'struct AVCodecContext' at avcodec.h:526:1:
>                      type size changed from 8448 to 8576 (in bits)
>   [...]
>         underlying type 'struct AVDCT' at avdct.h:29:1 changed:
>           type size changed from 896 to 960 (in bits)
> 
> 
> These struct changes constitute an ABI break, and ABI breaks require SO version
> bumps, but which was not done for 4.3. This is bad.
> 
> A user has summarily reported crashes through {not a proper bug reporting
> medium} already.
> https://build.opensuse.org/package/show/multimedia:libs/ffmpeg-4

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()).

Fields can be added at the end of such structs without the need for a
soname bump and remain backwards compatible. A break would be adding a
field in the middle of such structs, moving the offset of other fields,
and that's not what happened here.
Michael Niedermayer July 4, 2020, 11:09 p.m. UTC | #2
On Sun, Jul 05, 2020 at 12:54:53AM +0200, Jan Engelhardt wrote:
> Greetings.
> 
> 
> Between ffmpeg-4.2.3 and ffmpeg-4.3, struct AVCodecContext,
> publicly exposed through /usr/include, has been changed thus:
> 
> --- avcodec.h 2020-06-11 11:45:16.000000000 +0200
> +++ avcodec.h 2020-07-01 03:45:24.000000000 +0200
> @@ -3370,6 +2334,24 @@ typedef struct AVCodecContext {
>       * - encoding: unused
>       */
>      int discard_damaged_percentage;
> +
> +    /**
> +     * The number of samples per frame to maximally accept.
> +     *
> +     * - decoding: set by user
> +     * - encoding: set by user
> +     */
> +    int64_t max_samples;
> +
> +    /**
> +     * Bit set of AV_CODEC_EXPORT_DATA_* flags, which affects the kind of
> +     * metadata exported in frame, packet, or coded stream side data by
> +     * decoders and encoders.
> +     *
> +     * - decoding: set by user
> +     * - encoding: set by user
> +     */
> +    int export_side_data;
>  } AVCodecContext;
>  
>  #if FF_API_CODEC_GET_SET
> 
> 
> Second, struct AVDCT, which is publicly exposed through /usr/include, has been
> changed:
> 
> --- avdct.h   2020-06-11 11:45:16.000000000 +0200
> +++ avdct.h   2020-07-01 03:45:24.000000000 +0200
> @@ -67,6 +67,10 @@ typedef struct AVDCT {
>                         ptrdiff_t line_size);
>  
>      int bits_per_sample;
> +
> +    void (*get_pixels_unaligned)(int16_t *block /* align 16 */,
> +                       const uint8_t *pixels,
> +                       ptrdiff_t line_size);
>  } AVDCT;
>  
>  /**
> 
> 
> The abidiff(1) utility confirms this situation such:
> 
> $ abidiff b42/usr/lib/debug/usr/lib64/libavcodec.so.58.54.100-4.2.3-1.1.x86_64.debug \
>           b43/usr/lib/debug/usr/lib64/libavcodec.so.58.91.100-4.3-136.2.x86_64.debug
> 
>   [...] in pointed to type 'struct AVCodecContext' at avcodec.h:526:1:
>                      type size changed from 8448 to 8576 (in bits)
>   [...]
>         underlying type 'struct AVDCT' at avdct.h:29:1 changed:
>           type size changed from 896 to 960 (in bits)
> 
> 
> These struct changes constitute an ABI break, and ABI breaks require SO version
> bumps, but which was not done for 4.3. This is bad.

Quoting the AVCodecContext documentation from FFmpeg 4.2.3

/**
 * main external API structure.
 * New fields can be added to the end with minor version bumps.
 * Removal, reordering and changes to existing fields require a major
 * version bump.
 * You can use AVOptions (av_opt* / av_set/get*()) to access these fields from user
 * applications.
 * The name string for AVOptions options matches the associated command line
 * parameter name and can be found in libavcodec/options_table.h
 * The AVOption/command line parameter names differ in some cases from the C
 * structure field names for historic reasons or brevity.
 * sizeof(AVCodecContext) must not be used outside libav*.
 */
typedef struct AVCodecContext {


> 
> A user has summarily reported crashes through {not a proper bug reporting
> medium} already.
> https://build.opensuse.org/package/show/multimedia:libs/ffmpeg-4

have you identified what exactly causes the crashes ?

Thanks

[...]
Andreas Rheinhardt July 4, 2020, 11:38 p.m. UTC | #3
Michael Niedermayer:
> On Sun, Jul 05, 2020 at 12:54:53AM +0200, Jan Engelhardt wrote:
>> Greetings.
>>
>>
>> Between ffmpeg-4.2.3 and ffmpeg-4.3, struct AVCodecContext,
>> publicly exposed through /usr/include, has been changed thus:
>>
>> --- avcodec.h 2020-06-11 11:45:16.000000000 +0200
>> +++ avcodec.h 2020-07-01 03:45:24.000000000 +0200
>> @@ -3370,6 +2334,24 @@ typedef struct AVCodecContext {
>>       * - encoding: unused
>>       */
>>      int discard_damaged_percentage;
>> +
>> +    /**
>> +     * The number of samples per frame to maximally accept.
>> +     *
>> +     * - decoding: set by user
>> +     * - encoding: set by user
>> +     */
>> +    int64_t max_samples;
>> +
>> +    /**
>> +     * Bit set of AV_CODEC_EXPORT_DATA_* flags, which affects the kind of
>> +     * metadata exported in frame, packet, or coded stream side data by
>> +     * decoders and encoders.
>> +     *
>> +     * - decoding: set by user
>> +     * - encoding: set by user
>> +     */
>> +    int export_side_data;
>>  } AVCodecContext;
>>  
>>  #if FF_API_CODEC_GET_SET
>>
>>
>> Second, struct AVDCT, which is publicly exposed through /usr/include, has been
>> changed:
>>
>> --- avdct.h   2020-06-11 11:45:16.000000000 +0200
>> +++ avdct.h   2020-07-01 03:45:24.000000000 +0200
>> @@ -67,6 +67,10 @@ typedef struct AVDCT {
>>                         ptrdiff_t line_size);
>>  
>>      int bits_per_sample;
>> +
>> +    void (*get_pixels_unaligned)(int16_t *block /* align 16 */,
>> +                       const uint8_t *pixels,
>> +                       ptrdiff_t line_size);
>>  } AVDCT;
>>  
>>  /**
>>
>>
>> The abidiff(1) utility confirms this situation such:
>>
>> $ abidiff b42/usr/lib/debug/usr/lib64/libavcodec.so.58.54.100-4.2.3-1.1.x86_64.debug \
>>           b43/usr/lib/debug/usr/lib64/libavcodec.so.58.91.100-4.3-136.2.x86_64.debug
>>
>>   [...] in pointed to type 'struct AVCodecContext' at avcodec.h:526:1:
>>                      type size changed from 8448 to 8576 (in bits)
>>   [...]
>>         underlying type 'struct AVDCT' at avdct.h:29:1 changed:
>>           type size changed from 896 to 960 (in bits)
>>
>>
>> These struct changes constitute an ABI break, and ABI breaks require SO version
>> bumps, but which was not done for 4.3. This is bad.
> 
> Quoting the AVCodecContext documentation from FFmpeg 4.2.3
> 
> /**
>  * main external API structure.
>  * New fields can be added to the end with minor version bumps.
>  * Removal, reordering and changes to existing fields require a major
>  * version bump.
>  * You can use AVOptions (av_opt* / av_set/get*()) to access these fields from user
>  * applications.
>  * The name string for AVOptions options matches the associated command line
>  * parameter name and can be found in libavcodec/options_table.h
>  * The AVOption/command line parameter names differ in some cases from the C
>  * structure field names for historic reasons or brevity.
>  * sizeof(AVCodecContext) must not be used outside libav*.
>  */
> typedef struct AVCodecContext {
> 
> 
>>
>> A user has summarily reported crashes through {not a proper bug reporting
>> medium} already.
>> https://build.opensuse.org/package/show/multimedia:libs/ffmpeg-4
> 
> have you identified what exactly causes the crashes ?
> 
This is the user report:

"Unfortunately, this version is binary incompatible to 4.2.3 in some
aspects. Chromium crashes when accessing some sites, e.g.
https://www.xometry.com/ and push the quote button. Blender cannot
export PNG files anymore."

This crash is due to Chromium using av_max_alloc in an undocumented way;
more exactly, the Chromium FFmpeg fork changes the allocation functions
so that if max_alloc_size is zero, the limit is completely disabled; and
of course it also sets the limit to zero. Up until 4.2, this worked with
a normal FFmpeg, because FFmpeg didn't max_alloc_size as is, but instead
max_alloc_size - 32. But since 731c77589841c02e014aa7f8285dcfc8b20f2ee5
this is no longer so. The relevant chromium bug is at
https://bugs.chromium.org/p/chromium/issues/detail?id=1095962

I don't know what the issue with Blender is.

- Andreas
Jan Engelhardt July 5, 2020, 8:43 a.m. UTC | #4
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". 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);

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.
Hendrik Leppkes July 5, 2020, 9:54 a.m. UTC | #5
On Sun, Jul 5, 2020 at 10:43 AM Jan Engelhardt <jengelh@inai.de> wrote:
>
> Or, summarized: A program may have been built with 4.3 but is combined
> with 4.2.3 at runtime, then this can happen:
>

Running against an older version then the build version is never
supported. ABI compatibility is only guaranteed forwards. An entirely
new function could be added, and not touch existing ABI, and this
scenario would still break.
A distribution should never allow this to happen.

- Hendrik
Tomas Härdin July 5, 2020, 10:06 a.m. UTC | #6
sön 2020-07-05 klockan 10:43 +0200 skrev Jan Engelhardt:
> On Sunday 2020-07-05 01:04, James Almer wrote:
> > [[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". 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);

This looks like perfectly legitimate code to me. But, if Blender is
using libav*'s APIs incorrectly somewhere then Blender should be fixed.

> 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

"Doctor it hurts when I do this!"

Downgrading to a .so file with a lower minor version number than the
program is built against can never be expected to work. Else we
couldn't add new functions without a major bump.

/Tomas
Jan Engelhardt July 5, 2020, 10:54 a.m. UTC | #7
On Sunday 2020-07-05 12:06, Tomas Härdin wrote:
>
>> 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
>
>"Doctor it hurts when I do this!"

The application of this saying is short-sighted. It confounds basic
exercise of features with overuse of said features. For lack of a
better analogy, moving a leg normally _ought_ not to hurt in healthy
humans. What is seen as "normal" is indeed situation-dependent, but
the only other way to look at it is that ffmpeg is a disabled entity
with special needs.


>Downgrading to a .so file with a lower minor version number than the
>program is built against can never be expected to work. Else we
>couldn't add new functions without a major bump.

Then you are doing it wrong. If one tries to run a contemporary
program on an older distribution, which certainly has an older glibc,
it refuses to run - which is much preferable to a crash at an
arbitrary point down the line.

It requires the use ELF symbol versions -- which ffmpeg fails to
do properly. Between 4.2.3 and 4.3,

	avpriv_mpeg4audio_get_config2@@LIBAVCODEC_58

which is wrong. It should have been

	avpriv_mpeg4audio_get_config2@@LIBAVCODEC_58.91

Then the runtime linker ld-linux.so would have caught the problem
at startup, because then, a program built with 4.3 would have a
minimum requirement on elfsymver "58.91", and *not just* "58".
Tomas Härdin July 5, 2020, 11:39 a.m. UTC | #8
sön 2020-07-05 klockan 12:54 +0200 skrev Jan Engelhardt:
> On Sunday 2020-07-05 12:06, Tomas Härdin wrote:
> > > 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
> > 
> > "Doctor it hurts when I do this!"
> 
> The application of this saying is short-sighted. It confounds basic
> exercise of features with overuse of said features. For lack of a
> better analogy, moving a leg normally _ought_ not to hurt in healthy
> humans. What is seen as "normal" is indeed situation-dependent, but
> the only other way to look at it is that ffmpeg is a disabled entity
> with special needs.

We can certainly change the API to make this sort of break harder, with
a major bump. But given what the documentation says about the current
API it is not a break. It might not be the most user-friendly API, but
it is right there in the headers. There's quite a bit of inertia
involved too.

This makes me wonder if there's a way to prevent AVCodecContext from
being allocated anywhere but the heap, without having it and similar
structs just be opaque pointers on the user side.

> > Downgrading to a .so file with a lower minor version number than
> > the
> > program is built against can never be expected to work. Else we
> > couldn't add new functions without a major bump.
> 
> Then you are doing it wrong. If one tries to run a contemporary
> program on an older distribution, which certainly has an older glibc,
> it refuses to run - which is much preferable to a crash at an
> arbitrary point down the line.
> 
> It requires the use ELF symbol versions -- which ffmpeg fails to
> do properly. Between 4.2.3 and 4.3,
> 
> 	avpriv_mpeg4audio_get_config2@@LIBAVCODEC_58
> 
> which is wrong. It should have been
> 
> 	avpriv_mpeg4audio_get_config2@@LIBAVCODEC_58.91
> 
> Then the runtime linker ld-linux.so would have caught the problem
> at startup, because then, a program built with 4.3 would have a
> minimum requirement on elfsymver "58.91", and *not just* "58".

This is a fair point. I didn't actually know the loader can do stuff
like this, sounds super handy. How hard would it be to get that going?

/Tomas
Jan Engelhardt July 5, 2020, 12:10 p.m. UTC | #9
On Sunday 2020-07-05 13:39, Tomas Härdin wrote:
>>> Downgrading to a .so file with a lower minor version number than
>>> the program is built against can never be expected to work. Else
>>> we couldn't add new functions without a major bump.
>> 
>> It requires the use ELF symbol versions -- which ffmpeg fails to
>> do properly.[...]
>
>This is a fair point. I didn't actually know the loader can do stuff
>like this, sounds super handy. How hard would it be to get that going?

Change libavcodec.v to

LIBAVCODEC_58 {
  global:
  av_foo;
  av_bar;
  av_whatever_else_there_is;...
  local:
  *;
};
LIBAVCODEC_58.91 {
  global:
  avpriv_mpeg4audio_get_config2;
} LIBAVCODEC_58;
LIBAVCODEC_58.123 { /* future example */
  global:
  avblahblah;
} LIBAVCODEC_58.91;

Repeat likewise for other .v files. The file is no longer a template. The
automatic substitution of "_MAJOR" by the build system needs to cease. Version
numbers in the file are supposed to be fixed (among the set of all .so files
that share a SONAME).
James Almer July 5, 2020, 1:59 p.m. UTC | #10
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.
Carl Eugen Hoyos July 5, 2020, 2:18 p.m. UTC | #11
Am So., 5. Juli 2020 um 01:38 Uhr schrieb Andreas Rheinhardt
<andreas.rheinhardt@gmail.com>:

> This crash is due to Chromium using av_max_alloc in an undocumented way;
> more exactly, the Chromium FFmpeg fork changes the allocation functions
> so that if max_alloc_size is zero, the limit is completely disabled; and
> of course it also sets the limit to zero. Up until 4.2, this worked with
> a normal FFmpeg, because FFmpeg didn't max_alloc_size as is, but instead
> max_alloc_size - 32. But since 731c77589841c02e014aa7f8285dcfc8b20f2ee5
> this is no longer so.

I think it is not immediately obvious that this is a (severe!) issue in
Chromium which basically disabled a security feature of FFmpeg
that was intentionally set to a very conservative (read: not soo
secure) value but was completely disabled by somebody who
misunderstood the feature (and failed to ask, I mention this
because this person's understanding would have implied that we
have no clue in C programming whatsoever).

At least one of the "downstream" fixes I saw in the last weeks simply
repeat this failure by again removing the security feature instead of
removing the wrong call from Chromium.

I am not sure if it really is our responsibility to explain to downstream
that valid multimedia files theoretically can allocate arbitrary amounts
of memory but that a responsible caller has to limit this amount for
nearly every theoretical use case, the more so for browser decoding.

Carl Eugen
Carl Eugen Hoyos July 5, 2020, 2:23 p.m. UTC | #12
Am So., 5. Juli 2020 um 10:43 Uhr schrieb Jan Engelhardt <jengelh@inai.de>:

> A program may have been built with 4.3 but is combined
> with 4.2.3 at runtime

While I can only speak for myself, this is not only not supported,
it is also something that FFmpeg will not support in the future.

Carl Eugen


, 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Timo Rothenpieler July 5, 2020, 2:43 p.m. UTC | #13
On 05.07.2020 14:10, Jan Engelhardt wrote:
> 
> On Sunday 2020-07-05 13:39, Tomas Härdin wrote:
>>>> Downgrading to a .so file with a lower minor version number than
>>>> the program is built against can never be expected to work. Else
>>>> we couldn't add new functions without a major bump.
>>>
>>> It requires the use ELF symbol versions -- which ffmpeg fails to
>>> do properly.[...]
>>
>> This is a fair point. I didn't actually know the loader can do stuff
>> like this, sounds super handy. How hard would it be to get that going?
> 
> Change libavcodec.v to
> 
> LIBAVCODEC_58 {
>    global:
>    av_foo;
>    av_bar;
>    av_whatever_else_there_is;...
>    local:
>    *;
> };
> LIBAVCODEC_58.91 {
>    global:
>    avpriv_mpeg4audio_get_config2;
> } LIBAVCODEC_58;
> LIBAVCODEC_58.123 { /* future example */
>    global:
>    avblahblah;
> } LIBAVCODEC_58.91;
> 
> Repeat likewise for other .v files. The file is no longer a template. The
> automatic substitution of "_MAJOR" by the build system needs to cease. Version
> numbers in the file are supposed to be fixed (among the set of all .so files
> that share a SONAME).
> 

Won't that break the entire ABI of anything currently linked, and thus 
would require a major bump?

Generally, this seems incredibly hard to maintain and will very likely 
cause confusion every time someone adds stuff in the future.
Timo Rothenpieler July 5, 2020, 2:46 p.m. UTC | #14
On 05.07.2020 16:18, Carl Eugen Hoyos wrote:
> Am So., 5. Juli 2020 um 01:38 Uhr schrieb Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com>:
> 
>> This crash is due to Chromium using av_max_alloc in an undocumented way;
>> more exactly, the Chromium FFmpeg fork changes the allocation functions
>> so that if max_alloc_size is zero, the limit is completely disabled; and
>> of course it also sets the limit to zero. Up until 4.2, this worked with
>> a normal FFmpeg, because FFmpeg didn't max_alloc_size as is, but instead
>> max_alloc_size - 32. But since 731c77589841c02e014aa7f8285dcfc8b20f2ee5
>> this is no longer so.
> 
> I think it is not immediately obvious that this is a (severe!) issue in
> Chromium which basically disabled a security feature of FFmpeg
> that was intentionally set to a very conservative (read: not soo
> secure) value but was completely disabled by somebody who
> misunderstood the feature (and failed to ask, I mention this
> because this person's understanding would have implied that we
> have no clue in C programming whatsoever).
> 
> At least one of the "downstream" fixes I saw in the last weeks simply
> repeat this failure by again removing the security feature instead of
> removing the wrong call from Chromium.
> 
> I am not sure if it really is our responsibility to explain to downstream
> that valid multimedia files theoretically can allocate arbitrary amounts
> of memory but that a responsible caller has to limit this amount for
> nearly every theoretical use case, the more so for browser decoding.
> 
> Carl Eugen

Chrome is using a custom allocator, that crashes the entire application 
on OOM rather than returning NULL.
So it's not a security issue in their case.
Marton Balint July 5, 2020, 2:53 p.m. UTC | #15
On Sun, 5 Jul 2020, Timo Rothenpieler wrote:

> On 05.07.2020 16:18, Carl Eugen Hoyos wrote:
>> Am So., 5. Juli 2020 um 01:38 Uhr schrieb Andreas Rheinhardt
>> <andreas.rheinhardt@gmail.com>:
>> 
>>> This crash is due to Chromium using av_max_alloc in an undocumented way;
>>> more exactly, the Chromium FFmpeg fork changes the allocation functions
>>> so that if max_alloc_size is zero, the limit is completely disabled; and
>>> of course it also sets the limit to zero. Up until 4.2, this worked with
>>> a normal FFmpeg, because FFmpeg didn't max_alloc_size as is, but instead
>>> max_alloc_size - 32. But since 731c77589841c02e014aa7f8285dcfc8b20f2ee5
>>> this is no longer so.
>> 
>> I think it is not immediately obvious that this is a (severe!) issue in
>> Chromium which basically disabled a security feature of FFmpeg
>> that was intentionally set to a very conservative (read: not soo
>> secure) value but was completely disabled by somebody who
>> misunderstood the feature (and failed to ask, I mention this
>> because this person's understanding would have implied that we
>> have no clue in C programming whatsoever).
>> 
>> At least one of the "downstream" fixes I saw in the last weeks simply
>> repeat this failure by again removing the security feature instead of
>> removing the wrong call from Chromium.
>> 
>> I am not sure if it really is our responsibility to explain to downstream
>> that valid multimedia files theoretically can allocate arbitrary amounts
>> of memory but that a responsible caller has to limit this amount for
>> nearly every theoretical use case, the more so for browser decoding.
>> 
>> Carl Eugen
>
> Chrome is using a custom allocator, that crashes the entire application 
> on OOM rather than returning NULL.
> So it's not a security issue in their case.

This is based on the assumption that a libav* function only returns NULL 
if there is a memory allocation error. That is simply not true. I am sure 
we can find a function in the codebase which returns NULL because of 
invalid arguments, or some other condition. IMHO they should not make 
this assumption.

Regards,
Marton
Tomas Härdin July 5, 2020, 3:08 p.m. UTC | #16
sön 2020-07-05 klockan 14:10 +0200 skrev Jan Engelhardt:
> On Sunday 2020-07-05 13:39, Tomas Härdin wrote:
> > > > Downgrading to a .so file with a lower minor version number than
> > > > the program is built against can never be expected to work. Else
> > > > we couldn't add new functions without a major bump.
> > > 
> > > It requires the use ELF symbol versions -- which ffmpeg fails to
> > > do properly.[...]
> > 
> > This is a fair point. I didn't actually know the loader can do stuff
> > like this, sounds super handy. How hard would it be to get that going?
> 
> Change libavcodec.v to
> 
> LIBAVCODEC_58 {
>   global:
>   av_foo;
>   av_bar;
>   av_whatever_else_there_is;...
>   local:
>   *;
> };
> LIBAVCODEC_58.91 {
>   global:
>   avpriv_mpeg4audio_get_config2;
> } LIBAVCODEC_58;
> LIBAVCODEC_58.123 { /* future example */
>   global:
>   avblahblah;
> } LIBAVCODEC_58.91;
> 
> Repeat likewise for other .v files. The file is no longer a template. The
> automatic substitution of "_MAJOR" by the build system needs to cease. Version
> numbers in the file are supposed to be fixed (among the set of all .so files
> that share a SONAME).

Interesting. This also makes what's changed between versions more
explicit. Can this be checked by machine as well? Like having a post-
receive hook that makes sure the .v files are consistent, or maybe have
FATE check it somehow.

This probably needs to be passed through the technical committee. But I
don't think it'll break the ABI like Timo suggests, if we bump minor at
the same time.

/Tomas
Carl Eugen Hoyos July 5, 2020, 3:43 p.m. UTC | #17
Am So., 5. Juli 2020 um 16:46 Uhr schrieb Timo Rothenpieler
<timo@rothenpieler.org>:
>
> On 05.07.2020 16:18, Carl Eugen Hoyos wrote:
> > Am So., 5. Juli 2020 um 01:38 Uhr schrieb Andreas Rheinhardt
> > <andreas.rheinhardt@gmail.com>:
> >
> >> This crash is due to Chromium using av_max_alloc in an undocumented way;
> >> more exactly, the Chromium FFmpeg fork changes the allocation functions
> >> so that if max_alloc_size is zero, the limit is completely disabled; and
> >> of course it also sets the limit to zero. Up until 4.2, this worked with
> >> a normal FFmpeg, because FFmpeg didn't max_alloc_size as is, but instead
> >> max_alloc_size - 32. But since 731c77589841c02e014aa7f8285dcfc8b20f2ee5
> >> this is no longer so.
> >
> > I think it is not immediately obvious that this is a (severe!) issue in
> > Chromium which basically disabled a security feature of FFmpeg
> > that was intentionally set to a very conservative (read: not soo
> > secure) value but was completely disabled by somebody who
> > misunderstood the feature (and failed to ask, I mention this
> > because this person's understanding would have implied that we
> > have no clue in C programming whatsoever).
> >
> > At least one of the "downstream" fixes I saw in the last weeks simply
> > repeat this failure by again removing the security feature instead of
> > removing the wrong call from Chromium.
> >
> > I am not sure if it really is our responsibility to explain to downstream
> > that valid multimedia files theoretically can allocate arbitrary amounts
> > of memory but that a responsible caller has to limit this amount for
> > nearly every theoretical use case, the more so for browser decoding.
> >
> > Carl Eugen
>
> Chrome is using a custom allocator, that crashes the entire application
> on OOM rather than returning NULL.
> So it's not a security issue in their case.

I understood the situation differently (because this is not about oom):
FFmpeg (by default) limits the possible allocation of a single call
to malloc() - independently of the used memory allocator. The default
limit is extremely high and it is difficult to image a useful file that
reaches the limit.
Somebody at Google thought that the "limitation" means that
libavutil returns only a block of "limit" if a block bigger than limit
was requested and therefore decided to remove the limit.
Apart from the fact that this would be a very severe issue in
FFmpeg that this Google employee decided not to report to
us, in addition the removal of the limit means that it is possible
to allocate nearly the complete memory from a media file within
the browser which clearly is a security issue in my opinion.
(I have no idea if their allocator limits the max possible allocation
anyway but even if it does, there should be no reason for
their change.)

Carl Eugen
Carl Eugen Hoyos July 5, 2020, 3:44 p.m. UTC | #18
Am So., 5. Juli 2020 um 16:44 Uhr schrieb Timo Rothenpieler
<timo@rothenpieler.org>:

> Generally, this seems incredibly hard to maintain and will very likely
> cause confusion every time someone adds stuff in the future.

True, and this is while this will not even reach the committee.

Carl Eugen
Jan Engelhardt July 5, 2020, 3:46 p.m. UTC | #19
On Sunday 2020-07-05 16:43, Timo Rothenpieler wrote:
>> LIBAVCODEC_58 {
>>    global:
>>    av_foo;
>>    av_bar;
>>    av_whatever_else_there_is;...
>>    local:
>>    *;
>> };
>> LIBAVCODEC_58.91 {
>>    global:
>>    avpriv_mpeg4audio_get_config2;
>> } LIBAVCODEC_58;
>> LIBAVCODEC_58.123 { /* future example */
>>    global:
>>    avblahblah;
>> } LIBAVCODEC_58.91;
>
> Won't that break the entire ABI of anything currently linked, and thus would
> require a major bump?

Since 4.3 was sort of a break over 4.2.3 already, the situation is that:

 * $nextrelease can be compatible with 4.2.3's idea of the ABI, in which case
   avpriv_mpeg4audio_get_config2 should be placed inside LIBAVCODEC_58.91, or

 * $nextrelease can be compatible with 4.3's idea of the ABI, in which case
   avpriv_mpeg4audio_get_config2 should be placed inside LIBAVCODEC_58.

Each of the two prior options is equally non-compelling. "58" is tarnished
already. What software generally does at this point — ffmpeg is not the first
project to have these issues — is to bump and start fresh.


> Generally, this seems incredibly hard to maintain and will very likely cause
> confusion every time someone adds stuff in the future.

How often do exported functions get added? Between 4.2.3 and 4.3,
that was _just one_, and that one was even an avpriv_* (which
probably shouldn't have been exported given its "priv" nature?!).
Timo Rothenpieler July 5, 2020, 4:07 p.m. UTC | #20
On 05.07.2020 17:46, Jan Engelhardt wrote:
> 
> On Sunday 2020-07-05 16:43, Timo Rothenpieler wrote:
>>> LIBAVCODEC_58 {
>>>     global:
>>>     av_foo;
>>>     av_bar;
>>>     av_whatever_else_there_is;...
>>>     local:
>>>     *;
>>> };
>>> LIBAVCODEC_58.91 {
>>>     global:
>>>     avpriv_mpeg4audio_get_config2;
>>> } LIBAVCODEC_58;
>>> LIBAVCODEC_58.123 { /* future example */
>>>     global:
>>>     avblahblah;
>>> } LIBAVCODEC_58.91;
>>
>> Won't that break the entire ABI of anything currently linked, and thus would
>> require a major bump?
> 
> Since 4.3 was sort of a break over 4.2.3 already, the situation is that:

It wasn't. Structs expanding at the end have occurred multiple times in 
the past already, and are documented as not having their size be part of 
the ABI/API.
Also, no functions were removed.

>   * $nextrelease can be compatible with 4.2.3's idea of the ABI, in which case
>     avpriv_mpeg4audio_get_config2 should be placed inside LIBAVCODEC_58.91, or
> 
>   * $nextrelease can be compatible with 4.3's idea of the ABI, in which case
>     avpriv_mpeg4audio_get_config2 should be placed inside LIBAVCODEC_58.
> 
> Each of the two prior options is equally non-compelling. "58" is tarnished
> already. What software generally does at this point — ffmpeg is not the first
> project to have these issues — is to bump and start fresh.
> 
> 
>> Generally, this seems incredibly hard to maintain and will very likely cause
>> confusion every time someone adds stuff in the future.
> 
> How often do exported functions get added? Between 4.2.3 and 4.3,
> that was _just one_, and that one was even an avpriv_* (which
> probably shouldn't have been exported given its "priv" nature?!).

Sometimes the libraries have functions they access from each other that 
are not meant to be public, but still need to be exported.
James Almer July 5, 2020, 4:18 p.m. UTC | #21
On 7/5/2020 12:46 PM, Jan Engelhardt wrote:
> 
> On Sunday 2020-07-05 16:43, Timo Rothenpieler wrote:
>>> LIBAVCODEC_58 {
>>>    global:
>>>    av_foo;
>>>    av_bar;
>>>    av_whatever_else_there_is;...
>>>    local:
>>>    *;
>>> };
>>> LIBAVCODEC_58.91 {
>>>    global:
>>>    avpriv_mpeg4audio_get_config2;
>>> } LIBAVCODEC_58;
>>> LIBAVCODEC_58.123 { /* future example */
>>>    global:
>>>    avblahblah;
>>> } LIBAVCODEC_58.91;
>>
>> Won't that break the entire ABI of anything currently linked, and thus would
>> require a major bump?
> 
> Since 4.3 was sort of a break over 4.2.3 already

No, it wasn't. No field offsets were changed, no public structs where
their size is part of the ABI were altered, and no public symbols were
removed. The situation is exactly the same as when 4.2 was released
after 4.1, and every other release before those.

If you can run 4.3 at runtime on a program that linked to 4.2, then it's
working as intended. Attempting the inverse is not and will never be
allowed or considered a valid scenario.

And i want to stress the fact that the reported Chromium breakage and
most assuredly also the Blender breakage are unrelated to what you're
arguing about in this thread.

> , the situation is that:
> 
>  * $nextrelease can be compatible with 4.2.3's idea of the ABI, in which case
>    avpriv_mpeg4audio_get_config2 should be placed inside LIBAVCODEC_58.91, or
> 
>  * $nextrelease can be compatible with 4.3's idea of the ABI, in which case
>    avpriv_mpeg4audio_get_config2 should be placed inside LIBAVCODEC_58.
> 
> Each of the two prior options is equally non-compelling. "58" is tarnished
> already.

As "tarnished" as every previous soname was, if we go by your definition
of breakage. I don't recall it ever being an issue for anyone until now.

This nonetheless is a good proposal and can be considered for the next
soname bump, if it can help prevent people using the wrong release at
runtime.

> What software generally does at this point — ffmpeg is not the first
> project to have these issues — is to bump and start fresh.
> 
> 
>> Generally, this seems incredibly hard to maintain and will very likely cause
>> confusion every time someone adds stuff in the future.
> 
> How often do exported functions get added? Between 4.2.3 and 4.3,
> that was _just one_, and that one was even an avpriv_* (which
> probably shouldn't have been exported given its "priv" nature?!).

avpriv_* are inter library communication functions. An unfortunate
consequence of having the project split across half a dozen libraries.
They need to be exported, but not exposed in the installed headers.
Jan Engelhardt July 5, 2020, 4:58 p.m. UTC | #22
On Sunday 2020-07-05 18:18, James Almer wrote:
>>>
>>> Won't that break the entire ABI of anything currently linked, and thus would
>>> require a major bump?
>> 
>> Since 4.3 was sort of a break over 4.2.3 already
>
>No, it wasn't.

Perhaps not on a high level. But for the ELF system, it was a break,
because you used the <SONAME, SYMVER> tuple, specifically <libavcodec.so.58,
LIBAVCODEC_58>, with two _different sets of contained symbols_.

Was it the reason for blender crash? I do not know that, nor do I know the
blender internals, but if it can be ruled out (at the very least, in the
future) that version mixup between $buildhost and $userhost can be the
cause of present or future crashes in blender or otherwise, I'll be damned if I
didn't try.

===

The following changes since commit c2d000ec27af1a5cd5341a67e941e0313879ab18:

  lavc/qsvenc_hevc: add qmax/qmin support for HEVC encoding (2020-07-05 23:43:45 +0800)

are available in the Git repository at:

  git://github.com/jengelh/ffmpeg master

for you to fetch changes up to 3ec24e4e548ecd6d4cc2f11a7d6717548abdadab:

  build: do proper ELF symbol versioning (2020-07-05 18:50:58 +0200)

----------------------------------------------------------------
Jan Engelhardt (1):
      build: do proper ELF symbol versioning

 libavcodec/libavcodec.v       | 254 +++++++++++++++-
 libavdevice/libavdevice.v     |  28 +-
 libavfilter/libavfilter.v     |  78 ++++-
 libavformat/libavformat.v     | 185 +++++++++++-
 libavresample/libavresample.v |  30 +-
 libavutil/libavutil.v         | 555 +++++++++++++++++++++++++++++++++-
 libswresample/libswresample.v |  33 +-
 libswscale/libswscale.v       |  44 ++-
 8 files changed, 1163 insertions(+), 44 deletions(-)
James Almer July 5, 2020, 5:21 p.m. UTC | #23
On 7/5/2020 1:58 PM, Jan Engelhardt wrote:
> 
> On Sunday 2020-07-05 18:18, James Almer wrote:
>>>>
>>>> Won't that break the entire ABI of anything currently linked, and thus would
>>>> require a major bump?
>>>
>>> Since 4.3 was sort of a break over 4.2.3 already
>>
>> No, it wasn't.
> 
> Perhaps not on a high level. But for the ELF system, it was a break,
> because you used the <SONAME, SYMVER> tuple, specifically <libavcodec.so.58,
> LIBAVCODEC_58>, with two _different sets of contained symbols_.
> 
> Was it the reason for blender crash? I do not know that, nor do I know the
> blender internals, but if it can be ruled out (at the very least, in the
> future) that version mixup between $buildhost and $userhost can be the
> cause of present or future crashes in blender or otherwise, I'll be damned if I
> didn't try.
> 
> ===
> 
> The following changes since commit c2d000ec27af1a5cd5341a67e941e0313879ab18:
> 
>   lavc/qsvenc_hevc: add qmax/qmin support for HEVC encoding (2020-07-05 23:43:45 +0800)
> 
> are available in the Git repository at:
> 
>   git://github.com/jengelh/ffmpeg master
> 
> for you to fetch changes up to 3ec24e4e548ecd6d4cc2f11a7d6717548abdadab:
> 
>   build: do proper ELF symbol versioning (2020-07-05 18:50:58 +0200)
> 
> ----------------------------------------------------------------
> Jan Engelhardt (1):
>       build: do proper ELF symbol versioning
> 
>  libavcodec/libavcodec.v       | 254 +++++++++++++++-
>  libavdevice/libavdevice.v     |  28 +-
>  libavfilter/libavfilter.v     |  78 ++++-
>  libavformat/libavformat.v     | 185 +++++++++++-
>  libavresample/libavresample.v |  30 +-
>  libavutil/libavutil.v         | 555 +++++++++++++++++++++++++++++++++-
>  libswresample/libswresample.v |  33 +-
>  libswscale/libswscale.v       |  44 ++-
>  8 files changed, 1163 insertions(+), 44 deletions(-)

The list of functions is incomplete. After a quick look, you're for
example missing av_map_videotoolbox_format_* in libavutil.

This is another issue that i don't know if you considered, and that's
the fact currently some symbols may not present depending on configure
time options, build environment, and target system. Videotoolbox is of
course not available for you unless you're targeting OSX.

This afaik can be solved by ensuring they are always present, and
returning ENOSYS/NULL as required if the actual implementation is not
compiled in.
We're doing as much in a few places, but clearly not enough care was
taken to ensure that's always the case.
Hendrik Leppkes July 5, 2020, 5:44 p.m. UTC | #24
On Sun, Jul 5, 2020 at 7:29 PM James Almer <jamrial@gmail.com> wrote:
>
> On 7/5/2020 1:58 PM, Jan Engelhardt wrote:
> >
> > On Sunday 2020-07-05 18:18, James Almer wrote:
> >>>>
> >>>> Won't that break the entire ABI of anything currently linked, and thus would
> >>>> require a major bump?
> >>>
> >>> Since 4.3 was sort of a break over 4.2.3 already
> >>
> >> No, it wasn't.
> >
> > Perhaps not on a high level. But for the ELF system, it was a break,
> > because you used the <SONAME, SYMVER> tuple, specifically <libavcodec.so.58,
> > LIBAVCODEC_58>, with two _different sets of contained symbols_.
> >
> > Was it the reason for blender crash? I do not know that, nor do I know the
> > blender internals, but if it can be ruled out (at the very least, in the
> > future) that version mixup between $buildhost and $userhost can be the
> > cause of present or future crashes in blender or otherwise, I'll be damned if I
> > didn't try.
> >
> > ===
> >
> > The following changes since commit c2d000ec27af1a5cd5341a67e941e0313879ab18:
> >
> >   lavc/qsvenc_hevc: add qmax/qmin support for HEVC encoding (2020-07-05 23:43:45 +0800)
> >
> > are available in the Git repository at:
> >
> >   git://github.com/jengelh/ffmpeg master
> >
> > for you to fetch changes up to 3ec24e4e548ecd6d4cc2f11a7d6717548abdadab:
> >
> >   build: do proper ELF symbol versioning (2020-07-05 18:50:58 +0200)
> >
> > ----------------------------------------------------------------
> > Jan Engelhardt (1):
> >       build: do proper ELF symbol versioning
> >
> >  libavcodec/libavcodec.v       | 254 +++++++++++++++-
> >  libavdevice/libavdevice.v     |  28 +-
> >  libavfilter/libavfilter.v     |  78 ++++-
> >  libavformat/libavformat.v     | 185 +++++++++++-
> >  libavresample/libavresample.v |  30 +-
> >  libavutil/libavutil.v         | 555 +++++++++++++++++++++++++++++++++-
> >  libswresample/libswresample.v |  33 +-
> >  libswscale/libswscale.v       |  44 ++-
> >  8 files changed, 1163 insertions(+), 44 deletions(-)
>
> The list of functions is incomplete. After a quick look, you're for
> example missing av_map_videotoolbox_format_* in libavutil.
>
> This is another issue that i don't know if you considered, and that's
> the fact currently some symbols may not present depending on configure
> time options, build environment, and target system. Videotoolbox is of
> course not available for you unless you're targeting OSX.
>
> This afaik can be solved by ensuring they are always present, and
> returning ENOSYS/NULL as required if the actual implementation is not
> compiled in.
> We're doing as much in a few places, but clearly not enough care was
> taken to ensure that's always the case.

I don't think platform specific functionality should get such a
treatment. A build on the same platform should always present the same
ABI, but nothing you can do will make videotoolbox function on Linux
or Windows, so the symbols have no business being included.
Headers for such functionality might even require OS-specific headers
to be included (eg. D3D on Windows, VT on OSX), which if of course not
going to work.

In this case, the ABI is not dependent on configure, but on the
underlying platform you are building on. Of course it should be
corrected if they are purely configure-controlled, and instead perhaps
moved to a platform model.

- Hendrik
Jan Engelhardt July 5, 2020, 6:59 p.m. UTC | #25
On Sunday 2020-07-05 19:21, James Almer wrote:
>>  8 files changed, 1163 insertions(+), 44 deletions(-)
>
>The list of functions is incomplete. After a quick look, you're for
>example missing av_map_videotoolbox_format_* in libavutil.[..]
>This is another issue that i don't know if you considered, and that's
>the fact currently some symbols may not present depending on configure
>time options, build environment, and target system. Videotoolbox is of
>course not available for you unless you're targeting OSX.

An alternative to building an initial list from `nm` is to
visually work through the function declarations in .h files.
This also requires knowing which of those get installed to /usr/include
and which constitute private headers.
I am sure you are more capable of knowing which ones are
which; I have only worked with ffmpeg source for a day.
James Almer July 5, 2020, 7:12 p.m. UTC | #26
On 7/5/2020 3:59 PM, Jan Engelhardt wrote:
> 
> On Sunday 2020-07-05 19:21, James Almer wrote:
>>>  8 files changed, 1163 insertions(+), 44 deletions(-)
>>
>> The list of functions is incomplete. After a quick look, you're for
>> example missing av_map_videotoolbox_format_* in libavutil.[..]
>> This is another issue that i don't know if you considered, and that's
>> the fact currently some symbols may not present depending on configure
>> time options, build environment, and target system. Videotoolbox is of
>> course not available for you unless you're targeting OSX.
> 
> An alternative to building an initial list from `nm` is to
> visually work through the function declarations in .h files.
> This also requires knowing which of those get installed to /usr/include
> and which constitute private headers.

The list of installed headers is $(HEADERS) in each of the project's
library specific Makefiles.

> I am sure you are more capable of knowing which ones are
> which; I have only worked with ffmpeg source for a day.

My intention was to state that ultimately it may not be a simple task to
get a fixed, non build time generated .ver file for this purpose. The
fact some symbols (at least currently) can be present or not may play
against it.
Michael Niedermayer July 5, 2020, 9:17 p.m. UTC | #27
On Sun, Jul 05, 2020 at 05:08:51PM +0200, Tomas Härdin wrote:
> sön 2020-07-05 klockan 14:10 +0200 skrev Jan Engelhardt:
> > On Sunday 2020-07-05 13:39, Tomas Härdin wrote:
> > > > > Downgrading to a .so file with a lower minor version number than
> > > > > the program is built against can never be expected to work. Else
> > > > > we couldn't add new functions without a major bump.
> > > > 
> > > > It requires the use ELF symbol versions -- which ffmpeg fails to
> > > > do properly.[...]
> > > 
> > > This is a fair point. I didn't actually know the loader can do stuff
> > > like this, sounds super handy. How hard would it be to get that going?
> > 
> > Change libavcodec.v to
> > 
> > LIBAVCODEC_58 {
> >   global:
> >   av_foo;
> >   av_bar;
> >   av_whatever_else_there_is;...
> >   local:
> >   *;
> > };
> > LIBAVCODEC_58.91 {
> >   global:
> >   avpriv_mpeg4audio_get_config2;
> > } LIBAVCODEC_58;
> > LIBAVCODEC_58.123 { /* future example */
> >   global:
> >   avblahblah;
> > } LIBAVCODEC_58.91;
> > 
> > Repeat likewise for other .v files. The file is no longer a template. The
> > automatic substitution of "_MAJOR" by the build system needs to cease. Version
> > numbers in the file are supposed to be fixed (among the set of all .so files
> > that share a SONAME).
> 
> Interesting. This also makes what's changed between versions more
> explicit. Can this be checked by machine as well? Like having a post-
> receive hook that makes sure the .v files are consistent, or maybe have
> FATE check it somehow.

its possible to do some sanity checks at the git level that would catch some
issues.
(for example you can detect changed version numbers and reject if that doesnt
 also update APIchanges
 and then parse the change to APIchanges and the change to the .v file and
 check if the same functions and versions are listed.)
 
At the fate level it should be easier to detect the actual functions in the
object files and compare this to the .v files

so its all doable in theory, but at that point the question arises, can we
maybe generate this automatically from the APIchanges file if we decide to
do that.
APIchanges should list all added functions and the version numbers.


> 
> This probably needs to be passed through the technical committee. But I
> don't think it'll break the ABI like Timo suggests, if we bump minor at
> the same time.

the technical committee is for disagreement arbitraration.
ATM i dont think we have any disagreement

[...]
Jan Engelhardt July 5, 2020, 9:48 p.m. UTC | #28
On Sunday 2020-07-05 23:17, Michael Niedermayer wrote:
>
>so its all doable in theory, but at that point the question arises, can we
>maybe generate this automatically from the APIchanges file if we decide to
>do that.
>APIchanges should list all added functions and the version numbers.

APIchanges would have to become more "machine-readable". Like, for example,
https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
Michael Niedermayer July 5, 2020, 11:28 p.m. UTC | #29
On Sun, Jul 05, 2020 at 11:48:14PM +0200, Jan Engelhardt wrote:
> 
> On Sunday 2020-07-05 23:17, Michael Niedermayer wrote:
> >
> >so its all doable in theory, but at that point the question arises, can we
> >maybe generate this automatically from the APIchanges file if we decide to
> >do that.
> >APIchanges should list all added functions and the version numbers.
> 
> APIchanges would have to become more "machine-readable".

yes


> Like, for example,
> https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist

machines are not that stupid. The existing format is fine, it just has to be
consistent
just consider
git grep '[Aa]dd [a-z0-9_]*()' doc/APIchanges
that probably catches most added functions, the previous hash from each
gives you the git version to match it to.
Sure it wont work for 100% but the 5% this fails for can be just edited
to be consistent so it works too and it keeps the format enjoyable for humans
to read and write

[...]
Tomas Härdin July 7, 2020, 7 a.m. UTC | #30
mån 2020-07-06 klockan 01:28 +0200 skrev Michael Niedermayer:
> On Sun, Jul 05, 2020 at 11:48:14PM +0200, Jan Engelhardt wrote:
> > On Sunday 2020-07-05 23:17, Michael Niedermayer wrote:
> > > so its all doable in theory, but at that point the question arises, can we
> > > maybe generate this automatically from the APIchanges file if we decide to
> > > do that.
> > > APIchanges should list all added functions and the version numbers.
> > 
> > APIchanges would have to become more "machine-readable".
> 
> yes
> 
> 
> > Like, for example,
> > https://github.com/bminor/glibc/blob/master/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
> 
> machines are not that stupid. The existing format is fine, it just has to be
> consistent
> just consider
> git grep '[Aa]dd [a-z0-9_]*()' doc/APIchanges
> that probably catches most added functions, the previous hash from each
> gives you the git version to match it to.
> Sure it wont work for 100% but the 5% this fails for can be just edited
> to be consistent so it works too and it keeps the format enjoyable for humans
> to read and write

We could go either way really. APIchanges could be partly generated
from the .v files. But I think something like what Michael suggests
here is easier simply because we edit APIchanges manually already, and
the .v files are basically dummies.

We don't need to parse everything that's in APIchanges, just make sure
future additions to it follow some machine-readable format. Unless
someone wants to spend the time backporting this to previous releases.

/Tomas
diff mbox series

Patch

--- avcodec.h 2020-06-11 11:45:16.000000000 +0200
+++ avcodec.h 2020-07-01 03:45:24.000000000 +0200
@@ -3370,6 +2334,24 @@  typedef struct AVCodecContext {
      * - encoding: unused
      */
     int discard_damaged_percentage;
+
+    /**
+     * The number of samples per frame to maximally accept.
+     *
+     * - decoding: set by user
+     * - encoding: set by user
+     */
+    int64_t max_samples;
+
+    /**
+     * Bit set of AV_CODEC_EXPORT_DATA_* flags, which affects the kind of
+     * metadata exported in frame, packet, or coded stream side data by
+     * decoders and encoders.
+     *
+     * - decoding: set by user
+     * - encoding: set by user
+     */
+    int export_side_data;
 } AVCodecContext;
 
 #if FF_API_CODEC_GET_SET


Second, struct AVDCT, which is publicly exposed through /usr/include, has been
changed:

--- avdct.h   2020-06-11 11:45:16.000000000 +0200
+++ avdct.h   2020-07-01 03:45:24.000000000 +0200
@@ -67,6 +67,10 @@  typedef struct AVDCT {
                        ptrdiff_t line_size);
 
     int bits_per_sample;
+
+    void (*get_pixels_unaligned)(int16_t *block /* align 16 */,
+                       const uint8_t *pixels,
+                       ptrdiff_t line_size);
 } AVDCT;
 
 /**