On Sat, Jul 22, 2017 at 12:02 PM, Anton Khirnov <an...@khirnov.net> wrote:
> Quoting Vittorio Giovara (2017-06-29 00:10:51)
> > Signed-off-by: Vittorio Giovara <vittorio.giov...@gmail.com>
> > ---
> > libavfilter/audio.c | 17 +++++++++++++--
> > libavfilter/avfilter.c | 9 ++++----
> > libavfilter/avfilter.h | 13 ++++++++++-
> > libavfilter/avfiltergraph.c | 35 +++++++++++++++++++++++-------
> > libavfilter/buffersink.c | 2 +-
> > libavfilter/buffersrc.c | 53 ++++++++++++++++++++++++++++--
> ---------------
> > libavfilter/buffersrc.h | 12 +++++++++-
> > libavfilter/fifo.c | 7 +++---
> > 8 files changed, 106 insertions(+), 42 deletions(-)
> >
> > diff --git a/libavfilter/audio.c b/libavfilter/audio.c
> > index 5fe9da95c3..afd8bdc169 100644
> > --- a/libavfilter/audio.c
> > +++ b/libavfilter/audio.c
> > @@ -31,7 +31,7 @@ AVFrame *ff_null_get_audio_buffer(AVFilterLink *link,
> int nb_samples)
> > AVFrame *ff_default_get_audio_buffer(AVFilterLink *link, int
> nb_samples)
> > {
> > AVFrame *frame = av_frame_alloc();
> > - int channels = av_get_channel_layout_nb_
> channels(link->channel_layout);
> > + int channels = link->ch_layout.nb_channels;
> > int ret;
> >
> > if (!frame)
> > @@ -39,7 +39,20 @@ AVFrame *ff_default_get_audio_buffer(AVFilterLink
> *link, int nb_samples)
> >
> > frame->nb_samples = nb_samples;
> > frame->format = link->format;
> > - frame->channel_layout = link->channel_layout;
> > +
> > + ret = av_channel_layout_copy(&frame->ch_layout, &link->ch_layout);
> > + if (ret < 0) {
> > + av_frame_free(&frame);
> > + return NULL;
> > + }
> > +#if FF_API_OLD_CHANNEL_LAYOUT
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > + if (link->ch_layout.order == AV_CHANNEL_ORDER_NATIVE ||
> > + link->ch_layout.order == AV_CHANNEL_ORDER_UNSPEC)
> > + frame->channel_layout = link->channel_layout;
>
> This is incomplete, the channel layout should always be set, at least to
> (1<<count) - 1.
>
Right, I added this else as you suggested.
else
frame->channel_layout = (1 << channels) - 1;
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
> > +
> > frame->sample_rate = link->sample_rate;
> > ret = av_frame_get_buffer(frame, 0);
> > if (ret < 0) {
> > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> > index 83c1a7c20d..f2adefff3d 100644
> > --- a/libavfilter/avfilter.c
> > +++ b/libavfilter/avfilter.c
> > @@ -247,16 +247,15 @@ void ff_dlog_link(void *ctx, AVFilterLink *link,
> int end)
> > link->dst ? link->dst->filter->name : "",
> > end ? "\n" : "");
> > } else {
> > - char buf[128];
> > - av_get_channel_layout_string(buf, sizeof(buf), -1,
> link->channel_layout);
> > -
> > + char *chlstr = av_channel_layout_describe(&link->ch_layout);
> > av_log(ctx, AV_LOG_TRACE,
> > "link[%p r:%d cl:%s fmt:%-16s %-16s->%-16s]%s",
> > - link, link->sample_rate, buf,
> > + link, link->sample_rate, chlstr,
> > av_get_sample_fmt_name(link->format),
> > link->src ? link->src->filter->name : "",
> > link->dst ? link->dst->filter->name : "",
> > end ? "\n" : "");
> > + av_free(chlstr);
> > }
> > }
> >
> > @@ -683,7 +682,7 @@ int ff_filter_frame(AVFilterLink *link, AVFrame
> *frame)
> > case AVMEDIA_TYPE_AUDIO:
> > av_samples_copy(out->extended_data, frame->extended_data,
> > 0, 0, frame->nb_samples,
> > - av_get_channel_layout_nb_
> channels(frame->channel_layout),
> > + frame->ch_layout.nb_channels,
> > frame->format);
> > break;
> > default:
> > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h
> > index 6df69dbbbf..5d5edf0ed3 100644
> > --- a/libavfilter/avfilter.h
> > +++ b/libavfilter/avfilter.h
> > @@ -36,6 +36,7 @@
> > #include "libavutil/attributes.h"
> > #include "libavutil/avutil.h"
> > #include "libavutil/buffer.h"
> > +#include "libavutil/channel_layout.h"
> > #include "libavutil/frame.h"
> > #include "libavutil/log.h"
> > #include "libavutil/samplefmt.h"
> > @@ -334,7 +335,12 @@ struct AVFilterLink {
> > int h; ///< agreed upon image height
> > AVRational sample_aspect_ratio; ///< agreed upon sample aspect ratio
> > /* These two parameters apply only to audio */
> > - uint64_t channel_layout; ///< channel layout of current buffer
> (see libavutil/channel_layout.h)
> > +#if FF_API_OLD_CHANNEL_LAYOUT
> > + /**
> > + * @deprecated use ch_layout instead
> > + */
> > + attribute_deprecated uint64_t channel_layout;
> > +#endif
> > int sample_rate; ///< samples per second
> >
> > int format; ///< agreed upon media format
> > @@ -405,6 +411,11 @@ struct AVFilterLink {
> > * AVHWFramesContext describing the frames.
> > */
> > AVBufferRef *hw_frames_ctx;
> > +
> > + /**
> > + * Channel layout of current buffer.
>
> What 'current buffer' is this talking about?
>
I mostly moved the existing documentation, I believe this refers to the
avframe buffer that is being passed through the link.
BTW, I noticed that this field is in the wrong section of AVFilterLink,
I'll move it above after time_base.
> > + */
> > + AVChannelLayout ch_layout;
> > };
> >
> > /**
> > diff --git a/libavfilter/avfiltergraph.c b/libavfilter/avfiltergraph.c
> > index a0f797e283..c72016d2c8 100644
> > --- a/libavfilter/avfiltergraph.c
> > +++ b/libavfilter/avfiltergraph.c
>
[...]
> > @@ -124,8 +124,17 @@ int av_buffersrc_parameters_set(AVFilterContext
> *ctx, AVBufferSrcParameters *par
> > }
> > if (param->sample_rate > 0)
> > s->sample_rate = param->sample_rate;
> > - if (param->channel_layout)
> > - s->channel_layout = param->channel_layout;
> > +
> > + ret = av_channel_layout_copy(&s->ch_layout, ¶m->ch_layout);
> > + if (ret < 0)
> > + return ret;
> > +
> > +#if FF_API_OLD_CHANNEL_LAYOUT
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > + if (!av_channel_layout_check(&s->ch_layout))
> > + av_channel_layout_from_mask(&s->ch_layout,
> param->channel_layout);
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
>
> I think it would be more consistent to always prefer the deprecated
> channel_layout if it is set.
>
> > break;
> > default:
> > return AVERROR_BUG;
>
[...]
> > @@ -371,7 +375,16 @@ static int config_props(AVFilterLink *link)
> > }
> > break;
> > case AVMEDIA_TYPE_AUDIO:
> > - link->channel_layout = c->channel_layout;
> > + ret = av_channel_layout_copy(&link->ch_layout, &c->ch_layout);
> > + if (ret < 0)
> > + return ret;
> > +#if FF_API_OLD_CHANNEL_LAYOUT
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > + if (c->ch_layout.order == AV_CHANNEL_ORDER_NATIVE ||
> > + c->ch_layout.order == AV_CHANNEL_ORDER_UNSPEC)
> > + link->channel_layout = c->ch_layout.u.mask;
>
> Same as above
And leave it unset if the old channel_layout is unset? Won't that affect
the transition period for any lavfi API caller?
I also dropped the old channel_layout in BufferSourceContext so that I
could only validate one field only.
Unless I'm misunderstanding your suggestion, I'd rather move everything to
the new API and guarantee compatibility deriving from it, instead of
lingering on the old one.
--
Vittorio
_______________________________________________
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel
