Re: [libav-devel] [PATCH 07/25] lavfi, buffersrc: switch to the new channel layout API

[Vittorio Giovara]

On Sat, Jul 22, 2017 at 12:02 PM, Anton Khirnov  wrote:

> Quoting Vittorio Giovara (2017-06-29 00:10:51)
> > Signed-off-by: Vittorio Giovara 
> > ---
> >  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(>ch_layout, >ch_layout);
> > +if (ret < 0) {
> > +av_frame_free();
> > +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<

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(>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
> > @@ 

Re: [libav-devel] [PATCH 05/25] avtools: Use the new channel layout API in AVFrame

[Vittorio Giovara]

On Sat, Jul 22, 2017 at 10:15 AM, Anton Khirnov  wrote:

> Quoting Vittorio Giovara (2017-06-29 00:10:49)
> > ---
> >  avtools/avconv.c| 2 +-
> >  avtools/avconv_filter.c | 2 +-
> >  avtools/avplay.c| 4 ++--
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> >
>
> Again, seems premature. The switch should be done at the end. If it
> needs to be done here, then your compatibility layer is broken.
>

ok I'll move tools-related changes to the end
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 04/25] avframe: switch to the new channel layout API

[Vittorio Giovara]

On Sat, Jul 22, 2017 at 10:13 AM, Anton Khirnov  wrote:

> Quoting Vittorio Giovara (2017-06-29 00:10:48)
> > From: Anton Khirnov 
> >
> > Signed-off-by: Vittorio Giovara 
> > ---
> >  libavcodec/decode.c | 67 
> >  libavcodec/encode.c |  9 ++
> >  libavutil/frame.c   | 88 ++
> +--
> >  libavutil/frame.h   | 12 +++-
> >  4 files changed, 146 insertions(+), 30 deletions(-)
> >
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index a49cd77e51..b0d6b9fb33 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -132,7 +132,7 @@ static int unrefcount_frame(AVCodecInternal *avci,
> AVFrame *frame)
> >  memcpy(frame->data, avci->to_free->data,
>  sizeof(frame->data));
> >  memcpy(frame->linesize, avci->to_free->linesize,
> sizeof(frame->linesize));
> >  if (avci->to_free->extended_data != avci->to_free->data) {
> > -int planes = av_get_channel_layout_nb_channels(avci->to_free->
> channel_layout);
> > +int planes = avci->to_free->ch_layout.nb_channels;
> >  int size   = planes * sizeof(*frame->extended_data);
> >
> >  if (!size) {
> > @@ -153,9 +153,19 @@ static int unrefcount_frame(AVCodecInternal *avci,
> AVFrame *frame)
> >  frame->format = avci->to_free->format;
> >  frame->width  = avci->to_free->width;
> >  frame->height = avci->to_free->height;
> > +#if FF_API_OLD_CHANNEL_LAYOUT
> > +FF_DISABLE_DEPRECATION_WARNINGS
> >  frame->channel_layout = avci->to_free->channel_layout;
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
> >  frame->nb_samples = avci->to_free->nb_samples;
> >
> > +ret = av_channel_layout_copy(>ch_layout,
> >to_free->ch_layout);
> > +if (ret < 0) {
> > +av_frame_unref(frame);
> > +return ret;
> > +}
> > +
> >  return 0;
> >  }
> >
> > @@ -887,10 +897,20 @@ static int update_frame_pool(AVCodecContext
> *avctx, AVFrame *frame)
> >  break;
> >  }
> >  case AVMEDIA_TYPE_AUDIO: {
> > -int ch = av_get_channel_layout_nb_channels(frame->channel_
> layout);
> > +int ch = frame->ch_layout.nb_channels;
> >  int planar = av_sample_fmt_is_planar(frame->format);
> >  int planes = planar ? ch : 1;
> >
> > +#if FF_API_OLD_CHANNEL_LAYOUT
> > +FF_DISABLE_DEPRECATION_WARNINGS
> > +if (!ch && frame->channel_layout) {
> > +av_channel_layout_from_mask(>ch_layout,
> frame->channel_layout);
> > +ch = frame->ch_layout.nb_channels;
> > +planes = planar ? ch : 1;
> > +}
> > +FF_ENABLE_DEPRECATION_WARNINGS
> > +#endif
>
> What is this for? This code is the internal get_buffer2()
> implementation, so the new channel layout API should always be used.
>

Yes this chunk is unneeded I'll drop it.


> > +
> >  if (pool->format == frame->format && pool->planes == planes &&
> >  pool->channels == ch && frame->nb_samples == pool->samples)
> >  return 0;
> > @@ -,28 +1131,35 @@ int ff_get_buffer(AVCodecContext *avctx, AVFrame
> *frame, int flags)
> >  frame->sample_rate= avctx->sample_rate;
> >  if (frame->format < 0)
> >  frame->format = avctx->sample_fmt;
> > +if (!frame->ch_layout.nb_channels) {
> > +if (avctx->channel_layout)
> > +av_channel_layout_from_mask(>ch_layout,
> avctx->channel_layout);
> > +else
> > +av_channel_layout_default(>ch_layout,
> avctx->channels);
> > +}
>
> Same, why the deprecated API use.
>

This chunk is actually needed because some decoders don't set the channel
layout fully before ff_get_buffer.
Besides this use is going to be dropped in a later patch, as the
AVCodecContext is fully ported to the new API.
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 04/25] avframe: switch to the new channel layout API

[Vittorio Giovara]

On Sat, Jul 22, 2017 at 10:02 AM, Anton Khirnov  wrote:

> Quoting Vittorio Giovara (2017-06-29 00:10:48)
> > From: Anton Khirnov 
> >
> > Signed-off-by: Vittorio Giovara 
> > ---
> >  libavcodec/decode.c | 67 
> >  libavcodec/encode.c |  9 ++
>
> Why are there lavc changes in this patch. Should they not come after
> lavc conversion? I'd expect this patch to break FATE.
>

I'll split this patch in two. No this does not break FATE in the current
incarnation.
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 06/25] lavr: switch to the new channel layout API

