Web lists-archives.com

Re: [MPlayer-dev-eng] [PATCH] Replace deprecated avcodec resamples with libswresample in af_lavcresample




On Wed, 12 Aug 2015 14:17:58 +0200
Michael Niedermayer <michael@xxxxxxxxxxxxxx> wrote:

> On Mon, Aug 10, 2015 at 11:29:57PM +0200, Roberto Togni wrote:
> > On Mon, 10 Aug 2015 12:18:45 +0200
> > wm4 <nfxjfg@xxxxxxxxxxxxxx> wrote:
> > 
> > > On Mon, 10 Aug 2015 01:49:12 +0200
> > > Roberto Togni <rxt@xxxxxxxxx> wrote:
> > > 
> > > > Hello,
> > > >  this patch updates af_lavcresample to use libswresample instead of the
> > > > deprecated avcodec resampler (resample2).
> > > > 
> > > > It works for me; please test, it especially with multichannel setup.
> > > 
> > > This shouldn't change anything about multichannel. Since the input and
> > > output layouts are the same, libswresample will not remix the channels.
> > > 
> > > Also, you should use av_malloc to allocate aligned buffers for
> > > libswresample, and BUFFER_PADDING doesn't look like the correct
> > > constant to use.
> > 
> > Thanks for the feedback.
> > New version attached, with aligned allocation and minimal buffer
> > padding.
> > 
> > I'll apply shortly if nobody complains.
> > 
> > Ciao,
> >  Roberto
> 
> >  af_lavcresample.c |   99 ++++++++++++++++++++----------------------------------
> >  1 file changed, 38 insertions(+), 61 deletions(-)
> > 81dadefee29ae11f6c94cccc97969bb5f7eb13e4  af_lavcresample.c.diff
> > Index: af_lavcresample.c
> > ===================================================================
> > --- af_lavcresample.c	(revisione 37444)
> > +++ af_lavcresample.c	(copia locale)
> > @@ -25,15 +25,20 @@
> >  
> >  #include "config.h"
> >  #include "af.h"
> > -#include "libavcodec/avcodec.h"
> >  #include "libavutil/rational.h"
> > +#include "libswresample/swresample.h"
> > +#include "libavutil/channel_layout.h"
> > +#include "libavutil/opt.h"
> > +#include "libavutil/mem.h"
> > +#include "libavutil/common.h"
> >  
> >  // Data for specific instances of this filter
> >  typedef struct af_resample_s{
> > -    struct AVResampleContext *avrctx;
> > -    int16_t *in[AF_NCH];
> > +    struct SwrContext *swrctx;
> > +    uint8_t *in[1];
> > +    uint8_t *tmp[1];
> >      int in_alloc;
> > -    int index;
> > +    int tmp_alloc;
> >  
> >      int filter_length;
> >      int linear;
> > @@ -70,8 +75,19 @@
> >  
> >      if (s->ctx_out_rate != af->data->rate || s->ctx_in_rate != data->rate || s->ctx_filter_size != s->filter_length ||
> >          s->ctx_phase_shift != s->phase_shift || s->ctx_linear != s->linear || s->ctx_cutoff != s->cutoff) {
> > -        if(s->avrctx) av_resample_close(s->avrctx);
> > -        s->avrctx= av_resample_init(af->data->rate, /*in_rate*/data->rate, s->filter_length, s->phase_shift, s->linear, s->cutoff);
> 
> > +        if(s->swrctx) swr_free(&s->swrctx);
> 
> the if() should be unneeded
Removed.
Should this be added to the swr_free doxygen, or do all the ffmpeg
context freeing functions take NULL as a valid input?

> 
> 
> > +        s->swrctx=swr_alloc();
> 
> missing (malloc) failure check
> 
> > +        av_opt_set_int(s->swrctx, "out_sample_rate", af->data->rate, 0);
> > +        av_opt_set_int(s->swrctx, "in_sample_rate", data->rate, 0);
> > +        av_opt_set_int(s->swrctx, "filter_size", s->filter_length, 0);
> > +        av_opt_set_int(s->swrctx, "phase_shift", s->phase_shift, 0);
> > +        av_opt_set_int(s->swrctx, "linear_interp", s->linear, 0);
> > +        av_opt_set_double(s->swrctx, "cutoff", s->cutoff, 0);
> > +        av_opt_set_sample_fmt(s->swrctx, "in_sample_fmt", AV_SAMPLE_FMT_S16, 0);
> > +        av_opt_set_sample_fmt(s->swrctx, "out_sample_fmt", AV_SAMPLE_FMT_S16, 0);
> 
> > +        av_opt_set_channel_layout(s->swrctx, "in_channel_layout", av_get_default_channel_layout(af->data->nch), 0);
> > +        av_opt_set_channel_layout(s->swrctx, "out_channel_layout", av_get_default_channel_layout(af->data->nch), 0);
> 
> Is there a reason why you dont set the number of channels instead of
> the layout ?
> as is, failure is possible for cases where there is no default
> layout
No reason; I just see that the swr_alloc_set_opts() wants a channel
layout, so I thought it was required.
Changed.

> 
> 
> > +        swr_init(s->swrctx);
> 
> missing failure check
Added.

> 
> 
> >          s->ctx_out_rate    = af->data->rate;
> >          s->ctx_in_rate     = data->rate;
> >          s->ctx_filter_size = s->filter_length;
> > @@ -106,11 +122,10 @@
> >          free(af->data->audio);
> >      free(af->data);
> >      if(af->setup){
> > -        int i;
> >          af_resample_t *s = af->setup;
> > -        if(s->avrctx) av_resample_close(s->avrctx);
> > -        for (i=0; i < AF_NCH; i++)
> > -            free(s->in[i]);
> 
> > +        if(s->swrctx) swr_free(&s->swrctx);
> 
> if() should be uneededd
Removed.

> 
> 
> > +        av_free(s->in[0]);
> > +        av_free(s->tmp[0]);
> >          free(s);
> >      }
> >  }
> > @@ -119,71 +134,33 @@
> >  static af_data_t* play(struct af_instance_s* af, af_data_t* data)
> >  {
> >    af_resample_t *s = af->setup;
> > -  int i, j, consumed, ret;
> > -  int16_t *in = (int16_t*)data->audio;
> > -  int16_t *out;
> > +  int ret;
> > +  int8_t *in = (int8_t*)data->audio;
> > +  int8_t *out;
> >    int chans   = data->nch;
> > -  int in_len  = data->len/(2*chans);
> > +  int in_len  = data->len;
> >    int out_len = in_len * af->mul + 10;
> > -  int16_t tmp[AF_NCH][out_len];
> >  
> >    if(AF_OK != RESIZE_LOCAL_BUFFER(af,data))
> >        return NULL;
> >  
> > -  out= (int16_t*)af->data->audio;
> > +  av_fast_malloc(&s->tmp[0], &s->tmp_alloc, FFALIGN(out_len,32));
> >  
> > -  out_len= FFMIN(out_len, af->data->len/(2*chans));
> > +  out= (int8_t*)af->data->audio;
> >  
> > -  if(s->in_alloc < in_len + s->index){
> > -      s->in_alloc= in_len + s->index;
> > -      for(i=0; i<chans; i++){
> > -          s->in[i]= realloc(s->in[i], s->in_alloc*sizeof(int16_t));
> > -      }
> > -  }
> > +  out_len= FFMIN(out_len, af->data->len);
> >  
> > -  if(chans==1){
> > -      memcpy(&s->in[0][s->index], in, in_len * sizeof(int16_t));
> > -  }else if(chans==2){
> > -      for(j=0; j<in_len; j++){
> > -          s->in[0][j + s->index]= *(in++);
> > -          s->in[1][j + s->index]= *(in++);
> > -      }
> > -  }else{
> > -      for(j=0; j<in_len; j++){
> > -          for(i=0; i<chans; i++){
> > -              s->in[i][j + s->index]= *(in++);
> > -          }
> > -      }
> > -  }
> > -  in_len += s->index;
> > +  av_fast_malloc(&s->in[0], &s->in_alloc, FFALIGN(in_len,32));
> >  
> > -  for(i=0; i<chans; i++){
> > -      ret= av_resample(s->avrctx, tmp[i], s->in[i], &consumed, in_len, out_len, i+1 == chans);
> > -  }
> > -  out_len= ret;
> > +  memcpy(s->in[0], in, in_len);
> >  
> > -  s->index= in_len - consumed;
> > -  for(i=0; i<chans; i++){
> > -      memmove(s->in[i], s->in[i] + consumed, s->index*sizeof(int16_t));
> > -  }
> 
> > +  ret = swr_convert(s->swrctx, &s->tmp[0], out_len/chans/2, &s->in[0], in_len/chans/2);
> > +  out_len= ret*chans*2;
> 
> this is missing an error check
> failure is possible due to *malloc() failure at least
Added.

Since there was no failure check (malloc or other) in the old filter, I
didn't care about them.


New version attached.

Ciao,
 Roberto
Index: af_lavcresample.c
===================================================================
--- af_lavcresample.c	(revisione 37444)
+++ af_lavcresample.c	(copia locale)
@@ -25,15 +25,20 @@
 
 #include "config.h"
 #include "af.h"
-#include "libavcodec/avcodec.h"
 #include "libavutil/rational.h"
+#include "libswresample/swresample.h"
+#include "libavutil/channel_layout.h"
+#include "libavutil/opt.h"
+#include "libavutil/mem.h"
+#include "libavutil/common.h"
 
 // Data for specific instances of this filter
 typedef struct af_resample_s{
-    struct AVResampleContext *avrctx;
-    int16_t *in[AF_NCH];
+    struct SwrContext *swrctx;
+    uint8_t *in[1];
+    uint8_t *tmp[1];
     int in_alloc;
-    int index;
+    int tmp_alloc;
 
     int filter_length;
     int linear;
@@ -70,8 +75,19 @@
 
     if (s->ctx_out_rate != af->data->rate || s->ctx_in_rate != data->rate || s->ctx_filter_size != s->filter_length ||
         s->ctx_phase_shift != s->phase_shift || s->ctx_linear != s->linear || s->ctx_cutoff != s->cutoff) {
-        if(s->avrctx) av_resample_close(s->avrctx);
-        s->avrctx= av_resample_init(af->data->rate, /*in_rate*/data->rate, s->filter_length, s->phase_shift, s->linear, s->cutoff);
+        swr_free(&s->swrctx);
+        if((s->swrctx=swr_alloc()) == NULL) return AF_ERROR;
+        av_opt_set_int(s->swrctx, "out_sample_rate", af->data->rate, 0);
+        av_opt_set_int(s->swrctx, "in_sample_rate", data->rate, 0);
+        av_opt_set_int(s->swrctx, "filter_size", s->filter_length, 0);
+        av_opt_set_int(s->swrctx, "phase_shift", s->phase_shift, 0);
+        av_opt_set_int(s->swrctx, "linear_interp", s->linear, 0);
+        av_opt_set_double(s->swrctx, "cutoff", s->cutoff, 0);
+        av_opt_set_sample_fmt(s->swrctx, "in_sample_fmt", AV_SAMPLE_FMT_S16, 0);
+        av_opt_set_sample_fmt(s->swrctx, "out_sample_fmt", AV_SAMPLE_FMT_S16, 0);
+        av_opt_set_int(s->swrctx, "in_channel_count", af->data->nch, 0);
+        av_opt_set_int(s->swrctx, "out_channel_count", af->data->nch, 0);
+        if(swr_init(s->swrctx) < 0) return AF_ERROR;
         s->ctx_out_rate    = af->data->rate;
         s->ctx_in_rate     = data->rate;
         s->ctx_filter_size = s->filter_length;
@@ -106,11 +122,10 @@
         free(af->data->audio);
     free(af->data);
     if(af->setup){
-        int i;
         af_resample_t *s = af->setup;
-        if(s->avrctx) av_resample_close(s->avrctx);
-        for (i=0; i < AF_NCH; i++)
-            free(s->in[i]);
+        swr_free(&s->swrctx);
+        av_free(s->in[0]);
+        av_free(s->tmp[0]);
         free(s);
     }
 }
@@ -119,71 +134,36 @@
 static af_data_t* play(struct af_instance_s* af, af_data_t* data)
 {
   af_resample_t *s = af->setup;
-  int i, j, consumed, ret;
-  int16_t *in = (int16_t*)data->audio;
-  int16_t *out;
+  int ret;
+  int8_t *in = (int8_t*)data->audio;
+  int8_t *out;
   int chans   = data->nch;
-  int in_len  = data->len/(2*chans);
+  int in_len  = data->len;
   int out_len = in_len * af->mul + 10;
-  int16_t tmp[AF_NCH][out_len];
 
   if(AF_OK != RESIZE_LOCAL_BUFFER(af,data))
       return NULL;
 
-  out= (int16_t*)af->data->audio;
+  av_fast_malloc(&s->tmp[0], &s->tmp_alloc, FFALIGN(out_len,32));
+  if(s->tmp[0] == NULL) return NULL;
 
-  out_len= FFMIN(out_len, af->data->len/(2*chans));
+  out= (int8_t*)af->data->audio;
 
-  if(s->in_alloc < in_len + s->index){
-      s->in_alloc= in_len + s->index;
-      for(i=0; i<chans; i++){
-          s->in[i]= realloc(s->in[i], s->in_alloc*sizeof(int16_t));
-      }
-  }
+  out_len= FFMIN(out_len, af->data->len);
 
-  if(chans==1){
-      memcpy(&s->in[0][s->index], in, in_len * sizeof(int16_t));
-  }else if(chans==2){
-      for(j=0; j<in_len; j++){
-          s->in[0][j + s->index]= *(in++);
-          s->in[1][j + s->index]= *(in++);
-      }
-  }else{
-      for(j=0; j<in_len; j++){
-          for(i=0; i<chans; i++){
-              s->in[i][j + s->index]= *(in++);
-          }
-      }
-  }
-  in_len += s->index;
+  av_fast_malloc(&s->in[0], &s->in_alloc, FFALIGN(in_len,32));
+  if(s->in[0] == NULL) return NULL;
 
-  for(i=0; i<chans; i++){
-      ret= av_resample(s->avrctx, tmp[i], s->in[i], &consumed, in_len, out_len, i+1 == chans);
-  }
-  out_len= ret;
+  memcpy(s->in[0], in, in_len);
 
-  s->index= in_len - consumed;
-  for(i=0; i<chans; i++){
-      memmove(s->in[i], s->in[i] + consumed, s->index*sizeof(int16_t));
-  }
+  ret = swr_convert(s->swrctx, &s->tmp[0], out_len/chans/2, &s->in[0], in_len/chans/2);
+  if (ret < 0) return NULL;
+  out_len= ret*chans*2;
 
-  if(chans==1){
-      memcpy(out, tmp[0], out_len*sizeof(int16_t));
-  }else if(chans==2){
-      for(j=0; j<out_len; j++){
-          *(out++)= tmp[0][j];
-          *(out++)= tmp[1][j];
-      }
-  }else{
-      for(j=0; j<out_len; j++){
-          for(i=0; i<chans; i++){
-              *(out++)= tmp[i][j];
-          }
-      }
-  }
+  memcpy(out, s->tmp[0], out_len);
 
   data->audio = af->data->audio;
-  data->len   = out_len*chans*2;
+  data->len   = out_len;
   data->rate  = af->data->rate;
   return data;
 }
_______________________________________________
MPlayer-dev-eng mailing list
MPlayer-dev-eng@xxxxxxxxxxxx
https://lists.mplayerhq.hu/mailman/listinfo/mplayer-dev-eng