[FFmpeg-devel] lavfi/buffersink: deprecate non-AVOption init.

Submitted by Nicolas George on Sept. 12, 2017, 9:40 a.m.

Details

Message ID 20170912094055.16345-1-george@nsup.org
State New
Headers show

Commit Message

Nicolas George Sept. 12, 2017, 9:40 a.m.
Signed-off-by: Nicolas George <george@nsup.org>
---
 doc/APIchanges           |  3 +++
 libavfilter/buffersink.c | 10 ++++++++++
 libavfilter/buffersink.h | 12 ++++++++----
 3 files changed, 21 insertions(+), 4 deletions(-)

Comments

wm4 Sept. 19, 2017, 7:43 p.m.
On Tue, 12 Sep 2017 11:40:55 +0200
Nicolas George <george@nsup.org> wrote:

> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  doc/APIchanges           |  3 +++
>  libavfilter/buffersink.c | 10 ++++++++++
>  libavfilter/buffersink.h | 12 ++++++++----
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index cc67cbf6f8..be136ca11e 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2015-08-28
>  
>  API changes, most recent first:
>  
> +2017-09-12 - xxxxxxx - lavfi 6.XXX.100 - buffersink.h
> +  Deprecate non-AVOption init of buffersink.
> +
>  2017-09-08 - xxxxxxx - lavfi 6.103.100 - buffersrc.h
>    Add av_buffersrc_close().
>  
> diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c
> index 0f87b5439a..0d5380c681 100644
> --- a/libavfilter/buffersink.c
> +++ b/libavfilter/buffersink.c
> @@ -127,6 +127,8 @@ int attribute_align_arg av_buffersink_get_samples(AVFilterContext *ctx,
>      return get_frame_internal(ctx, frame, 0, nb_samples);
>  }
>  
> +FF_DISABLE_DEPRECATION_WARNINGS
> +
>  AVBufferSinkParams *av_buffersink_params_alloc(void)
>  {
>      static const int pixel_fmts[] = { AV_PIX_FMT_NONE };
> @@ -147,6 +149,8 @@ AVABufferSinkParams *av_abuffersink_params_alloc(void)
>      return params;
>  }
>  
> +FF_ENABLE_DEPRECATION_WARNINGS
> +
>  static av_cold int common_init(AVFilterContext *ctx)
>  {
>      BufferSinkContext *buf = ctx->priv;
> @@ -201,6 +205,7 @@ MAKE_AVFILTERLINK_ACCESSOR(int              , sample_rate        )
>  
>  MAKE_AVFILTERLINK_ACCESSOR(AVBufferRef *    , hw_frames_ctx      )
>  
> +FF_DISABLE_DEPRECATION_WARNINGS
>  static av_cold int vsink_init(AVFilterContext *ctx, void *opaque)
>  {
>      BufferSinkContext *buf = ctx->priv;
> @@ -208,12 +213,14 @@ static av_cold int vsink_init(AVFilterContext *ctx, void *opaque)
>      int ret;
>  
>      if (params) {
> +        av_log(ctx, AV_LOG_WARNING, "non-AVOption init of buffersink is deprecated\n");
>          if ((ret = av_opt_set_int_list(buf, "pix_fmts", params->pixel_fmts, AV_PIX_FMT_NONE, 0)) < 0)
>              return ret;
>      }
>  
>      return common_init(ctx);
>  }
> +FF_ENABLE_DEPRECATION_WARNINGS
>  
>  #define CHECK_LIST_SIZE(field) \
>          if (buf->field ## _size % sizeof(*buf->field)) { \
> @@ -244,6 +251,7 @@ static int vsink_query_formats(AVFilterContext *ctx)
>      return 0;
>  }
>  
> +FF_DISABLE_DEPRECATION_WARNINGS
>  static av_cold int asink_init(AVFilterContext *ctx, void *opaque)
>  {
>      BufferSinkContext *buf = ctx->priv;
> @@ -251,6 +259,7 @@ static av_cold int asink_init(AVFilterContext *ctx, void *opaque)
>      int ret;
>  
>      if (params) {
> +        av_log(ctx, AV_LOG_WARNING, "non-AVOption init of abuffersink is deprecated\n");
>          if ((ret = av_opt_set_int_list(buf, "sample_fmts",     params->sample_fmts,  AV_SAMPLE_FMT_NONE, 0)) < 0 ||
>              (ret = av_opt_set_int_list(buf, "sample_rates",    params->sample_rates,    -1, 0)) < 0 ||
>              (ret = av_opt_set_int_list(buf, "channel_layouts", params->channel_layouts, -1, 0)) < 0 ||
> @@ -260,6 +269,7 @@ static av_cold int asink_init(AVFilterContext *ctx, void *opaque)
>      }
>      return common_init(ctx);
>  }
> +FF_ENABLE_DEPRECATION_WARNINGS
>  
>  static int asink_query_formats(AVFilterContext *ctx)
>  {
> diff --git a/libavfilter/buffersink.h b/libavfilter/buffersink.h
> index 21d6bb505b..d607bca631 100644
> --- a/libavfilter/buffersink.h
> +++ b/libavfilter/buffersink.h
> @@ -61,8 +61,10 @@ int av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flag
>  
>  /**
>   * Struct to use for initializing a buffersink context.
> + *
> + * @deprecated Use av_opt_set() on the newly created filter.
>   */
> -typedef struct AVBufferSinkParams {
> +attribute_deprecated typedef struct AVBufferSinkParams {
>      const enum AVPixelFormat *pixel_fmts; ///< list of allowed pixel formats, terminated by AV_PIX_FMT_NONE
>  } AVBufferSinkParams;
>  
> @@ -71,12 +73,14 @@ typedef struct AVBufferSinkParams {
>   *
>   * Must be freed with av_free().
>   */
> -AVBufferSinkParams *av_buffersink_params_alloc(void);
> +attribute_deprecated AVBufferSinkParams *av_buffersink_params_alloc(void);
>  
>  /**
>   * Struct to use for initializing an abuffersink context.
> + *
> + * @deprecated Use av_opt_set() on the newly created filter.
>   */
> -typedef struct AVABufferSinkParams {
> +attribute_deprecated typedef struct AVABufferSinkParams {
>      const enum AVSampleFormat *sample_fmts; ///< list of allowed sample formats, terminated by AV_SAMPLE_FMT_NONE
>      const int64_t *channel_layouts;         ///< list of allowed channel layouts, terminated by -1
>      const int *channel_counts;              ///< list of allowed channel counts, terminated by -1
> @@ -89,7 +93,7 @@ typedef struct AVABufferSinkParams {
>   *
>   * Must be freed with av_free().
>   */
> -AVABufferSinkParams *av_abuffersink_params_alloc(void);
> +attribute_deprecated AVABufferSinkParams *av_abuffersink_params_alloc(void);
>  
>  /**
>   * Set the frame size for an audio buffer sink.

I'm very strongly against this and veto it with all my might.
Nicolas George Sept. 28, 2017, 2:04 p.m.
Le sextidi 26 fructidor, an CCXXV, Nicolas George a écrit :
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  doc/APIchanges           |  3 +++
>  libavfilter/buffersink.c | 10 ++++++++++
>  libavfilter/buffersink.h | 12 ++++++++----
>  3 files changed, 21 insertions(+), 4 deletions(-)

Will push soon if there is no objection.

Regards,
James Almer Sept. 28, 2017, 2:10 p.m.
On 9/28/2017 11:04 AM, Nicolas George wrote:
> Le sextidi 26 fructidor, an CCXXV, Nicolas George a écrit :
>> Signed-off-by: Nicolas George <george@nsup.org>
>> ---
>>  doc/APIchanges           |  3 +++
>>  libavfilter/buffersink.c | 10 ++++++++++
>>  libavfilter/buffersink.h | 12 ++++++++----
>>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> Will push soon if there is no objection.
> 
> Regards,

There was a very strong objection to this patch in another reply. You
should ask him to elaborate if necessary.
Nicolas George Sept. 28, 2017, 2:14 p.m.
Le septidi 7 vendémiaire, an CCXXVI, James Almer a écrit :
> There was a very strong objection to this patch in another reply. You
> should ask him to elaborate if necessary.

I do not remember. Checking. Oh, you mean wm4. I no longer read their
mails, and it seems I was right: this is no objection.

Will push soon unless there is an objection.

Regards,
James Almer Sept. 28, 2017, 2:20 p.m.
On 9/28/2017 11:14 AM, Nicolas George wrote:
> Le septidi 7 vendémiaire, an CCXXVI, James Almer a écrit :
>> There was a very strong objection to this patch in another reply. You
>> should ask him to elaborate if necessary.
> 
> I do not remember. Checking. Oh, you mean wm4. I no longer read their
> mails, and it seems I was right: this is no objection.
> 
> Will push soon unless there is an objection.
> 
> Regards,

That's not how things are done and you know it. You do not simply ignore
another developer's email just because you don't like them. And
*especially* not when it's a veto to an API change.

This is a project handled by several people and it doesn't care about
your personal grudges. So please, ask him to elaborate why he's against
this patch, and don't push it until both parts are satisfied or other
people intervene to argue in favor or one side of the discussion or the
other.
wm4 Sept. 28, 2017, 2:21 p.m.
On Thu, 28 Sep 2017 16:14:40 +0200
Nicolas George <george@nsup.org> wrote:

> Le septidi 7 vendémiaire, an CCXXVI, James Almer a écrit :
> > There was a very strong objection to this patch in another reply. You
> > should ask him to elaborate if necessary.  
> 
> I do not remember. Checking. Oh, you mean wm4. I no longer read their
> mails, and it seems I was right: this is no objection.
> 
> Will push soon unless there is an objection.

So you openly admit on shitting on the project rules? That is nice of
you. (It's not the first time you ignore my patch review comments.)
wm4 Sept. 28, 2017, 2:45 p.m.
On Tue, 12 Sep 2017 11:40:55 +0200
Nicolas George <george@nsup.org> wrote:

> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  doc/APIchanges           |  3 +++
>  libavfilter/buffersink.c | 10 ++++++++++
>  libavfilter/buffersink.h | 12 ++++++++----
>  3 files changed, 21 insertions(+), 4 deletions(-)

jamrial asked to clarify this, so here's some reasoning why I'm against
it:

The alternative would be using AVOptions, but AVOptions are clunky.
Especially when setting things like lists of allowed formats or channel
layouts. I don't like AVOption APis in general, but with those lists
it gets seriously error-prone on the API user side.

On the source filter side, AVBufferSrcParameters was recently
introduced, so that alone is an indicator that a dedicated params
struct makes sense, and that we shouldn't remove it.

I would argue that there should be a av_abuffersink_parameters_set
function, instead of using the really unsafe opaque init parameter. I'd
also respect if we were to merge AVABufferSinkParams into
AVBufferSinkParams, or if this were preparation for adding a similar 
but more general mechanism for both FFmpeg/Libav (non-AVOption of
course), but I haven't noticed such plans.

All in all, just deprecating/removing this would be a step backwards,
and would force current users of this API to use more awkward API
instead. There was also no good (or any) reason given _for_ removing
the API.
James Almer Sept. 28, 2017, 3:51 p.m.
On 9/28/2017 11:29 AM, Nicolas George wrote:
> [ Replying in private so as not to escalate the troll. ]

No. I'm CCing to the list as this is a public discussion.

> 
> Le septidi 7 vendémiaire, an CCXXVI, James Almer a écrit :
>> That's not how things are done and you know it. You do not simply ignore
>> another developer's email just because you don't like them. And
> 
> I could answer just: watch me. But I like to be more constructive.

I say it again, you don't ignore other developer's emails. That "watch
me do whatever i want" attitude is what nobody likes or wants in any
kind of OSS project, and is absolutely unacceptable.

> 
>> *especially* not when it's a veto to an API change.
> 
> There are no vetos in this project, only arguments. The message you
> pointed me is devoid of it.

If there are developers against a patch, unless they're outnumbered then
it works as a veto. Then of course we also have the voting system to
make it official if things are too tied or heated up.

> 
>> This is a project handled by several people and it doesn't care about
>> your personal grudges. So please, ask him to elaborate why he's against
>> this patch, and don't push it until both parts are satisfied or other
>> people intervene to argue in favor or one side of the discussion or the
>> other.
> 
> If they had anything to elaborate, they could have done, and they still
> can. I wrote that I will push *if there are no objections*. There are
> none, the message you pointed is not an objection, it is a tantrum.

You didn't explain why you want the patch applied either. The commit
message is empty. And since now wm4 explained why he's against, it's
your turn to both explain why you're in favor and to address his argument.

Don't escalate this thing any further. We don't need more drama or
flamewars here. Nothing good ever comes from that. You two argue why
you're in favor or against this patch, and don't commit anything until
that's solved.

Patch hide | download patch | download mbox

diff --git a/doc/APIchanges b/doc/APIchanges
index cc67cbf6f8..be136ca11e 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@  libavutil:     2015-08-28
 
 API changes, most recent first:
 
+2017-09-12 - xxxxxxx - lavfi 6.XXX.100 - buffersink.h
+  Deprecate non-AVOption init of buffersink.
+
 2017-09-08 - xxxxxxx - lavfi 6.103.100 - buffersrc.h
   Add av_buffersrc_close().
 
diff --git a/libavfilter/buffersink.c b/libavfilter/buffersink.c
index 0f87b5439a..0d5380c681 100644
--- a/libavfilter/buffersink.c
+++ b/libavfilter/buffersink.c
@@ -127,6 +127,8 @@  int attribute_align_arg av_buffersink_get_samples(AVFilterContext *ctx,
     return get_frame_internal(ctx, frame, 0, nb_samples);
 }
 
+FF_DISABLE_DEPRECATION_WARNINGS
+
 AVBufferSinkParams *av_buffersink_params_alloc(void)
 {
     static const int pixel_fmts[] = { AV_PIX_FMT_NONE };
@@ -147,6 +149,8 @@  AVABufferSinkParams *av_abuffersink_params_alloc(void)
     return params;
 }
 
+FF_ENABLE_DEPRECATION_WARNINGS
+
 static av_cold int common_init(AVFilterContext *ctx)
 {
     BufferSinkContext *buf = ctx->priv;
@@ -201,6 +205,7 @@  MAKE_AVFILTERLINK_ACCESSOR(int              , sample_rate        )
 
 MAKE_AVFILTERLINK_ACCESSOR(AVBufferRef *    , hw_frames_ctx      )
 
+FF_DISABLE_DEPRECATION_WARNINGS
 static av_cold int vsink_init(AVFilterContext *ctx, void *opaque)
 {
     BufferSinkContext *buf = ctx->priv;
@@ -208,12 +213,14 @@  static av_cold int vsink_init(AVFilterContext *ctx, void *opaque)
     int ret;
 
     if (params) {
+        av_log(ctx, AV_LOG_WARNING, "non-AVOption init of buffersink is deprecated\n");
         if ((ret = av_opt_set_int_list(buf, "pix_fmts", params->pixel_fmts, AV_PIX_FMT_NONE, 0)) < 0)
             return ret;
     }
 
     return common_init(ctx);
 }
+FF_ENABLE_DEPRECATION_WARNINGS
 
 #define CHECK_LIST_SIZE(field) \
         if (buf->field ## _size % sizeof(*buf->field)) { \
@@ -244,6 +251,7 @@  static int vsink_query_formats(AVFilterContext *ctx)
     return 0;
 }
 
+FF_DISABLE_DEPRECATION_WARNINGS
 static av_cold int asink_init(AVFilterContext *ctx, void *opaque)
 {
     BufferSinkContext *buf = ctx->priv;
@@ -251,6 +259,7 @@  static av_cold int asink_init(AVFilterContext *ctx, void *opaque)
     int ret;
 
     if (params) {
+        av_log(ctx, AV_LOG_WARNING, "non-AVOption init of abuffersink is deprecated\n");
         if ((ret = av_opt_set_int_list(buf, "sample_fmts",     params->sample_fmts,  AV_SAMPLE_FMT_NONE, 0)) < 0 ||
             (ret = av_opt_set_int_list(buf, "sample_rates",    params->sample_rates,    -1, 0)) < 0 ||
             (ret = av_opt_set_int_list(buf, "channel_layouts", params->channel_layouts, -1, 0)) < 0 ||
@@ -260,6 +269,7 @@  static av_cold int asink_init(AVFilterContext *ctx, void *opaque)
     }
     return common_init(ctx);
 }
+FF_ENABLE_DEPRECATION_WARNINGS
 
 static int asink_query_formats(AVFilterContext *ctx)
 {
diff --git a/libavfilter/buffersink.h b/libavfilter/buffersink.h
index 21d6bb505b..d607bca631 100644
--- a/libavfilter/buffersink.h
+++ b/libavfilter/buffersink.h
@@ -61,8 +61,10 @@  int av_buffersink_get_frame_flags(AVFilterContext *ctx, AVFrame *frame, int flag
 
 /**
  * Struct to use for initializing a buffersink context.
+ *
+ * @deprecated Use av_opt_set() on the newly created filter.
  */
-typedef struct AVBufferSinkParams {
+attribute_deprecated typedef struct AVBufferSinkParams {
     const enum AVPixelFormat *pixel_fmts; ///< list of allowed pixel formats, terminated by AV_PIX_FMT_NONE
 } AVBufferSinkParams;
 
@@ -71,12 +73,14 @@  typedef struct AVBufferSinkParams {
  *
  * Must be freed with av_free().
  */
-AVBufferSinkParams *av_buffersink_params_alloc(void);
+attribute_deprecated AVBufferSinkParams *av_buffersink_params_alloc(void);
 
 /**
  * Struct to use for initializing an abuffersink context.
+ *
+ * @deprecated Use av_opt_set() on the newly created filter.
  */
-typedef struct AVABufferSinkParams {
+attribute_deprecated typedef struct AVABufferSinkParams {
     const enum AVSampleFormat *sample_fmts; ///< list of allowed sample formats, terminated by AV_SAMPLE_FMT_NONE
     const int64_t *channel_layouts;         ///< list of allowed channel layouts, terminated by -1
     const int *channel_counts;              ///< list of allowed channel counts, terminated by -1
@@ -89,7 +93,7 @@  typedef struct AVABufferSinkParams {
  *
  * Must be freed with av_free().
  */
-AVABufferSinkParams *av_abuffersink_params_alloc(void);
+attribute_deprecated AVABufferSinkParams *av_abuffersink_params_alloc(void);
 
 /**
  * Set the frame size for an audio buffer sink.