[FFmpeg-devel] [PATCH] swresample: Add swr_get_out_samples()

[wm4]

On Thu, 4 Jun 2015 04:37:31 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Wed, Jun 03, 2015 at 11:11:23AM +0200, wm4 wrote:
> > On Wed,  3 Jun 2015 01:22:49 +0200
> > Michael Niedermayer <michaelni at gmx.at> wrote:
> > 
> > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > ---
> > >  libswresample/resample.c            |   17 +++++++++++++++++
> > >  libswresample/swresample.c          |   30
> > > ++++++++++++++++++++++++++++++ libswresample/swresample.h
> > > |   14 ++++++++++++++ libswresample/swresample_internal.h |    2 ++
> > >  libswresample/version.h             |    2 +-
> > >  5 files changed, 64 insertions(+), 1 deletion(-)
> > > 
> > 
> > Well, it's interesting to see that the implementation is much more
> > complicated than what's suggested in the public API doxygen, or what
> > af_aresample.c does.
> 
> i dont know, it doesnt look much more complex to me, its handling
> compensation_distance, and does a bit of error checking
> 
> 
> > 
> > > diff --git a/libswresample/resample.c b/libswresample/resample.c
> > > index d4c7d06..c61bcb0 100644
> > > --- a/libswresample/resample.c
> > > +++ b/libswresample/resample.c
> > > @@ -345,6 +345,22 @@ static int64_t get_delay(struct SwrContext *s,
> > > int64_t base){ return av_rescale(num, base,
> > > s->in_sample_rate*(int64_t)c->src_incr << c->phase_shift); }
> > >  
> > > +static int64_t get_out_samples(struct SwrContext *s, int in_samples)
> > > {
> > > +    ResampleContext *c = s->resample;
> > > +    int64_t num = s->in_buffer_count + 2LL + in_samples;
> > > +    num *= 1 << c->phase_shift;
> > > +    num -= c->index;
> > > +    num = av_rescale_rnd(num, s->out_sample_rate,
> > > ((int64_t)s->in_sample_rate) << c->phase_shift, AV_ROUND_UP); +
> > > +    if (c->compensation_distance) {
> > > +        if (num > INT_MAX)
> > > +            return AVERROR(EINVAL);
> > > +
> > > +        num = FFMAX(num, (num * c->ideal_dst_incr - 1) / c->dst_incr
> > > + 1);
> > > +    }
> > > +    return num + 2;
> > 
> > What's the are the two "+2" for (here and at initialization of num)?
> > Maybe there should be a comment explaining them.
> 
> comment added

The comment you added to the final version had 2 typos.

> 
> > 
> > > +}
> > > +
> > >  static int resample_flush(struct SwrContext *s) {
> > >      AudioData *a= &s->in_buffer;
> > >      int i, j, ret;
> > > @@ -414,4 +430,5 @@ struct Resampler const swri_resampler={
> > >    set_compensation,
> > >    get_delay,
> > >    invert_initial_buffer,
> > > +  get_out_samples,
> > >  };
> > > diff --git a/libswresample/swresample.c b/libswresample/swresample.c
> > > index 3d3ab83..a5f5930 100644
> > > --- a/libswresample/swresample.c
> > > +++ b/libswresample/swresample.c
> > > @@ -672,11 +672,15 @@ int attribute_align_arg swr_convert(struct
> > > SwrContext *s, uint8_t *out_arg[SWR_C const uint8_t *in_arg
> > > [SWR_CH_MAX], int  in_count){ AudioData * in= &s->in;
> > >      AudioData *out= &s->out;
> > > +    int av_unused max_output;
> > >  
> > >      if (!swr_is_initialized(s)) {
> > >          av_log(s, AV_LOG_ERROR, "Context has not been
> > > initialized\n"); return AVERROR(EINVAL);
> > >      }
> > > +#if ASSERT_LEVEL >1
> > > +    max_output = swr_get_out_samples(s, in_count);
> > > +#endif
> > >  
> > >      while(s->drop_output > 0){
> > >          int ret;
> > > @@ -719,6 +723,9 @@ int attribute_align_arg swr_convert(struct
> > > SwrContext *s, uint8_t *out_arg[SWR_C int ret =
> > > swr_convert_internal(s, out, out_count, in, in_count); if(ret>0
> > > && !s->drop_output) s->outpts += ret * (int64_t)s->in_sample_rate;
> > > +
> > > +        av_assert2(max_output < 0 || ret < 0 || ret <= max_output);
> > > +
> > >          return ret;
> > >      }else{
> > >          AudioData tmp= *in;
> > > @@ -770,6 +777,7 @@ int attribute_align_arg swr_convert(struct
> > > SwrContext *s, uint8_t *out_arg[SWR_C }
> > >          if(ret2>0 && !s->drop_output)
> > >              s->outpts += ret2 * (int64_t)s->in_sample_rate;
> > > +        av_assert2(max_output < 0 || ret2 < 0 || ret2 <= max_output);
> > >          return ret2;
> > >      }
> > >  }
> > > @@ -821,6 +829,28 @@ int64_t swr_get_delay(struct SwrContext *s,
> > > int64_t base){ }
> > >  }
> > >  
> > > +int swr_get_out_samples(struct SwrContext *s, int in_samples)
> > > +{
> > > +    int64_t out_samples;
> > > +
> > > +    if (in_samples < 0)
> > > +        return AVERROR(EINVAL);
> > > +
> > > +    if (s->resampler && s->resample) {
> > > +        if (!s->resampler->get_out_samples)
> > > +            return AVERROR(ENOSYS);
> > 
> > When does this happen? Unless it's only before the context is
> > "opened", I'd say this is unacceptable. It should at least attempt to
> > return an upper bound. How else would the API user be able to tell how
> > much data to read?
> > 
> > (Unless you require the API user to do repeated resample calls with no
> > input to drain the remaining internal buffers?)
> 
> ENOSYS happens if you select the soxr resampler, ive not yet looked
> into if this is possible to know this based on API without depending
> on soxr internals.
> 
> either its possible to do then it should get implemented
> or its not in which case its just not possible if the user selects
> soxr
> 

This is a bit of a problem. Since soxr can be enabled via the generic
AVOptions, a user could easily break the API. Which is not good. (I
already had this issue with libswresample/soxr in a different case. I
didn't even know about it, until a user tried to use it.)