Message ID | 20170316042052.29707-1-nfxjfg@googlemail.com |
---|---|
State | Accepted |
Headers | show |
On 16 March 2017 at 04:20, wm4 <nfxjfg@googlemail.com> wrote: > This patch deprecates anything that has to do with merging/splitting > side data. Automatic side data merging (and splitting), as well as all > API symbols involved in it, are removed completely. > > Two FF_API_ defines are dedicated to deprecating API symbols related to > this: FF_API_MERGE_SD_API removes av_packet_split/merge_side_data in > libavcodec, and FF_API_LAVF_KEEPSIDE_FLAG deprecates > AVFMT_FLAG_KEEP_SIDE_DATA in libavformat. > > Since it was claimed that changing the default from merging side data to > not doing it is an ABI change, there are two additional FF_API_ defines, > which stop using the side data merging/splitting by default (and remove > any code in avformat/avcodec doing this): FF_API_MERGE_SD in libavcodec, > and FF_API_LAVF_MERGE_SD in libavformat. > > It is very much intended that FF_API_MERGE_SD and FF_API_LAVF_MERGE_SD > are quickly defined to 0 in the next ABI bump, while the API symbols are > retained for a longer time for the sake of compatibility. > AVFMT_FLAG_KEEP_SIDE_DATA will (very much intentionally) do nothing for > most of the time it will still be defined. Keep in mind that no code > exists that actually tries to unset this flag for any reason, nor does > such code need to exist. Code setting this flag explicitly will work as > before. Thus it's ok for AVFMT_FLAG_KEEP_SIDE_DATA to do nothing once > side data merging has been removed from libavformat. > > In order to avoid that anyone in the future does this incorrectly, here > is a small guide how to update the internal code on bumps: > > - next ABI bump (probably soon): > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > - define FF_API_MERGE_SD to 0, and remove all code covered by it > - next API bump (typically two years in the future or so): > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > - define FF_API_MERGE_SD_API to 0, and remove all code covered by it > > Isn't a major bump and API bump? So why wait until major 59 when all things getting deprecated get removed at major 58? There's no reason to keep it around for longer when it isn't even used.
On 3/16/2017 1:20 AM, wm4 wrote: > This patch deprecates anything that has to do with merging/splitting > side data. Automatic side data merging (and splitting), as well as all > API symbols involved in it, are removed completely. > > Two FF_API_ defines are dedicated to deprecating API symbols related to > this: FF_API_MERGE_SD_API removes av_packet_split/merge_side_data in > libavcodec, and FF_API_LAVF_KEEPSIDE_FLAG deprecates > AVFMT_FLAG_KEEP_SIDE_DATA in libavformat. > > Since it was claimed that changing the default from merging side data to > not doing it is an ABI change, there are two additional FF_API_ defines, > which stop using the side data merging/splitting by default (and remove > any code in avformat/avcodec doing this): FF_API_MERGE_SD in libavcodec, > and FF_API_LAVF_MERGE_SD in libavformat. > > It is very much intended that FF_API_MERGE_SD and FF_API_LAVF_MERGE_SD > are quickly defined to 0 in the next ABI bump, while the API symbols are > retained for a longer time for the sake of compatibility. > AVFMT_FLAG_KEEP_SIDE_DATA will (very much intentionally) do nothing for > most of the time it will still be defined. Keep in mind that no code > exists that actually tries to unset this flag for any reason, nor does > such code need to exist. Code setting this flag explicitly will work as > before. Thus it's ok for AVFMT_FLAG_KEEP_SIDE_DATA to do nothing once > side data merging has been removed from libavformat. > > In order to avoid that anyone in the future does this incorrectly, here > is a small guide how to update the internal code on bumps: > > - next ABI bump (probably soon): > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > - define FF_API_MERGE_SD to 0, and remove all code covered by it > - next API bump (typically two years in the future or so): > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > - define FF_API_MERGE_SD_API to 0, and remove all code covered by it I assume you meant to write FF_API_LAVF_KEEPSIDE_FLAG in the API bump part. > > This forces anyone who actually wants packet side data to temporarily > use deprecated API to get it all. If you ask me, this is batshit fucked > up crazy, but it's how we roll. Making AVFMT_FLAG_KEEP_SIDE_DATA to be > set by default was rejected as an ABI change, so I'm going all the way > to get rid of this once and for all. > --- > doc/APIchanges | 5 +++++ > libavcodec/avcodec.h | 4 ++++ > libavcodec/avpacket.c | 4 ++++ > libavcodec/utils.c | 16 ++++++++++++++++ > libavcodec/version.h | 9 ++++++++- > libavformat/avformat.h | 4 +++- > libavformat/mux.c | 6 ++++++ > libavformat/options_table.h | 2 ++ > libavformat/utils.c | 2 ++ > libavformat/version.h | 8 +++++++- > 10 files changed, 57 insertions(+), 3 deletions(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index dc36a6bea7..acb67d38e4 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,11 @@ libavutil: 2015-08-28 > > API changes, most recent first: > > +2017-03-16 - xxxxxxx - lavf 57.66.105, lavc 57.83.101 - avformat.h, avcodec.h > + Deprecate AVFMT_FLAG_KEEP_SIDE_DATA. It will be ignored after the next major > + bump, and libavformat will behave as if it were always set. > + Deprecate av_packet_merge_side_data() and av_packet_split_side_data(). > + > 2017-02-10 - xxxxxxx - lavu 55.48.100 / 55.33.0 - spherical.h > Add AV_SPHERICAL_EQUIRECTANGULAR_TILE, av_spherical_tile_bounds(), > and projection-specific properties (bound_left, bound_top, bound_right, > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h > index e32f57983c..e279dd59ee 100644 > --- a/libavcodec/avcodec.h > +++ b/libavcodec/avcodec.h > @@ -4575,9 +4575,13 @@ int av_packet_shrink_side_data(AVPacket *pkt, enum AVPacketSideDataType type, > uint8_t* av_packet_get_side_data(AVPacket *pkt, enum AVPacketSideDataType type, > int *size); > > +#if FF_API_MERGE_SD_API > +attribute_deprecated > int av_packet_merge_side_data(AVPacket *pkt); > > +attribute_deprecated > int av_packet_split_side_data(AVPacket *pkt); > +#endif > > const char *av_packet_side_data_name(enum AVPacketSideDataType type); > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > index 60269aa63d..6af3bb68f1 100644 > --- a/libavcodec/avpacket.c > +++ b/libavcodec/avpacket.c > @@ -379,6 +379,8 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type) > return NULL; > } > > +#if FF_API_MERGE_SD_API > + > #define FF_MERGE_MARKER 0x8c4d9d108e25e9feULL > > int av_packet_merge_side_data(AVPacket *pkt){ > @@ -460,6 +462,8 @@ int av_packet_split_side_data(AVPacket *pkt){ > return 0; > } > > +#endif > + > uint8_t *av_packet_pack_dictionary(AVDictionary *dict, int *size) > { > AVDictionaryEntry *t = NULL; > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index 4d1b63222f..456e707c8f 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -2263,7 +2263,9 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi > > if ((avctx->codec->capabilities & AV_CODEC_CAP_DELAY) || avpkt->size || > (avctx->active_thread_type & FF_THREAD_FRAME)) { > +#if FF_API_MERGE_SD > int did_split = av_packet_split_side_data(&tmp); > +#endif You should do #if FF_API_MERGE_SD FF_DISABLE_DEPRECATION_WARNINGS int did_split = av_packet_split_side_data(&tmp); FF_ENABLE_DEPRECATION_WARNINGS #endif To remove the "deprecated" warning spam during compilation. Same with every other call to any of the deprecated functions. What about the ffmpeg.c calls to av_packet_split_side_data()? You should also wrap them with one of these new FF_API defines. > ret = apply_param_change(avctx, &tmp); > if (ret < 0) > goto fail; > @@ -2295,11 +2297,13 @@ fail: > emms_c(); //needed to avoid an emms_c() call before every return; > > avctx->internal->pkt = NULL; > +#if FF_API_MERGE_SD > if (did_split) { > av_packet_free_side_data(&tmp); > if(ret == tmp.size) > ret = avpkt->size; > } > +#endif > if (picture->flags & AV_FRAME_FLAG_DISCARD) { > *got_picture_ptr = 0; > } > @@ -2369,7 +2373,9 @@ int attribute_align_arg avcodec_decode_audio4(AVCodecContext *avctx, > uint8_t discard_reason = 0; > // copy to ensure we do not change avpkt > AVPacket tmp = *avpkt; > +#if FF_API_MERGE_SD > int did_split = av_packet_split_side_data(&tmp); > +#endif > ret = apply_param_change(avctx, &tmp); > if (ret < 0) > goto fail; > @@ -2481,11 +2487,13 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > fail: > avctx->internal->pkt = NULL; > +#if FF_API_MERGE_SD > if (did_split) { > av_packet_free_side_data(&tmp); > if(ret == tmp.size) > ret = avpkt->size; > } > +#endif > > if (ret >= 0 && *got_frame_ptr) { > if (!avctx->refcounted_frames) { > @@ -2682,6 +2690,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, > if ((avctx->codec->capabilities & AV_CODEC_CAP_DELAY) || avpkt->size) { > AVPacket pkt_recoded; > AVPacket tmp = *avpkt; > +#if FF_API_MERGE_SD > int did_split = av_packet_split_side_data(&tmp); > //apply_param_change(avctx, &tmp); > > @@ -2694,6 +2703,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, > memset(tmp.data + tmp.size, 0, > FFMIN(avpkt->size - tmp.size, AV_INPUT_BUFFER_PADDING_SIZE)); > } > +#endif > > pkt_recoded = tmp; > ret = recode_subtitle(avctx, &pkt_recoded, &tmp); > @@ -2753,11 +2763,13 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, > avctx->internal->pkt = NULL; > } > > +#if FF_API_MERGE_SD > if (did_split) { > av_packet_free_side_data(&tmp); > if(ret == tmp.size) > ret = avpkt->size; > } > +#endif > > if (*got_sub_ptr) > avctx->frame_number++; > @@ -2873,12 +2885,16 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke > if (avctx->codec->send_packet) { > if (avpkt) { > AVPacket tmp = *avpkt; > +#if FF_API_MERGE_SD > int did_split = av_packet_split_side_data(&tmp); > +#endif > ret = apply_param_change(avctx, &tmp); > if (ret >= 0) > ret = avctx->codec->send_packet(avctx, &tmp); > +#if FF_API_MERGE_SD > if (did_split) > av_packet_free_side_data(&tmp); > +#endif > return ret; > } else { > return avctx->codec->send_packet(avctx, NULL); > diff --git a/libavcodec/version.h b/libavcodec/version.h > index 3ed5a718d4..ec8651f086 100644 > --- a/libavcodec/version.h > +++ b/libavcodec/version.h > @@ -29,7 +29,7 @@ > > #define LIBAVCODEC_VERSION_MAJOR 57 > #define LIBAVCODEC_VERSION_MINOR 83 > -#define LIBAVCODEC_VERSION_MICRO 100 > +#define LIBAVCODEC_VERSION_MICRO 101 > > #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ > LIBAVCODEC_VERSION_MINOR, \ > @@ -157,6 +157,9 @@ > #ifndef FF_API_VAAPI_CONTEXT > #define FF_API_VAAPI_CONTEXT (LIBAVCODEC_VERSION_MAJOR < 58) > #endif > +#ifndef FF_API_MERGE_SD > +#define FF_API_MERGE_SD (LIBAVCODEC_VERSION_MAJOR < 58) > +#endif > #ifndef FF_API_AVCTX_TIMEBASE > #define FF_API_AVCTX_TIMEBASE (LIBAVCODEC_VERSION_MAJOR < 59) > #endif > @@ -229,5 +232,9 @@ > #ifndef FF_API_STRUCT_VAAPI_CONTEXT > #define FF_API_STRUCT_VAAPI_CONTEXT (LIBAVCODEC_VERSION_MAJOR < 59) > #endif > +#ifndef FF_API_MERGE_SD_API > +#define FF_API_MERGE_SD_API (LIBAVCODEC_VERSION_MAJOR < 59) > +#endif > + > > #endif /* AVCODEC_VERSION_H */ > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 4c1b18e002..4ab217dc17 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -1468,7 +1468,9 @@ typedef struct AVFormatContext { > #define AVFMT_FLAG_MP4A_LATM 0x8000 ///< Enable RTP MP4A-LATM payload > #define AVFMT_FLAG_SORT_DTS 0x10000 ///< try to interleave outputted packets by dts (using this flag can slow demuxing down) > #define AVFMT_FLAG_PRIV_OPT 0x20000 ///< Enable use of private options by delaying codec open (this could be made default once all code is converted) > -#define AVFMT_FLAG_KEEP_SIDE_DATA 0x40000 ///< Don't merge side data but keep it separate. > +#if FF_API_LAVF_KEEPSIDE_FLAG > +#define AVFMT_FLAG_KEEP_SIDE_DATA 0x40000 ///< Don't merge side data but keep it separate. Deprecated, will be the default. > +#endif > #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats > #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. > #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Wait for packet data before writing a header, and add bitstream filters as requested by the muxer > diff --git a/libavformat/mux.c b/libavformat/mux.c > index e500531789..7de5d03fec 100644 > --- a/libavformat/mux.c > +++ b/libavformat/mux.c > @@ -752,7 +752,9 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt) > } > } > > +#if FF_API_LAVF_MERGE_SD > did_split = av_packet_split_side_data(pkt); > +#endif > > if (!s->internal->header_written) { > ret = s->internal->write_header_ret ? s->internal->write_header_ret : write_header_internal(s); > @@ -777,8 +779,10 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt) > } > > fail: > +#if FF_API_LAVF_MERGE_SD > if (did_split) > av_packet_merge_side_data(pkt); > +#endif > > if (ret < 0) { > pkt->pts = pts_backup; > @@ -875,8 +879,10 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { > } > } > > +#if FF_API_LAVF_MERGE_SD > if (st->internal->nb_bsfcs) > av_packet_split_side_data(pkt); > +#endif > > for (i = 0; i < st->internal->nb_bsfcs; i++) { > AVBSFContext *ctx = st->internal->bsfcs[i]; > diff --git a/libavformat/options_table.h b/libavformat/options_table.h > index a537dda95e..0c1915d6d4 100644 > --- a/libavformat/options_table.h > +++ b/libavformat/options_table.h > @@ -48,7 +48,9 @@ static const AVOption avformat_options[] = { > {"igndts", "ignore dts", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_IGNDTS }, INT_MIN, INT_MAX, D, "fflags"}, > {"discardcorrupt", "discard corrupted frames", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_DISCARD_CORRUPT }, INT_MIN, INT_MAX, D, "fflags"}, > {"sortdts", "try to interleave outputted packets by dts", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_SORT_DTS }, INT_MIN, INT_MAX, D, "fflags"}, > +#if FF_API_LAVF_KEEPSIDE_FLAG > {"keepside", "don't merge side data", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_KEEP_SIDE_DATA }, INT_MIN, INT_MAX, D, "fflags"}, > +#endif > {"fastseek", "fast but inaccurate seeks", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_FAST_SEEK }, INT_MIN, INT_MAX, D, "fflags"}, > {"latm", "enable RTP MP4A-LATM payload", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_MP4A_LATM }, INT_MIN, INT_MAX, E, "fflags"}, > {"nobuffer", "reduce the latency introduced by optional buffering", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_NOBUFFER }, 0, INT_MAX, D, "fflags"}, > diff --git a/libavformat/utils.c b/libavformat/utils.c > index 37d7024465..a4b4466851 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -1675,8 +1675,10 @@ FF_ENABLE_DEPRECATION_WARNINGS > st->inject_global_side_data = 0; > } > > +#if FF_API_LAVF_MERGE_SD > if (!(s->flags & AVFMT_FLAG_KEEP_SIDE_DATA)) > av_packet_merge_side_data(pkt); > +#endif > } > > av_opt_get_dict_val(s, "metadata", AV_OPT_SEARCH_CHILDREN, &metadata); > diff --git a/libavformat/version.h b/libavformat/version.h > index dc689d45fb..bfc42e3f15 100644 > --- a/libavformat/version.h > +++ b/libavformat/version.h > @@ -33,7 +33,7 @@ > // Also please add any ticket numbers that you believe might be affected here > #define LIBAVFORMAT_VERSION_MAJOR 57 > #define LIBAVFORMAT_VERSION_MINOR 66 > -#define LIBAVFORMAT_VERSION_MICRO 104 > +#define LIBAVFORMAT_VERSION_MICRO 105 > > #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \ > LIBAVFORMAT_VERSION_MINOR, \ > @@ -88,6 +88,12 @@ > #ifndef FF_API_HLS_WRAP > #define FF_API_HLS_WRAP (LIBAVFORMAT_VERSION_MAJOR < 58) > #endif > +#ifndef FF_API_LAVF_MERGE_SD > +#define FF_API_LAVF_MERGE_SD (LIBAVFORMAT_VERSION_MAJOR < 58) > +#endif > +#ifndef FF_API_LAVF_KEEPSIDE_FLAG > +#define FF_API_LAVF_KEEPSIDE_FLAG (LIBAVFORMAT_VERSION_MAJOR < 58) This one should probably be set as "< 59", much like FF_API_MERGE_SD_API. > +#endif > > > #ifndef FF_API_R_FRAME_RATE >
On Thu, 16 Mar 2017 13:38:03 +0000 Rostislav Pehlivanov <atomnuker@gmail.com> wrote: > On 16 March 2017 at 04:20, wm4 <nfxjfg@googlemail.com> wrote: > > > This patch deprecates anything that has to do with merging/splitting > > side data. Automatic side data merging (and splitting), as well as all > > API symbols involved in it, are removed completely. > > > > Two FF_API_ defines are dedicated to deprecating API symbols related to > > this: FF_API_MERGE_SD_API removes av_packet_split/merge_side_data in > > libavcodec, and FF_API_LAVF_KEEPSIDE_FLAG deprecates > > AVFMT_FLAG_KEEP_SIDE_DATA in libavformat. > > > > Since it was claimed that changing the default from merging side data to > > not doing it is an ABI change, there are two additional FF_API_ defines, > > which stop using the side data merging/splitting by default (and remove > > any code in avformat/avcodec doing this): FF_API_MERGE_SD in libavcodec, > > and FF_API_LAVF_MERGE_SD in libavformat. > > > > It is very much intended that FF_API_MERGE_SD and FF_API_LAVF_MERGE_SD > > are quickly defined to 0 in the next ABI bump, while the API symbols are > > retained for a longer time for the sake of compatibility. > > AVFMT_FLAG_KEEP_SIDE_DATA will (very much intentionally) do nothing for > > most of the time it will still be defined. Keep in mind that no code > > exists that actually tries to unset this flag for any reason, nor does > > such code need to exist. Code setting this flag explicitly will work as > > before. Thus it's ok for AVFMT_FLAG_KEEP_SIDE_DATA to do nothing once > > side data merging has been removed from libavformat. > > > > In order to avoid that anyone in the future does this incorrectly, here > > is a small guide how to update the internal code on bumps: > > > > - next ABI bump (probably soon): > > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > > - define FF_API_MERGE_SD to 0, and remove all code covered by it > > - next API bump (typically two years in the future or so): > > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > > - define FF_API_MERGE_SD_API to 0, and remove all code covered by it > > > > > Isn't a major bump and API bump? So why wait until major 59 when all things > getting deprecated get removed at major 58? There's no reason to keep it > around for longer when it isn't even used. Apparently we're supposed to keep deprecated APIs around for 2 years. So to get this crap dead sooner, I've made separate deprecations for the actual mechanisms, which according to some (michaelni) is an ABI change.
On Thu, 16 Mar 2017 11:07:44 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/16/2017 1:20 AM, wm4 wrote: > > This patch deprecates anything that has to do with merging/splitting > > side data. Automatic side data merging (and splitting), as well as all > > API symbols involved in it, are removed completely. > > > > Two FF_API_ defines are dedicated to deprecating API symbols related to > > this: FF_API_MERGE_SD_API removes av_packet_split/merge_side_data in > > libavcodec, and FF_API_LAVF_KEEPSIDE_FLAG deprecates > > AVFMT_FLAG_KEEP_SIDE_DATA in libavformat. > > > > Since it was claimed that changing the default from merging side data to > > not doing it is an ABI change, there are two additional FF_API_ defines, > > which stop using the side data merging/splitting by default (and remove > > any code in avformat/avcodec doing this): FF_API_MERGE_SD in libavcodec, > > and FF_API_LAVF_MERGE_SD in libavformat. > > > > It is very much intended that FF_API_MERGE_SD and FF_API_LAVF_MERGE_SD > > are quickly defined to 0 in the next ABI bump, while the API symbols are > > retained for a longer time for the sake of compatibility. > > AVFMT_FLAG_KEEP_SIDE_DATA will (very much intentionally) do nothing for > > most of the time it will still be defined. Keep in mind that no code > > exists that actually tries to unset this flag for any reason, nor does > > such code need to exist. Code setting this flag explicitly will work as > > before. Thus it's ok for AVFMT_FLAG_KEEP_SIDE_DATA to do nothing once > > side data merging has been removed from libavformat. > > > > In order to avoid that anyone in the future does this incorrectly, here > > is a small guide how to update the internal code on bumps: > > > > - next ABI bump (probably soon): > > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > > - define FF_API_MERGE_SD to 0, and remove all code covered by it > > - next API bump (typically two years in the future or so): > > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > > - define FF_API_MERGE_SD_API to 0, and remove all code covered by it > > I assume you meant to write FF_API_LAVF_KEEPSIDE_FLAG in the API bump part. Yes. > > You should do > > #if FF_API_MERGE_SD > FF_DISABLE_DEPRECATION_WARNINGS > int did_split = av_packet_split_side_data(&tmp); > FF_ENABLE_DEPRECATION_WARNINGS > #endif > > To remove the "deprecated" warning spam during compilation. Same with every > other call to any of the deprecated functions. > > What about the ffmpeg.c calls to av_packet_split_side_data()? You should > also wrap them with one of these new FF_API defines. The deprecated API uses go soon (next ABI bump), so I'm not sure it's worth the trouble. > > +#ifndef FF_API_LAVF_MERGE_SD > > +#define FF_API_LAVF_MERGE_SD (LIBAVFORMAT_VERSION_MAJOR < 58) > > +#endif > > +#ifndef FF_API_LAVF_KEEPSIDE_FLAG > > +#define FF_API_LAVF_KEEPSIDE_FLAG (LIBAVFORMAT_VERSION_MAJOR < 58) > > This one should probably be set as "< 59", much like FF_API_MERGE_SD_API. Well, what we normally do is delaying the bump for certain FF_API symbols when bumping, so this is not all clear.
On Thu, Mar 16, 2017 at 05:20:51AM +0100, wm4 wrote: > This patch deprecates anything that has to do with merging/splitting > side data. Automatic side data merging (and splitting), as well as all > API symbols involved in it, are removed completely. > > Two FF_API_ defines are dedicated to deprecating API symbols related to > this: FF_API_MERGE_SD_API removes av_packet_split/merge_side_data in > libavcodec, and FF_API_LAVF_KEEPSIDE_FLAG deprecates > AVFMT_FLAG_KEEP_SIDE_DATA in libavformat. > > Since it was claimed that changing the default from merging side data to > not doing it is an ABI change, there are two additional FF_API_ defines, > which stop using the side data merging/splitting by default (and remove > any code in avformat/avcodec doing this): FF_API_MERGE_SD in libavcodec, > and FF_API_LAVF_MERGE_SD in libavformat. > > It is very much intended that FF_API_MERGE_SD and FF_API_LAVF_MERGE_SD > are quickly defined to 0 in the next ABI bump, while the API symbols are > retained for a longer time for the sake of compatibility. > AVFMT_FLAG_KEEP_SIDE_DATA will (very much intentionally) do nothing for > most of the time it will still be defined. Keep in mind that no code > exists that actually tries to unset this flag for any reason, nor does > such code need to exist. Code setting this flag explicitly will work as > before. Thus it's ok for AVFMT_FLAG_KEEP_SIDE_DATA to do nothing once > side data merging has been removed from libavformat. > > In order to avoid that anyone in the future does this incorrectly, here > is a small guide how to update the internal code on bumps: > > - next ABI bump (probably soon): > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > - define FF_API_MERGE_SD to 0, and remove all code covered by it > - next API bump (typically two years in the future or so): > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > - define FF_API_MERGE_SD_API to 0, and remove all code covered by it > > This forces anyone who actually wants packet side data to temporarily > use deprecated API to get it all. If you ask me, this is batshit fucked > up crazy, but it's how we roll. Making AVFMT_FLAG_KEEP_SIDE_DATA to be > set by default was rejected as an ABI change, so I'm going all the way > to get rid of this once and for all. > --- What is the advantage to deprecate the API ? Applications which depend on the current default would need the code provided by the API when the default changes [...]
Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit :
> Applications which depend on the current default would need
... to implement a correct structure to carry the data from their
demuxer to the lavc decoders.
Regards,
On Thu, 16 Mar 2017 21:01:04 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Mar 16, 2017 at 05:20:51AM +0100, wm4 wrote: > > This patch deprecates anything that has to do with merging/splitting > > side data. Automatic side data merging (and splitting), as well as all > > API symbols involved in it, are removed completely. > > > > Two FF_API_ defines are dedicated to deprecating API symbols related to > > this: FF_API_MERGE_SD_API removes av_packet_split/merge_side_data in > > libavcodec, and FF_API_LAVF_KEEPSIDE_FLAG deprecates > > AVFMT_FLAG_KEEP_SIDE_DATA in libavformat. > > > > Since it was claimed that changing the default from merging side data to > > not doing it is an ABI change, there are two additional FF_API_ defines, > > which stop using the side data merging/splitting by default (and remove > > any code in avformat/avcodec doing this): FF_API_MERGE_SD in libavcodec, > > and FF_API_LAVF_MERGE_SD in libavformat. > > > > It is very much intended that FF_API_MERGE_SD and FF_API_LAVF_MERGE_SD > > are quickly defined to 0 in the next ABI bump, while the API symbols are > > retained for a longer time for the sake of compatibility. > > AVFMT_FLAG_KEEP_SIDE_DATA will (very much intentionally) do nothing for > > most of the time it will still be defined. Keep in mind that no code > > exists that actually tries to unset this flag for any reason, nor does > > such code need to exist. Code setting this flag explicitly will work as > > before. Thus it's ok for AVFMT_FLAG_KEEP_SIDE_DATA to do nothing once > > side data merging has been removed from libavformat. > > > > In order to avoid that anyone in the future does this incorrectly, here > > is a small guide how to update the internal code on bumps: > > > > - next ABI bump (probably soon): > > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > > - define FF_API_MERGE_SD to 0, and remove all code covered by it > > - next API bump (typically two years in the future or so): > > - define FF_API_LAVF_MERGE_SD to 0, and remove all code covered by it > > - define FF_API_MERGE_SD_API to 0, and remove all code covered by it > > > > This forces anyone who actually wants packet side data to temporarily > > use deprecated API to get it all. If you ask me, this is batshit fucked > > up crazy, but it's how we roll. Making AVFMT_FLAG_KEEP_SIDE_DATA to be > > set by default was rejected as an ABI change, so I'm going all the way > > to get rid of this once and for all. > > --- > > What is the advantage to deprecate the API ? > > Applications which depend on the current default would need the code > provided by the API when the default changes The API deprecation is intentionally decoupled from the ABI bump, so API users which really want to use those functions get enough time to fix their code (I think the "standard" is 2 years). I'll push this on Saturday or Monday then, with the changes suggested by the others.
On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: > Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : > > Applications which depend on the current default would need > > ... to implement a correct structure to carry the data from their > demuxer to the lavc decoders. Is this possible for every application using every framework ? [...]
On Fri, 17 Mar 2017 00:32:04 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: > > Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : > > > Applications which depend on the current default would need > > > > ... to implement a correct structure to carry the data from their > > demuxer to the lavc decoders. > > Is this possible for every application using every framework ? They can invent their own side data merging mechanism if they don't. There are plenty of API mechanisms in FFmpeg that don't map well to other frameworks at all, and this is one of the easiest to sidestep.
Le septidi 27 ventôse, an CCXXV, Michael Niedermayer a écrit :
> Is this possible for every application using every framework ?
Probably not. Not our problem.
Regards,
On Fri, Mar 17, 2017 at 11:48:18AM +0100, Nicolas George wrote: > Le septidi 27 ventôse, an CCXXV, Michael Niedermayer a écrit : > > Is this possible for every application using every framework ? > > Probably not. Not our problem. If your users problems are not your problem than you have the wrong attitude to provide or design a service, API, lib, ... to users [...]
On Fri, 17 Mar 2017 13:32:40 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Mar 17, 2017 at 11:48:18AM +0100, Nicolas George wrote: > > Le septidi 27 ventôse, an CCXXV, Michael Niedermayer a écrit : > > > Is this possible for every application using every framework ? > > > > Probably not. Not our problem. > > If your users problems are not your problem than you have the wrong > attitude to provide or design a service, API, lib, ... to users This whole thing is idiotic. How are client applications dealing with transporting the AVCodecContext stream parameters over their layers? Why didn't you write code to serialize the AVStream's AVCodecContext into the first packet? Why do you hate your users (well those who need it) this much? Are you a monster? Your argumentation goes like this: 1. invent a mechanism that everyone WTFs at 2. claim that someone needs it 3. if someone removes it, pretend there is a problem because of imaginary people invented in 2 4. if you lose, accuse them of bad attitude etc. Please stop.
On Fri, Mar 17, 2017 at 12:32 AM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: >> Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : >> > Applications which depend on the current default would need >> >> ... to implement a correct structure to carry the data from their >> demuxer to the lavc decoders. > > Is this possible for every application using every framework ? > If you are using a multimedia framework, then putting non-media data into the media buffers is even worse then doing it internally in avformat->avcodec only. There could be all sorts of other components in play in such a framework which have no idea what this sidedata is, and as someone actually working with such a framework daily (DirectShow), it can and has broken playback when non-avcodec decoders were used and such data was in the packets. So in short, the "merge" solution has never solved anything for me using a media framework, luckily it was only used very sparingly (until recently when every packet out of the mpegts demuxer suddenly had pointless sidedata and broke all the things). If you use a multimedia framework that does in no way allow you to attach side data to the packets, then the person working with the framework should decide what to do about that, because only they know how their framework may work - merging it into the data has potential for way more breakage. - Hendrik
On Fri, Mar 17, 2017 at 01:45:40PM +0100, wm4 wrote: > On Fri, 17 Mar 2017 13:32:40 +0100 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Fri, Mar 17, 2017 at 11:48:18AM +0100, Nicolas George wrote: > > > Le septidi 27 ventôse, an CCXXV, Michael Niedermayer a écrit : > > > > Is this possible for every application using every framework ? > > > > > > Probably not. Not our problem. > > > > If your users problems are not your problem than you have the wrong > > attitude to provide or design a service, API, lib, ... to users > > This whole thing is idiotic. How are client applications dealing with > transporting the AVCodecContext stream parameters over their layers? > Why didn't you write code to serialize the AVStream's AVCodecContext > into the first packet? Why do you hate your users (well those who need > it) this much? Are you a monster? > > Your argumentation goes like this: > 1. invent a mechanism that everyone WTFs at > 2. claim that someone needs it > 3. if someone removes it, pretend there is a problem because of > imaginary people invented in 2 > 4. if you lose, accuse them of bad attitude etc. > > Please stop. please stop putting words in my mouth please look at the quoted text above, i replied to > > > > Is this possible for every application using every framework ? > > > > > > Probably not. Not our problem. with "If your users problems are not your problem than you have the wrong attitude to provide or design a service, API, lib, ... to users" The chain of arguments you list and argue against has not occured What has occured is that i asked if the suggested solution for users after the API change is possible for everyone. someone replied to this that its probably not (i dont know if thats so but i tend to agree here) and that its "Not our problem." which is something i felt is the wrong attitude so i stated my oppinon about that. I understand that this attitude is common, ive heared it from many people over the years both inside FFmpeg and as well other free software projects. [...]
On Fri, Mar 17, 2017 at 01:59:40PM +0100, Hendrik Leppkes wrote: > On Fri, Mar 17, 2017 at 12:32 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: > >> Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : > >> > Applications which depend on the current default would need > >> > >> ... to implement a correct structure to carry the data from their > >> demuxer to the lavc decoders. > > > > Is this possible for every application using every framework ? > > > > If you are using a multimedia framework, then putting non-media data > into the media buffers is even worse then doing it internally in > avformat->avcodec only. > There could be all sorts of other components in play in such a > framework which have no idea what this sidedata is, and as someone > actually working with such a framework daily (DirectShow), it can and > has broken playback when non-avcodec decoders were used and such data > was in the packets. > > So in short, the "merge" solution has never solved anything for me > using a media framework, luckily it was only used very sparingly > (until recently when every packet out of the mpegts demuxer suddenly > had pointless sidedata and broke all the things). > If you use a multimedia framework that does in no way allow you to > attach side data to the packets, then the person working with the > framework should decide what to do about that, because only they know > how their framework may work - merging it into the data has potential > for way more breakage. i agree i think the open question is if removing the possibility for user apps to call the merge / split functions is a good idea or not. Removing them makes it obviously impossible for a user app to use them Leaving them gives a user application the choice to use or not use them [...]
Le septidi 27 ventôse, an CCXXV, Michael Niedermayer a écrit : > If your users problems are not your problem than you have the wrong > attitude to provide or design a service, API, lib, ... to users I was on a crappy phone virtual keyboard, I could not elaborate comfortably. Many many projects nowadays use FFmpeg's API correctly, without this merged side data thingie. That proves it is possible. If some projects insist on using bad frameworks, they will get bad results. I do not think it is our purpose to accommodate bad frameworks. And, in fact, are there projects who use this thing? I mean current, alive projects. Unless I am mistaken, even the purpose of this API was not documented. Regards,
On Fri, Mar 17, 2017 at 09:37:17PM +0100, Nicolas George wrote: > Le septidi 27 ventôse, an CCXXV, Michael Niedermayer a écrit : > > If your users problems are not your problem than you have the wrong > > attitude to provide or design a service, API, lib, ... to users > > I was on a crappy phone virtual keyboard, I could not elaborate > comfortably. > > Many many projects nowadays use FFmpeg's API correctly, without this > merged side data thingie. That proves it is possible. If some projects > insist on using bad frameworks, they will get bad results. I do not > think it is our purpose to accommodate bad frameworks. People cant always choose what they use, and IMHO a frame work which doesnt allow passing side data cleanly may have simply different goals or be build around a differnt architecture than what you have in mind that doesnt make it bad IMHO nor should we IMHO disregard usability with bad frameworks [...] > > And, in fact, are there projects who use this thing? I mean current, > alive projects. Unless I am mistaken, even the purpose of this API was > not documented. I belive if we remove something we should be open to users problems as in writing in the release notes that function "this" was deprecated and is planned to be removed because of "that". If this removal affects you and its not possible for you to pass the side data cleanly, please contact ffmpeg-devel. We deprecated it on the assumptation that noone needs the functions. all above is my oppinion, not intended to ofend anyone [...]
On Fri, 17 Mar 2017 13:59:40 +0100 Hendrik Leppkes <h.leppkes@gmail.com> wrote: > On Fri, Mar 17, 2017 at 12:32 AM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: > >> Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : > >> > Applications which depend on the current default would need > >> > >> ... to implement a correct structure to carry the data from their > >> demuxer to the lavc decoders. > > > > Is this possible for every application using every framework ? > > > > If you are using a multimedia framework, then putting non-media data > into the media buffers is even worse then doing it internally in > avformat->avcodec only. > There could be all sorts of other components in play in such a > framework which have no idea what this sidedata is, and as someone > actually working with such a framework daily (DirectShow), it can and > has broken playback when non-avcodec decoders were used and such data > was in the packets. This was how I actually found out about this. I was feeding libavformat data to other non-libavcodec decoders, and there were mysterious failures about corrupt data in the packets. I wasn't very amused when finding out libavformat deliberately put crap there. Another time it was because libavformat apparently failed to add side data to an AVPacket that should have been there. Again, libavformat's "helpful" hack caused trouble. Not to mention that we sometimes go length to remove minor inefficiencies like avoiding copying packets (such as requiring padding for bitstream readers), but then copy and reallocate the packet _twice_ because of the side data merging thing. > So in short, the "merge" solution has never solved anything for me > using a media framework, luckily it was only used very sparingly > (until recently when every packet out of the mpegts demuxer suddenly > had pointless sidedata and broke all the things). > If you use a multimedia framework that does in no way allow you to > attach side data to the packets, then the person working with the > framework should decide what to do about that, because only they know > how their framework may work - merging it into the data has potential > for way more breakage. That gives us another nice argument: users of different media frameworks might generally not know about this hack, because side data is rare. But when a demuxer suddenly adds side data, their code would suddenly break for no apparent reason. It would probably take them quite some debugging to find out what happens here. This whole side data merging thing is so god damn ridiculous, and even more so because it's actually defended.
On 3/17/2017 11:38 PM, wm4 wrote: > On Fri, 17 Mar 2017 13:59:40 +0100 > Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >> On Fri, Mar 17, 2017 at 12:32 AM, Michael Niedermayer >> <michael@niedermayer.cc> wrote: >>> On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: >>>> Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : >>>>> Applications which depend on the current default would need >>>> >>>> ... to implement a correct structure to carry the data from their >>>> demuxer to the lavc decoders. >>> >>> Is this possible for every application using every framework ? >>> >> >> If you are using a multimedia framework, then putting non-media data >> into the media buffers is even worse then doing it internally in >> avformat->avcodec only. >> There could be all sorts of other components in play in such a >> framework which have no idea what this sidedata is, and as someone >> actually working with such a framework daily (DirectShow), it can and >> has broken playback when non-avcodec decoders were used and such data >> was in the packets. > > This was how I actually found out about this. I was feeding > libavformat data to other non-libavcodec decoders, and there were > mysterious failures about corrupt data in the packets. I wasn't very > amused when finding out libavformat deliberately put crap there. > > Another time it was because libavformat apparently failed to add side > data to an AVPacket that should have been there. Again, libavformat's > "helpful" hack caused trouble. > > Not to mention that we sometimes go length to remove minor > inefficiencies like avoiding copying packets (such as requiring padding > for bitstream readers), but then copy and reallocate the packet _twice_ > because of the side data merging thing. > >> So in short, the "merge" solution has never solved anything for me >> using a media framework, luckily it was only used very sparingly >> (until recently when every packet out of the mpegts demuxer suddenly >> had pointless sidedata and broke all the things). >> If you use a multimedia framework that does in no way allow you to >> attach side data to the packets, then the person working with the >> framework should decide what to do about that, because only they know >> how their framework may work - merging it into the data has potential >> for way more breakage. > > That gives us another nice argument: users of different media > frameworks might generally not know about this hack, because side data > is rare. But when a demuxer suddenly adds side data, their code would > suddenly break for no apparent reason. It would probably take them > quite some debugging to find out what happens here. > > This whole side data merging thing is so god damn ridiculous, and even > more so because it's actually defended. Lets start by applying the ABI changes so merging doesn't happen automatically anymore. The discussion of deprecating and removing the functionality itself or not shouldn't affect the above.
On Fri, 17 Mar 2017 14:23:21 +0100 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Mar 17, 2017 at 01:59:40PM +0100, Hendrik Leppkes wrote: > > On Fri, Mar 17, 2017 at 12:32 AM, Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: > > >> Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : > > >> > Applications which depend on the current default would need > > >> > > >> ... to implement a correct structure to carry the data from their > > >> demuxer to the lavc decoders. > > > > > > Is this possible for every application using every framework ? > > > > > > > If you are using a multimedia framework, then putting non-media data > > into the media buffers is even worse then doing it internally in > > avformat->avcodec only. > > There could be all sorts of other components in play in such a > > framework which have no idea what this sidedata is, and as someone > > actually working with such a framework daily (DirectShow), it can and > > has broken playback when non-avcodec decoders were used and such data > > was in the packets. > > > > So in short, the "merge" solution has never solved anything for me > > using a media framework, luckily it was only used very sparingly > > (until recently when every packet out of the mpegts demuxer suddenly > > had pointless sidedata and broke all the things). > > If you use a multimedia framework that does in no way allow you to > > attach side data to the packets, then the person working with the > > framework should decide what to do about that, because only they know > > how their framework may work - merging it into the data has potential > > for way more breakage. > > i agree > i think the open question is if removing the possibility for user > apps to call the merge / split functions is a good idea or not. > > Removing them makes it obviously impossible for a user app to use them > Leaving them gives a user application the choice to use or not use them My argument is that this mechanism is close to worthless. A media framework which really needs this will generally have no direct connection between demuxer and decoder. Instead, it must all go through the framework's abstraction layer. (Otherwise, you could obviously just pass the AVPacket directly.) If this is the case, the packet side data will be the least of their worries. Packet side data is relatively rare, and if it's missing, it usually causes small problems only, such as missing metadata. After all, side data is for _exceptions_ - normal data needed by the codec is usually part of the proper data. As mentioned in other replies, if the decoder side is actually not libavcodec (not unlikely if you embed libavformat into a foreign media framework), the merged side data will make things actively _worse_, because to other decoders it will look like undecodable garbage data was added to the packets. Anyway, the worse problem about interfacing lavf/lavc through a generic media framework are the codec parameters. Until recently, any field in AVCodecContext could have been required for correct decoding. How would you pass this AVCodecContext to the decoder? If you can't do this with AVPacket, you likely have the same problem with AVCodecContext. So why is there not a helper for this? As I mentioned in the other mail, there could have been a mechanism that serializes the AVCodecContext fields to the first packet, which could be used to lazily initialize the decoder correctly. (Like in the merged side data case, this would of course only work for lavf->layers->lavc.) (Nowadays, the situation is slightly better - only AVCodecParameters needs to be transported over the layers, which has fewer and simpler fields, and in particular no private fields.) All this makes me think that your argument about this isn't as strong as you think. Those who want to embed lavf/lavc into foreign frameworks will face a number of challenges. Sure, side data will be one of them. But as I argued here and in another reply, merged side data will also be a challenge, because it could upset foreign decoders. And the codec parameters issue is a worse issue anyway. Users of such frameworks will have to come up with their own tricks how to best embed FFmpeg into them. No, we can't provide them with a set of wild hacks. We can't know their needs, and they can do it better. Even if we provided such helpers, ways to cleanly serialize such fields to a separate byte array would probably be more helpful. As I argued before, the current way how side data is merged/unmerged opens to potential for attackers to "setup" side data in unexpected/unintended, and normally impossible ways, which opens a whole other can of worms.
On Fri, 17 Mar 2017 23:47:26 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/17/2017 11:38 PM, wm4 wrote: > > On Fri, 17 Mar 2017 13:59:40 +0100 > > Hendrik Leppkes <h.leppkes@gmail.com> wrote: > > > >> On Fri, Mar 17, 2017 at 12:32 AM, Michael Niedermayer > >> <michael@niedermayer.cc> wrote: > >>> On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: > >>>> Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : > >>>>> Applications which depend on the current default would need > >>>> > >>>> ... to implement a correct structure to carry the data from their > >>>> demuxer to the lavc decoders. > >>> > >>> Is this possible for every application using every framework ? > >>> > >> > >> If you are using a multimedia framework, then putting non-media data > >> into the media buffers is even worse then doing it internally in > >> avformat->avcodec only. > >> There could be all sorts of other components in play in such a > >> framework which have no idea what this sidedata is, and as someone > >> actually working with such a framework daily (DirectShow), it can and > >> has broken playback when non-avcodec decoders were used and such data > >> was in the packets. > > > > This was how I actually found out about this. I was feeding > > libavformat data to other non-libavcodec decoders, and there were > > mysterious failures about corrupt data in the packets. I wasn't very > > amused when finding out libavformat deliberately put crap there. > > > > Another time it was because libavformat apparently failed to add side > > data to an AVPacket that should have been there. Again, libavformat's > > "helpful" hack caused trouble. > > > > Not to mention that we sometimes go length to remove minor > > inefficiencies like avoiding copying packets (such as requiring padding > > for bitstream readers), but then copy and reallocate the packet _twice_ > > because of the side data merging thing. > > > >> So in short, the "merge" solution has never solved anything for me > >> using a media framework, luckily it was only used very sparingly > >> (until recently when every packet out of the mpegts demuxer suddenly > >> had pointless sidedata and broke all the things). > >> If you use a multimedia framework that does in no way allow you to > >> attach side data to the packets, then the person working with the > >> framework should decide what to do about that, because only they know > >> how their framework may work - merging it into the data has potential > >> for way more breakage. > > > > That gives us another nice argument: users of different media > > frameworks might generally not know about this hack, because side data > > is rare. But when a demuxer suddenly adds side data, their code would > > suddenly break for no apparent reason. It would probably take them > > quite some debugging to find out what happens here. > > > > This whole side data merging thing is so god damn ridiculous, and even > > more so because it's actually defended. > > Lets start by applying the ABI changes so merging doesn't happen > automatically anymore. > The discussion of deprecating and removing the functionality > itself or not shouldn't affect the above. I hope so - that's why I separated them in the first place. Deprecation of functions doesn't mean much anyway, since we apparently wait a whole 2 years (?) until removing them.
On 3/17/2017 11:56 PM, wm4 wrote: > On Fri, 17 Mar 2017 23:47:26 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 3/17/2017 11:38 PM, wm4 wrote: >>> On Fri, 17 Mar 2017 13:59:40 +0100 >>> Hendrik Leppkes <h.leppkes@gmail.com> wrote: >>> >>>> On Fri, Mar 17, 2017 at 12:32 AM, Michael Niedermayer >>>> <michael@niedermayer.cc> wrote: >>>>> On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: >>>>>> Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : >>>>>>> Applications which depend on the current default would need >>>>>> >>>>>> ... to implement a correct structure to carry the data from their >>>>>> demuxer to the lavc decoders. >>>>> >>>>> Is this possible for every application using every framework ? >>>>> >>>> >>>> If you are using a multimedia framework, then putting non-media data >>>> into the media buffers is even worse then doing it internally in >>>> avformat->avcodec only. >>>> There could be all sorts of other components in play in such a >>>> framework which have no idea what this sidedata is, and as someone >>>> actually working with such a framework daily (DirectShow), it can and >>>> has broken playback when non-avcodec decoders were used and such data >>>> was in the packets. >>> >>> This was how I actually found out about this. I was feeding >>> libavformat data to other non-libavcodec decoders, and there were >>> mysterious failures about corrupt data in the packets. I wasn't very >>> amused when finding out libavformat deliberately put crap there. >>> >>> Another time it was because libavformat apparently failed to add side >>> data to an AVPacket that should have been there. Again, libavformat's >>> "helpful" hack caused trouble. >>> >>> Not to mention that we sometimes go length to remove minor >>> inefficiencies like avoiding copying packets (such as requiring padding >>> for bitstream readers), but then copy and reallocate the packet _twice_ >>> because of the side data merging thing. >>> >>>> So in short, the "merge" solution has never solved anything for me >>>> using a media framework, luckily it was only used very sparingly >>>> (until recently when every packet out of the mpegts demuxer suddenly >>>> had pointless sidedata and broke all the things). >>>> If you use a multimedia framework that does in no way allow you to >>>> attach side data to the packets, then the person working with the >>>> framework should decide what to do about that, because only they know >>>> how their framework may work - merging it into the data has potential >>>> for way more breakage. >>> >>> That gives us another nice argument: users of different media >>> frameworks might generally not know about this hack, because side data >>> is rare. But when a demuxer suddenly adds side data, their code would >>> suddenly break for no apparent reason. It would probably take them >>> quite some debugging to find out what happens here. >>> >>> This whole side data merging thing is so god damn ridiculous, and even >>> more so because it's actually defended. >> >> Lets start by applying the ABI changes so merging doesn't happen >> automatically anymore. >> The discussion of deprecating and removing the functionality >> itself or not shouldn't affect the above. > > I hope so - that's why I separated them in the first place. I mean separate patches. The ABI changes can go in right now or as soon as we bump major version. That alone is enough to get most of the headaches out of the way. > > Deprecation of functions doesn't mean much anyway, since we apparently > wait a whole 2 years (?) until removing them. If we deprecate the functions it's something that will be reported and reflected in APIChanges. If we then decide to change that then the deprecation reversal also needs to be reported in APIChanges. So to avoid the noise, better to make sure we all agree on the deprecation from the beginning.
On Sat, 18 Mar 2017 00:15:53 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/17/2017 11:56 PM, wm4 wrote: > > On Fri, 17 Mar 2017 23:47:26 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> On 3/17/2017 11:38 PM, wm4 wrote: > >>> On Fri, 17 Mar 2017 13:59:40 +0100 > >>> Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >>> > >>>> On Fri, Mar 17, 2017 at 12:32 AM, Michael Niedermayer > >>>> <michael@niedermayer.cc> wrote: > >>>>> On Thu, Mar 16, 2017 at 09:20:55PM +0100, Nicolas George wrote: > >>>>>> Le sextidi 26 ventôse, an CCXXV, Michael Niedermayer a écrit : > >>>>>>> Applications which depend on the current default would need > >>>>>> > >>>>>> ... to implement a correct structure to carry the data from their > >>>>>> demuxer to the lavc decoders. > >>>>> > >>>>> Is this possible for every application using every framework ? > >>>>> > >>>> > >>>> If you are using a multimedia framework, then putting non-media data > >>>> into the media buffers is even worse then doing it internally in > >>>> avformat->avcodec only. > >>>> There could be all sorts of other components in play in such a > >>>> framework which have no idea what this sidedata is, and as someone > >>>> actually working with such a framework daily (DirectShow), it can and > >>>> has broken playback when non-avcodec decoders were used and such data > >>>> was in the packets. > >>> > >>> This was how I actually found out about this. I was feeding > >>> libavformat data to other non-libavcodec decoders, and there were > >>> mysterious failures about corrupt data in the packets. I wasn't very > >>> amused when finding out libavformat deliberately put crap there. > >>> > >>> Another time it was because libavformat apparently failed to add side > >>> data to an AVPacket that should have been there. Again, libavformat's > >>> "helpful" hack caused trouble. > >>> > >>> Not to mention that we sometimes go length to remove minor > >>> inefficiencies like avoiding copying packets (such as requiring padding > >>> for bitstream readers), but then copy and reallocate the packet _twice_ > >>> because of the side data merging thing. > >>> > >>>> So in short, the "merge" solution has never solved anything for me > >>>> using a media framework, luckily it was only used very sparingly > >>>> (until recently when every packet out of the mpegts demuxer suddenly > >>>> had pointless sidedata and broke all the things). > >>>> If you use a multimedia framework that does in no way allow you to > >>>> attach side data to the packets, then the person working with the > >>>> framework should decide what to do about that, because only they know > >>>> how their framework may work - merging it into the data has potential > >>>> for way more breakage. > >>> > >>> That gives us another nice argument: users of different media > >>> frameworks might generally not know about this hack, because side data > >>> is rare. But when a demuxer suddenly adds side data, their code would > >>> suddenly break for no apparent reason. It would probably take them > >>> quite some debugging to find out what happens here. > >>> > >>> This whole side data merging thing is so god damn ridiculous, and even > >>> more so because it's actually defended. > >> > >> Lets start by applying the ABI changes so merging doesn't happen > >> automatically anymore. > >> The discussion of deprecating and removing the functionality > >> itself or not shouldn't affect the above. > > > > I hope so - that's why I separated them in the first place. > > I mean separate patches. The ABI changes can go in right now or as > soon as we bump major version. That alone is enough to get most of > the headaches out of the way. > > > > > Deprecation of functions doesn't mean much anyway, since we apparently > > wait a whole 2 years (?) until removing them. > > If we deprecate the functions it's something that will be reported > and reflected in APIChanges. If we then decide to change that then > the deprecation reversal also needs to be reported in APIChanges. > So to avoid the noise, better to make sure we all agree on the > deprecation from the beginning. I'd rather push the patch as-is. Whether someone wants to un-deprecate those 2 functions is a different question. From what I can see, nobody has actually rejected these patches, not even Michael Niedermayer.
L'octidi 28 ventôse, an CCXXV, Michael Niedermayer a écrit : > People cant always choose what they use, and IMHO a frame work which > doesnt allow passing side data cleanly may have simply different goals > or be build around a differnt architecture than what you have in mind > that doesnt make it bad IMHO I think it does. > nor should we IMHO disregard usability with bad frameworks I think we can and should. More importantly, I think planning for unknown problems is a waste of time. Speculating on what bad frameworks people might possibly be using is also a waste of time. > I belive if we remove something we should be open to users problems > as in writing in the release notes that I am not against a paragraph in the release notes.
On Thu, 16 Mar 2017 05:20:51 +0100
wm4 <nfxjfg@googlemail.com> wrote:
> This patch deprecates anything that has to do with merging/splitting
Review comments applied and pushed both patches. This doesn't
change anything yet (other than adding deprecations) - bumping the
ABI/API correctly is up to people in the future.
2017-03-17 13:45 GMT+01:00 wm4 <nfxjfg@googlemail.com>: > Why do you hate your users (well those who need it) this much? Aren't you the one suggesting that we change API every year? You do realize that this is the only thing users don't want us to do? > Are you a monster? It is so good to know that you completely ignore any rule that you yourself agreed to. > Please stop. It would be so great if you would do so. Before, please consider fixing one or two of the regressions you currently add every second commit. Carl Eugen
diff --git a/doc/APIchanges b/doc/APIchanges index dc36a6bea7..acb67d38e4 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,11 @@ libavutil: 2015-08-28 API changes, most recent first: +2017-03-16 - xxxxxxx - lavf 57.66.105, lavc 57.83.101 - avformat.h, avcodec.h + Deprecate AVFMT_FLAG_KEEP_SIDE_DATA. It will be ignored after the next major + bump, and libavformat will behave as if it were always set. + Deprecate av_packet_merge_side_data() and av_packet_split_side_data(). + 2017-02-10 - xxxxxxx - lavu 55.48.100 / 55.33.0 - spherical.h Add AV_SPHERICAL_EQUIRECTANGULAR_TILE, av_spherical_tile_bounds(), and projection-specific properties (bound_left, bound_top, bound_right, diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index e32f57983c..e279dd59ee 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4575,9 +4575,13 @@ int av_packet_shrink_side_data(AVPacket *pkt, enum AVPacketSideDataType type, uint8_t* av_packet_get_side_data(AVPacket *pkt, enum AVPacketSideDataType type, int *size); +#if FF_API_MERGE_SD_API +attribute_deprecated int av_packet_merge_side_data(AVPacket *pkt); +attribute_deprecated int av_packet_split_side_data(AVPacket *pkt); +#endif const char *av_packet_side_data_name(enum AVPacketSideDataType type); diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index 60269aa63d..6af3bb68f1 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -379,6 +379,8 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type) return NULL; } +#if FF_API_MERGE_SD_API + #define FF_MERGE_MARKER 0x8c4d9d108e25e9feULL int av_packet_merge_side_data(AVPacket *pkt){ @@ -460,6 +462,8 @@ int av_packet_split_side_data(AVPacket *pkt){ return 0; } +#endif + uint8_t *av_packet_pack_dictionary(AVDictionary *dict, int *size) { AVDictionaryEntry *t = NULL; diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 4d1b63222f..456e707c8f 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -2263,7 +2263,9 @@ int attribute_align_arg avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi if ((avctx->codec->capabilities & AV_CODEC_CAP_DELAY) || avpkt->size || (avctx->active_thread_type & FF_THREAD_FRAME)) { +#if FF_API_MERGE_SD int did_split = av_packet_split_side_data(&tmp); +#endif ret = apply_param_change(avctx, &tmp); if (ret < 0) goto fail; @@ -2295,11 +2297,13 @@ fail: emms_c(); //needed to avoid an emms_c() call before every return; avctx->internal->pkt = NULL; +#if FF_API_MERGE_SD if (did_split) { av_packet_free_side_data(&tmp); if(ret == tmp.size) ret = avpkt->size; } +#endif if (picture->flags & AV_FRAME_FLAG_DISCARD) { *got_picture_ptr = 0; } @@ -2369,7 +2373,9 @@ int attribute_align_arg avcodec_decode_audio4(AVCodecContext *avctx, uint8_t discard_reason = 0; // copy to ensure we do not change avpkt AVPacket tmp = *avpkt; +#if FF_API_MERGE_SD int did_split = av_packet_split_side_data(&tmp); +#endif ret = apply_param_change(avctx, &tmp); if (ret < 0) goto fail; @@ -2481,11 +2487,13 @@ FF_ENABLE_DEPRECATION_WARNINGS } fail: avctx->internal->pkt = NULL; +#if FF_API_MERGE_SD if (did_split) { av_packet_free_side_data(&tmp); if(ret == tmp.size) ret = avpkt->size; } +#endif if (ret >= 0 && *got_frame_ptr) { if (!avctx->refcounted_frames) { @@ -2682,6 +2690,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, if ((avctx->codec->capabilities & AV_CODEC_CAP_DELAY) || avpkt->size) { AVPacket pkt_recoded; AVPacket tmp = *avpkt; +#if FF_API_MERGE_SD int did_split = av_packet_split_side_data(&tmp); //apply_param_change(avctx, &tmp); @@ -2694,6 +2703,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, memset(tmp.data + tmp.size, 0, FFMIN(avpkt->size - tmp.size, AV_INPUT_BUFFER_PADDING_SIZE)); } +#endif pkt_recoded = tmp; ret = recode_subtitle(avctx, &pkt_recoded, &tmp); @@ -2753,11 +2763,13 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, avctx->internal->pkt = NULL; } +#if FF_API_MERGE_SD if (did_split) { av_packet_free_side_data(&tmp); if(ret == tmp.size) ret = avpkt->size; } +#endif if (*got_sub_ptr) avctx->frame_number++; @@ -2873,12 +2885,16 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke if (avctx->codec->send_packet) { if (avpkt) { AVPacket tmp = *avpkt; +#if FF_API_MERGE_SD int did_split = av_packet_split_side_data(&tmp); +#endif ret = apply_param_change(avctx, &tmp); if (ret >= 0) ret = avctx->codec->send_packet(avctx, &tmp); +#if FF_API_MERGE_SD if (did_split) av_packet_free_side_data(&tmp); +#endif return ret; } else { return avctx->codec->send_packet(avctx, NULL); diff --git a/libavcodec/version.h b/libavcodec/version.h index 3ed5a718d4..ec8651f086 100644 --- a/libavcodec/version.h +++ b/libavcodec/version.h @@ -29,7 +29,7 @@ #define LIBAVCODEC_VERSION_MAJOR 57 #define LIBAVCODEC_VERSION_MINOR 83 -#define LIBAVCODEC_VERSION_MICRO 100 +#define LIBAVCODEC_VERSION_MICRO 101 #define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \ LIBAVCODEC_VERSION_MINOR, \ @@ -157,6 +157,9 @@ #ifndef FF_API_VAAPI_CONTEXT #define FF_API_VAAPI_CONTEXT (LIBAVCODEC_VERSION_MAJOR < 58) #endif +#ifndef FF_API_MERGE_SD +#define FF_API_MERGE_SD (LIBAVCODEC_VERSION_MAJOR < 58) +#endif #ifndef FF_API_AVCTX_TIMEBASE #define FF_API_AVCTX_TIMEBASE (LIBAVCODEC_VERSION_MAJOR < 59) #endif @@ -229,5 +232,9 @@ #ifndef FF_API_STRUCT_VAAPI_CONTEXT #define FF_API_STRUCT_VAAPI_CONTEXT (LIBAVCODEC_VERSION_MAJOR < 59) #endif +#ifndef FF_API_MERGE_SD_API +#define FF_API_MERGE_SD_API (LIBAVCODEC_VERSION_MAJOR < 59) +#endif + #endif /* AVCODEC_VERSION_H */ diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 4c1b18e002..4ab217dc17 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -1468,7 +1468,9 @@ typedef struct AVFormatContext { #define AVFMT_FLAG_MP4A_LATM 0x8000 ///< Enable RTP MP4A-LATM payload #define AVFMT_FLAG_SORT_DTS 0x10000 ///< try to interleave outputted packets by dts (using this flag can slow demuxing down) #define AVFMT_FLAG_PRIV_OPT 0x20000 ///< Enable use of private options by delaying codec open (this could be made default once all code is converted) -#define AVFMT_FLAG_KEEP_SIDE_DATA 0x40000 ///< Don't merge side data but keep it separate. +#if FF_API_LAVF_KEEPSIDE_FLAG +#define AVFMT_FLAG_KEEP_SIDE_DATA 0x40000 ///< Don't merge side data but keep it separate. Deprecated, will be the default. +#endif #define AVFMT_FLAG_FAST_SEEK 0x80000 ///< Enable fast, but inaccurate seeks for some formats #define AVFMT_FLAG_SHORTEST 0x100000 ///< Stop muxing when the shortest stream stops. #define AVFMT_FLAG_AUTO_BSF 0x200000 ///< Wait for packet data before writing a header, and add bitstream filters as requested by the muxer diff --git a/libavformat/mux.c b/libavformat/mux.c index e500531789..7de5d03fec 100644 --- a/libavformat/mux.c +++ b/libavformat/mux.c @@ -752,7 +752,9 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt) } } +#if FF_API_LAVF_MERGE_SD did_split = av_packet_split_side_data(pkt); +#endif if (!s->internal->header_written) { ret = s->internal->write_header_ret ? s->internal->write_header_ret : write_header_internal(s); @@ -777,8 +779,10 @@ static int write_packet(AVFormatContext *s, AVPacket *pkt) } fail: +#if FF_API_LAVF_MERGE_SD if (did_split) av_packet_merge_side_data(pkt); +#endif if (ret < 0) { pkt->pts = pts_backup; @@ -875,8 +879,10 @@ static int do_packet_auto_bsf(AVFormatContext *s, AVPacket *pkt) { } } +#if FF_API_LAVF_MERGE_SD if (st->internal->nb_bsfcs) av_packet_split_side_data(pkt); +#endif for (i = 0; i < st->internal->nb_bsfcs; i++) { AVBSFContext *ctx = st->internal->bsfcs[i]; diff --git a/libavformat/options_table.h b/libavformat/options_table.h index a537dda95e..0c1915d6d4 100644 --- a/libavformat/options_table.h +++ b/libavformat/options_table.h @@ -48,7 +48,9 @@ static const AVOption avformat_options[] = { {"igndts", "ignore dts", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_IGNDTS }, INT_MIN, INT_MAX, D, "fflags"}, {"discardcorrupt", "discard corrupted frames", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_DISCARD_CORRUPT }, INT_MIN, INT_MAX, D, "fflags"}, {"sortdts", "try to interleave outputted packets by dts", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_SORT_DTS }, INT_MIN, INT_MAX, D, "fflags"}, +#if FF_API_LAVF_KEEPSIDE_FLAG {"keepside", "don't merge side data", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_KEEP_SIDE_DATA }, INT_MIN, INT_MAX, D, "fflags"}, +#endif {"fastseek", "fast but inaccurate seeks", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_FAST_SEEK }, INT_MIN, INT_MAX, D, "fflags"}, {"latm", "enable RTP MP4A-LATM payload", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_MP4A_LATM }, INT_MIN, INT_MAX, E, "fflags"}, {"nobuffer", "reduce the latency introduced by optional buffering", 0, AV_OPT_TYPE_CONST, {.i64 = AVFMT_FLAG_NOBUFFER }, 0, INT_MAX, D, "fflags"}, diff --git a/libavformat/utils.c b/libavformat/utils.c index 37d7024465..a4b4466851 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -1675,8 +1675,10 @@ FF_ENABLE_DEPRECATION_WARNINGS st->inject_global_side_data = 0; } +#if FF_API_LAVF_MERGE_SD if (!(s->flags & AVFMT_FLAG_KEEP_SIDE_DATA)) av_packet_merge_side_data(pkt); +#endif } av_opt_get_dict_val(s, "metadata", AV_OPT_SEARCH_CHILDREN, &metadata); diff --git a/libavformat/version.h b/libavformat/version.h index dc689d45fb..bfc42e3f15 100644 --- a/libavformat/version.h +++ b/libavformat/version.h @@ -33,7 +33,7 @@ // Also please add any ticket numbers that you believe might be affected here #define LIBAVFORMAT_VERSION_MAJOR 57 #define LIBAVFORMAT_VERSION_MINOR 66 -#define LIBAVFORMAT_VERSION_MICRO 104 +#define LIBAVFORMAT_VERSION_MICRO 105 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \ LIBAVFORMAT_VERSION_MINOR, \ @@ -88,6 +88,12 @@ #ifndef FF_API_HLS_WRAP #define FF_API_HLS_WRAP (LIBAVFORMAT_VERSION_MAJOR < 58) #endif +#ifndef FF_API_LAVF_MERGE_SD +#define FF_API_LAVF_MERGE_SD (LIBAVFORMAT_VERSION_MAJOR < 58) +#endif +#ifndef FF_API_LAVF_KEEPSIDE_FLAG +#define FF_API_LAVF_KEEPSIDE_FLAG (LIBAVFORMAT_VERSION_MAJOR < 58) +#endif #ifndef FF_API_R_FRAME_RATE