| Message ID | 20170912094055.16345-1-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
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.
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,
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.
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,
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.
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.)
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.
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.
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.
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(-)