| Message ID | AS1PR01MB95649A21F5F9A45D40E6AC1D8F1F9@AS1PR01MB9564.eurprd01.prod.exchangelabs.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/7] avcodec/options: Fix AVClassCategory of decoders with .receive_frame | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
Quoting Andreas Rheinhardt (2022-03-31 00:49:57) > From: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > doc/APIchanges | 3 +++ > fftools/ffmpeg.c | 4 ++-- > fftools/ffprobe.c | 2 +- > libavcodec/avcodec.h | 3 +-- > libavcodec/decode.c | 9 ++++----- > libavcodec/version.h | 2 +- > tools/target_dec_fuzzer.c | 4 ++-- > 7 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index 1a9f0a303e..326a3c721c 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -14,6 +14,9 @@ libavutil: 2021-04-27 > > API changes, most recent first: > > +2022-03-30 - xxxxxxxxxx - lavc 59.26.100 - avcodec.h > + avcodec_decode_subtitle2() now accepts const AVPacket*. I vaguely recall C++ having a problem with such changes. Anybody remembers the details? Do we care?
> On Mar 31, 2022, at 7:44 PM, Anton Khirnov <anton@khirnov.net> wrote: > > Quoting Andreas Rheinhardt (2022-03-31 00:49:57) >> From: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> doc/APIchanges | 3 +++ >> fftools/ffmpeg.c | 4 ++-- >> fftools/ffprobe.c | 2 +- >> libavcodec/avcodec.h | 3 +-- >> libavcodec/decode.c | 9 ++++----- >> libavcodec/version.h | 2 +- >> tools/target_dec_fuzzer.c | 4 ++-- >> 7 files changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index 1a9f0a303e..326a3c721c 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -14,6 +14,9 @@ libavutil: 2021-04-27 >> >> API changes, most recent first: >> >> +2022-03-30 - xxxxxxxxxx - lavc 59.26.100 - avcodec.h >> + avcodec_decode_subtitle2() now accepts const AVPacket*. > > I vaguely recall C++ having a problem with such changes. Anybody > remembers the details? Do we care? const or not affects mangle and is part of the ABI for C++. Since FFmpeg is in C and must be included within 'extern “C”’ for C++, it has no effect. The patch LGTM. > > -- > Anton Khirnov > _______________________________________________ > 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".
Anton Khirnov: > Quoting Andreas Rheinhardt (2022-03-31 00:49:57) >> From: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> doc/APIchanges | 3 +++ >> fftools/ffmpeg.c | 4 ++-- >> fftools/ffprobe.c | 2 +- >> libavcodec/avcodec.h | 3 +-- >> libavcodec/decode.c | 9 ++++----- >> libavcodec/version.h | 2 +- >> tools/target_dec_fuzzer.c | 4 ++-- >> 7 files changed, 14 insertions(+), 13 deletions(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index 1a9f0a303e..326a3c721c 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -14,6 +14,9 @@ libavutil: 2021-04-27 >> >> API changes, most recent first: >> >> +2022-03-30 - xxxxxxxxxx - lavc 59.26.100 - avcodec.h >> + avcodec_decode_subtitle2() now accepts const AVPacket*. > > I vaguely recall C++ having a problem with such changes. Anybody > remembers the details? Do we care? > You did something like this in 0a0f19b577d54ff2e72cc9a0fe027952db83f332 without a major bump; IIRC there are more instances likes this. For C functions, the signature is not part of the exported symbol name (as zhilizhao already said) and the ABIs just presume that the caller and callee agree on the signature. Constifying this parameter does not break this, as "pointers to qualified or unqualified versions of compatible types shall have the same representation and alignment requirements" (C11 6.2.5/26). Ordinary API usage will be fine, because the conversion from AVPacket* to const AVPacket* is safe and therefore performed automatically; it requires no cast. API-wise there is only one thing that might cause problems: If one does not use avcodec_decode_subtitle2 directly, but via a function pointer like this: #include <libavcodec/avcodec.h> typedef int (*unconst)(AVCodecContext *, AVSubtitle *, int *, AVPacket *); void foo(void) { unconst funcptr = avcodec_decode_subtitle2; /* use funcptr */ } Here the initialization of avcodec_decode_subtitle2 is not fine; for C compilers will by default emit a warning, for C++ it is common to fail. But avcodec_decode_subtitle2 is the only function with this signature at all, so it makes no sense to use function pointers instead of using this function directly. I don't think that anyone does and therefore my answer to your last question is: I don't really care given that ordinary usages are fine. (Notice that the situation is different if one constified a level further down, e.g. by changing const AVCodec *av_codec_iterate(void **opaque) to const AVCodec *av_codec_iterate(const void **opaque) as there is no implicit conversion in this case. So the ordinary API usage would result in a warning or error. Might this be the reason for what you vaguely remember?) - Andreas
On Thu, Apr 14, 2022 at 9:33 PM Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote: > > Anton Khirnov: > > Quoting Andreas Rheinhardt (2022-03-31 00:49:57) > >> From: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > >> --- > >> doc/APIchanges | 3 +++ > >> fftools/ffmpeg.c | 4 ++-- > >> fftools/ffprobe.c | 2 +- > >> libavcodec/avcodec.h | 3 +-- > >> libavcodec/decode.c | 9 ++++----- > >> libavcodec/version.h | 2 +- > >> tools/target_dec_fuzzer.c | 4 ++-- > >> 7 files changed, 14 insertions(+), 13 deletions(-) > >> > >> diff --git a/doc/APIchanges b/doc/APIchanges > >> index 1a9f0a303e..326a3c721c 100644 > >> --- a/doc/APIchanges > >> +++ b/doc/APIchanges > >> @@ -14,6 +14,9 @@ libavutil: 2021-04-27 > >> > >> API changes, most recent first: > >> > >> +2022-03-30 - xxxxxxxxxx - lavc 59.26.100 - avcodec.h > >> + avcodec_decode_subtitle2() now accepts const AVPacket*. > > > > I vaguely recall C++ having a problem with such changes. Anybody > > remembers the details? Do we care? > > > > You did something like this in 0a0f19b577d54ff2e72cc9a0fe027952db83f332 > without a major bump; IIRC there are more instances likes this. > > For C functions, the signature is not part of the exported symbol name > (as zhilizhao already said) and the ABIs just presume that the caller > and callee agree on the signature. Constifying this parameter does not > break this, as "pointers to qualified or unqualified versions of > compatible types shall have the same representation and alignment > requirements" (C11 6.2.5/26). > > Ordinary API usage will be fine, because the conversion from AVPacket* > to const AVPacket* is safe and therefore performed automatically; it > requires no cast. API-wise there is only one thing that might cause > problems: If one does not use avcodec_decode_subtitle2 directly, but > via a function pointer like this: > > #include <libavcodec/avcodec.h> > > typedef int (*unconst)(AVCodecContext *, AVSubtitle *, int *, AVPacket *); > > void foo(void) > { > unconst funcptr = avcodec_decode_subtitle2; > > /* use funcptr */ > } > > Here the initialization of avcodec_decode_subtitle2 is not fine; for C > compilers will by default emit a warning, for C++ it is common to fail. > But avcodec_decode_subtitle2 is the only function with this signature > at all, so it makes no sense to use function pointers instead of using > this function directly. I don't think that anyone does and therefore my > answer to your last question is: I don't really care given that ordinary > usages are fine. > If one does manual dynamic loading (eg. for optional components), you would use a function pointer .. for all functions, not just this one. On the other hand, if you use C++ and you manually define the type for every function pointer (as we don't define function pointer types), its a rather fragile construct to begin with, especially as you could just infer the types automatically using decltype constructs, in which case there would be no issues as it just updates automatically. In theory such a change can break someones code, but.. its not great code to begin with and requires constant maintenance. - Hendrik
diff --git a/doc/APIchanges b/doc/APIchanges index 1a9f0a303e..326a3c721c 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -14,6 +14,9 @@ libavutil: 2021-04-27 API changes, most recent first: +2022-03-30 - xxxxxxxxxx - lavc 59.26.100 - avcodec.h + avcodec_decode_subtitle2() now accepts const AVPacket*. + 2022-03-16 - xxxxxxxxxx - all libraries - version_major.h Add lib<name>/version_major.h as new installed headers, which only contain the major version number (and corresponding API deprecation diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c index 6d62bdc7b0..979e33c9ab 100644 --- a/fftools/ffmpeg.c +++ b/fftools/ffmpeg.c @@ -2491,8 +2491,8 @@ fail: return err < 0 ? err : ret; } -static int transcode_subtitles(InputStream *ist, AVPacket *pkt, int *got_output, - int *decode_failed) +static int transcode_subtitles(InputStream *ist, const AVPacket *pkt, + int *got_output, int *decode_failed) { AVSubtitle subtitle; int free_sub = 1; diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c index 05c167eeb5..d2aacb6a0d 100644 --- a/fftools/ffprobe.c +++ b/fftools/ffprobe.c @@ -2629,7 +2629,7 @@ static void show_frame(WriterContext *w, AVFrame *frame, AVStream *stream, static av_always_inline int process_frame(WriterContext *w, InputFile *ifile, - AVFrame *frame, AVPacket *pkt, + AVFrame *frame, const AVPacket *pkt, int *packet_new) { AVFormatContext *fmt_ctx = ifile->fmt_ctx; diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 4dae23d06e..64804c2a50 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -2544,8 +2544,7 @@ enum AVChromaLocation avcodec_chroma_pos_to_enum(int xpos, int ypos); * @param[in] avpkt The input AVPacket containing the input buffer. */ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, - int *got_sub_ptr, - AVPacket *avpkt); + int *got_sub_ptr, const AVPacket *avpkt); /** * Supply raw packet data as input to a decoder. diff --git a/libavcodec/decode.c b/libavcodec/decode.c index e51a39e70a..8bb6e8c265 100644 --- a/libavcodec/decode.c +++ b/libavcodec/decode.c @@ -751,8 +751,8 @@ static void get_subtitle_defaults(AVSubtitle *sub) } #define UTF8_MAX_BYTES 4 /* 5 and 6 bytes sequences should not be used */ -static int recode_subtitle(AVCodecContext *avctx, AVPacket **outpkt, - AVPacket *inpkt, AVPacket *buf_pkt) +static int recode_subtitle(AVCodecContext *avctx, const AVPacket **outpkt, + const AVPacket *inpkt, AVPacket *buf_pkt) { #if CONFIG_ICONV iconv_t cd = (iconv_t)-1; @@ -832,8 +832,7 @@ static int utf8_check(const uint8_t *str) } int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, - int *got_sub_ptr, - AVPacket *avpkt) + int *got_sub_ptr, const AVPacket *avpkt) { int ret = 0; @@ -853,7 +852,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, if ((avctx->codec->capabilities & AV_CODEC_CAP_DELAY) || avpkt->size) { AVCodecInternal *avci = avctx->internal; - AVPacket *pkt; + const AVPacket *pkt; ret = recode_subtitle(avctx, &pkt, avpkt, avci->buffer_pkt); if (ret < 0) diff --git a/libavcodec/version.h b/libavcodec/version.h index a744e7469f..26ee41eb1f 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -29,7 +29,7 @@ #include "version_major.h" -#define LIBAVCODEC_VERSION_MINOR 25 +#define LIBAVCODEC_VERSION_MINOR 26 #define LIBAVCODEC_VERSION_MICRO 100 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c index 288aa63313..bb9fad613d 100644 --- a/tools/target_dec_fuzzer.c +++ b/tools/target_dec_fuzzer.c @@ -81,8 +81,8 @@ static const FFCodec *AVCodecInitialize(enum AVCodecID codec_id) return ffcodec(res); } -static int subtitle_handler(AVCodecContext *avctx, void *frame, - int *got_sub_ptr, AVPacket *avpkt) +static int subtitle_handler(AVCodecContext *avctx, AVFrame *unused, + int *got_sub_ptr, const AVPacket *avpkt) { AVSubtitle sub; int ret = avcodec_decode_subtitle2(avctx, &sub, got_sub_ptr, avpkt);