[FFmpeg-devel] [PATCH] convert to S16 when resampling is requested

[Michael Niedermayer]

On Mon, Feb 09, 2009 at 11:12:13AM -0800, Baptiste Coudurier wrote:
> Michael Niedermayer wrote:
> > On Sun, Feb 08, 2009 at 05:15:14PM -0800, Baptiste Coudurier wrote:
> >> Michael Niedermayer wrote:
> >>> [...]
> >>>
> >>>> 3rd try.
> >>> [...]
> >>>> @@ -126,8 +132,10 @@
> >>>>      }
> >>>>  }
> >>>>  
> >>>> -ReSampleContext *audio_resample_init(int output_channels, int input_channels,
> >>>> -                                      int output_rate, int input_rate)
> >>>> +ReSampleContext *audio_resample_init2(int output_channels, int input_channels,
> >>> this needs a ff or av prefix
> >>> also there should be a way for the user to set/pass the other params
> >>> to the resampler namely 
> >>> int filter_size, int phase_shift, int linear, double cutoff
> >>> when we already change the API ...
> >> Good idea, updated.
> >>
> >>> [...]
> >>>> +            av_log(NULL, AV_LOG_ERROR,
> >>>> +                   "Cannot convert %s sample format to s16 sample format\n",
> >>>> +                   avcodec_get_sample_fmt_name(s->sample_fmt[0]));
> >>> av_log() should have some context different from NULL otherwise
> >>> it may become hard in some cases to tell from where a message came
> >>>
> >> Ok, separate patch attached, which can be applied first.
> > 
> > are there 2 patches supposed to be attached? because there is just 1
> > 
> 
> The idea was to seperate the context add and the actual conversion code.
> I will seperate commits.
> 
> > 
> >> Index: libavcodec/resample.c
> >> ===================================================================
> >> --- libavcodec/resample.c	(revision 17085)
> >> +++ libavcodec/resample.c	(working copy)
> >> @@ -25,16 +25,32 @@
> >>   */
> >>  
> >>  #include "avcodec.h"
> >> +#include "audioconvert.h"
> >> +#include "opt.h"
> >>  
> >>  struct AVResampleContext;
> >>  
> > 
> >> +static const const char *context_to_name(void *ptr)
> > 
> > const const?
> 
> Fixed.
> 
> > [...]
> >> @@ -153,6 +173,35 @@
> >>      if (s->output_channels < s->filter_channels)
> >>          s->filter_channels = s->output_channels;
> >>  
> >> +    s->sample_fmt [0] = sample_fmt_in;
> >> +    s->sample_fmt [1] = sample_fmt_out;
> > 
> >> +    s->sample_size[0] = av_get_bits_per_sample_format(s->sample_fmt[0])/8;
> >> +    s->sample_size[1] = av_get_bits_per_sample_format(s->sample_fmt[1])/8;
> > 
> >>> 3
> 
> Sure ? This is initialization. Changed it nonetheless.
> 
> >> +
> >> +    if (s->sample_fmt[0] != SAMPLE_FMT_S16) {
> >> +        av_audio_convert_free(s->convert_ctx[0]);
> >> +        if (!(s->convert_ctx[0] = av_audio_convert_alloc(SAMPLE_FMT_S16, 1,
> >> +                                                         s->sample_fmt[0], 1, NULL, 0))) {
> >> +            av_log(NULL, AV_LOG_ERROR,
> >> +                   "Cannot convert %s sample format to s16 sample format\n",
> >> +                   avcodec_get_sample_fmt_name(s->sample_fmt[0]));
> >> +            av_free(s);
> >> +            return NULL;
> >> +        }
> >> +    }
> >> +
> >> +    if (s->sample_fmt[1] != SAMPLE_FMT_S16) {
> >> +        av_audio_convert_free(s->convert_ctx[1]);
> >> +        if (!(s->convert_ctx[1] = av_audio_convert_alloc(s->sample_fmt[1], 1,
> >> +                                                         SAMPLE_FMT_S16, 1, NULL, 0))) {
> >> +            av_log(NULL, AV_LOG_ERROR,
> >> +                   "Cannot convert s16 sample format to %s sample format\n",
> >> +                   avcodec_get_sample_fmt_name(s->sample_fmt[1]));
> >> +            av_free(s);
> >> +            return NULL;
> > 
> > can leak s->convert_ctx[0]
> 
> Fixed.

[...]
> @@ -153,6 +173,36 @@
>      if (s->output_channels < s->filter_channels)
>          s->filter_channels = s->output_channels;
>  
> +    s->sample_fmt [0] = sample_fmt_in;
> +    s->sample_fmt [1] = sample_fmt_out;
> +    s->sample_size[0] = av_get_bits_per_sample_format(s->sample_fmt[0])>>3;
> +    s->sample_size[1] = av_get_bits_per_sample_format(s->sample_fmt[1])>>3;
> +

> +    if (s->sample_fmt[0] != SAMPLE_FMT_S16) {
> +        av_audio_convert_free(s->convert_ctx[0]);
> +        if (!(s->convert_ctx[0] = av_audio_convert_alloc(SAMPLE_FMT_S16, 1,
> +                                                         s->sample_fmt[0], 1, NULL, 0))) {
> +            av_log(s, AV_LOG_ERROR,
> +                   "Cannot convert %s sample format to s16 sample format\n",
> +                   avcodec_get_sample_fmt_name(s->sample_fmt[0]));
> +            av_free(s);
> +            return NULL;

considering that there is
av_audio_convert_free(s->convert_ctx[1]);
below i wonder if it could be leaking here


> +        }
> +    }
> +
> +    if (s->sample_fmt[1] != SAMPLE_FMT_S16) {
> +        av_audio_convert_free(s->convert_ctx[1]);
> +        if (!(s->convert_ctx[1] = av_audio_convert_alloc(s->sample_fmt[1], 1,
> +                                                         SAMPLE_FMT_S16, 1, NULL, 0))) {
> +            av_log(s, AV_LOG_ERROR,
> +                   "Cannot convert s16 sample format to %s sample format\n",
> +                   avcodec_get_sample_fmt_name(s->sample_fmt[1]));

> +            av_free(s->convert_ctx[0]);

you use av_audio_convert_free() for it and av_free() here

the patch looks ok except these.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090209/aaf3b347/attachment.pgp>