[Vittorio Giovara]

On Sat, Jul 22, 2017 at 10:40 AM, Anton Khirnov  wrote:

> Quoting Vittorio Giovara (2017-06-29 00:10:50)
> > diff --git a/libavresample/utils.c b/libavresample/utils.c
> > index bab2153b4b..15c827efbe 100644
> > --- a/libavresample/utils.c
> > +++ b/libavresample/utils.c
> > @@ -18,6 +18,7 @@
> >   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> >   */
> >
> > +#include "libavutil/channel_layout.h"
> >  #include "libavutil/common.h"
> >  #include "libavutil/dict.h"
> >  #include "libavutil/error.h"
> > @@ -35,6 +36,7 @@
> >
> >  int avresample_open(AVAudioResampleContext *avr)
> >  {
> > +int in_ch, out_ch;
> >  int ret;
> >
> >  if (avresample_is_open(avr)) {
> > @@ -42,24 +44,67 @@ int avresample_open(AVAudioResampleContext *avr)
> >  return AVERROR(EINVAL);
> >  }
> >
> > +if ( avr->in_ch_layout.order == AV_CHANNEL_ORDER_CUSTOM ||
> > +avr->out_ch_layout.order == AV_CHANNEL_ORDER_CUSTOM) {
> > +av_log(avr, AV_LOG_ERROR,
> > +  "Resampling a custom channel layout order is not
> supported.\n");
> > +   return AVERROR(ENOSYS);
>
> Why? I don't remember if I tested it properly back then, but IIRC the
> matrix building code should work custom orders just fine.
>
> > +}
> > +
> > +if (avr->in_ch_layout.order == AV_CHANNEL_ORDER_UNSPEC) {
> > +if (avr->in_ch_layout.nb_channels > 63) {
> > +   av_log(avr, AV_LOG_ERROR,
> > +  "Unspecified channel layout order is supported only
> for up "
> > +  "to 63 channels (got %d).\n",
> avr->in_ch_layout.nb_channels);
> > +   return AVERROR(ENOSYS);
> > +}
> > +av_channel_layout_default(>in_ch_layout,
> avr->in_ch_layout.nb_channels);
> > +}
> > +if (avr->out_ch_layout.order == AV_CHANNEL_ORDER_UNSPEC) {
> > +if (avr->out_ch_layout.nb_channels > 63) {
> > +   av_log(avr, AV_LOG_ERROR,
> > +  "Unspecified channel layout order is supported only
> for up "
> > +  "to 63 channels (got %d).\n", avr->out_ch_layout.nb_
> channels);
> > +   return AVERROR(ENOSYS);
> > +}
> > +av_channel_layout_default(>out_ch_layout,
> avr->out_ch_layout.nb_channels);
> > +}
>
> Why are those needed? Seems they are redundant given the other checks
> right below.
>

You are right on both points, the code is fine (and simpler) without these
checks.
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel