On Thu, 4 Jun 2015 04:37:31 +0200
Michael Niedermayer <michae...@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 <michae...@gmx.at> wrote:
> >
> > > Signed-off-by: Michael Niedermayer <michae...@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.)
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
