[FFmpeg-devel,1/2] avcodec, avformat: deprecate anything related to side data merging

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

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