| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/configure | warning | Failed to apply patch |
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.
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 [...]
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
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.
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
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
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".
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
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).
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.
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
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".
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.
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.
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
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
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
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
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?!).
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.
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.
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(-)
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.
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
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.
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.
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 [...]
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
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 [...]
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
--- 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; /**
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