[FFmpeg-devel] [PATCH] Implement av_samples_fill_arrays().

Michael Niedermayer michaelni
Fri Jan 28 21:13:30 CET 2011


On Fri, Jan 28, 2011 at 12:55:09AM +0100, Stefano Sabatini wrote:
> On date Saturday 2011-01-15 20:18:20 +0100, Michael Niedermayer encoded:
> > On Sat, Jan 15, 2011 at 07:24:01PM +0100, Stefano Sabatini wrote:
> > > On date Saturday 2011-01-15 14:24:35 +0100, Michael Niedermayer encoded:
> > > > On Sat, Jan 15, 2011 at 02:27:03AM +0100, Stefano Sabatini wrote:
> > > > > ---
> > > > >  libavcore/audioconvert.c |   14 ++++++++++++++
> > > > >  libavcore/audioconvert.h |   21 +++++++++++++++++++++
> > > > >  2 files changed, 35 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/libavcore/audioconvert.c b/libavcore/audioconvert.c
> > > > > index 1c6a511..7a46a3e 100644
> > > > > --- a/libavcore/audioconvert.c
> > > > > +++ b/libavcore/audioconvert.c
> > > > > @@ -166,3 +166,17 @@ int av_samples_fill_pointers(uint8_t *pointers[8], uint8_t *buf, int buf_size,
> > > > >  
> > > > >      return per_channel_size / sample_size;
> > > > >  }
> > > > > +
> > > > > +int av_samples_fill_arrays(uint8_t *data[8], int linesizes[8],
> > > > > +                           uint8_t *buf, int buf_size,
> > > > > +                           enum AVSampleFormat sample_fmt, int planar,
> > > > > +                           int64_t channel_layout, int nb_channels)
> > > > > +{
> > > > 
> > > > again channel_layout has nothing to do with what the code does
> > > 
> > > Of course, so now I wonder if libavcore/samples.h is a better place
> > > for this API.
> > > 
> > > Now it is:
> > > int av_samples_fill_linesizes(int linesizes[8], enum AVSampleFormat sample_fmt,
> > >                               int planar, int nb_channels);
> > > int av_samples_fill_pointers(uint8_t *pointers[8], uint8_t *buf, int buf_size,
> > >                              enum AVSampleFormat sample_fmt, int planar,
> > >                              int nb_channels);
> > > int av_samples_fill_arrays(uint8_t *data[8], int linesizes[8],
> > >                            uint8_t *buf, int buf_size,
> > >                            enum AVSampleFormat sample_fmt, int planar,
> > >                            int nb_channels);
> > > 
> > > This is useful for factorizing code in
> > > avfilter_default_get_audio_buffer() (see attached patch) and in
> > > avfilter_get_audio_buffer_ref_from_buffer().
> > > 
> > > I can eventually merge the implementation of the three functions in 
> > > av_samples_fill_arrays(), since currently there is not the need of two
> > > separate av_samples_fill_linesizes/pointers functions.
> > > 
> > > Please comment on this API.
> > 
> > I agree with avcore
> > what iam unsure about is linesize.
> > currently its all set equal, that is mightly useless
> > linesize[0] is as good as linesize[chan]
> > 
> > we could do something more usefull with linesize[1+]
> > like setting it so that
> > linesize[1] = data[1][0] - data[0][0]
> > 
> > the idea is that things could be addressed like
> > data[0] + time*linesize[0] +  chan*linesize[1]
> > the advantage of this over
> > data[chan] + time*linesize[0]
> > is that you need fewer variables (no data[0], data[1], data[2] ...) which can
> > help register starved architectures
> > 
> > 
> > also, if possible a single public function like:
> >  int av_samples_fill_arrays(uint8_t *data[8], int linesizes[8],
> >                             uint8_t *buf, int buf_size,
> >                             enum AVSampleFormat sample_fmt, int planar,
> >                             int nb_channels);
> >
> > that allows data & linesize to be NULL
> > would mean simpler public API thanb having 3 functions
> 
> Done, indeed it's much nicer.
> 
> As for the linesize stuff, I'm wondering if it is better to have
> something more similar to the imgutils stuff, that is to have linesize
> simply give the planar size (for planar) and the buffer size for
> packed (eventually aligned).
> 
> This would be useful for implementing:
> av_samples_copy
> av_samples_alloc
> 
> does it make sense?

you have 8 linesize values you set them all equal. you could store differnet
things in there instead of choosing which of 2 usefull values to duplicate 7
times


> 
> Check new patch.
> -- 
> FFmpeg = Fantastic and Fierce Majestic Picky Educated Governor

>  samplefmt.c |   33 +++++++++++++++++++++++++++++++++
>  samplefmt.h |   29 +++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 1b8852e158f3f1efa3811e230f94aa503c506e9c  0003-Implement-av_samples_fill_arrays.patch
> From 2d8bb08f6bea315948af9cbc6c37964e3ba54930 Mon Sep 17 00:00:00 2001
> From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> Date: Sat, 15 Jan 2011 00:00:00 +0100
> Subject: [PATCH] Implement av_samples_fill_arrays().
> 
> ---
>  libavcore/samplefmt.c |   33 +++++++++++++++++++++++++++++++++
>  libavcore/samplefmt.h |   29 +++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+), 0 deletions(-)
> 
> diff --git a/libavcore/samplefmt.c b/libavcore/samplefmt.c
> index 532acd9..d2a7c4f 100644
> --- a/libavcore/samplefmt.c
> +++ b/libavcore/samplefmt.c
> @@ -68,3 +68,36 @@ int av_get_bits_per_sample_fmt(enum AVSampleFormat sample_fmt)
>      return sample_fmt < 0 || sample_fmt >= AV_SAMPLE_FMT_NB ?
>          0 : sample_fmt_info[sample_fmt].bits;
>  }
> +
> +int av_samples_fill_arrays(uint8_t *pointers[8], int linesizes[8],
> +                           uint8_t *buf, int buf_size,
> +                           enum AVSampleFormat sample_fmt, int planar,
> +                           int nb_channels)
> +{
> +    int i, step_size = 0;
> +    int sample_size = av_get_bits_per_sample_fmt(sample_fmt) >>3;
> +    int per_channel_size;
> +
> +    per_channel_size = buf_size / nb_channels;
> +    if (pointers) {


> +        pointers[0] = buf;
> +        if (planar) {
> +            for (i = 1; i < nb_channels; i++) {
> +                step_size += per_channel_size;
> +                pointers[i] = buf + step_size;
> +            }
> +        } else { /* packed */
> +            for (i = 1; i < nb_channels; i++)
> +                pointers[i] = buf;
> +        }

for (i = 0; i < nb_channels; i++) {
    pointers[i] = buf + step_size;
    if (planar)
        step_size += per_channel_size;
}



> +        memset(&pointers[nb_channels], 0, (8-nb_channels) * sizeof(pointers[0]));
> +    }
> +
> +    if (linesizes) {
> +        for (i = 0; i < nb_channels; i++)
> +            linesizes[i] = planar ? sample_size : nb_channels * sample_size;
> +        memset(&linesizes[nb_channels], 0, (8-nb_channels) * sizeof(linesizes[0]));
> +    }
> +
> +    return per_channel_size / sample_size;
> +}
> diff --git a/libavcore/samplefmt.h b/libavcore/samplefmt.h
> index 9701efe..b1abe3d 100644
> --- a/libavcore/samplefmt.h
> +++ b/libavcore/samplefmt.h
> @@ -69,4 +69,33 @@ char *av_get_sample_fmt_string(char *buf, int buf_size, enum AVSampleFormat samp
>   */
>  int av_get_bits_per_sample_fmt(enum AVSampleFormat sample_fmt);
>  
> +/**
> + * Fill channel data pointers and linesizes for samples with sample
> + * format sample_fmt.
> + *

> + * The data array is filled with the pointers to the samples data:
> + * for planar, set the start point of each channel's data within the buffer,

> + * for packed, set the start point of the entire buffer only.

iam not sure if this is a good idea, why does it differ from planar?

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110128/636c2eea/attachment.pgp>



More information about the ffmpeg-devel mailing list