[FFmpeg-devel] [PATCH 7/7] Re-implement avpicture_layout() using pixdesc and imgutils API.

[Stefano Sabatini]

On date Monday 2010-11-15 14:39:56 +0100, Michael Niedermayer encoded:
> On Mon, Nov 15, 2010 at 12:40:43PM +0100, Stefano Sabatini wrote:
> > On date Tuesday 2010-11-09 19:31:16 +0100, Michael Niedermayer encoded:
> > > On Sun, Nov 07, 2010 at 11:07:30PM +0100, Stefano Sabatini wrote:
> > > > The new implementation is more compact, more correct and doesn't hurt
> > > > the eyes.
> > > > ---
> > > >  libavcodec/imgconvert.c |   65 ++++++++++------------------------------------
> > > >  1 files changed, 14 insertions(+), 51 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/imgconvert.c b/libavcodec/imgconvert.c
> > > > index df2a90e..6fba703 100644
> > > > --- a/libavcodec/imgconvert.c
> > > > +++ b/libavcodec/imgconvert.c
> > > > @@ -490,68 +490,31 @@ int avpicture_fill(AVPicture *picture, uint8_t *ptr,
> > > >  int avpicture_layout(const AVPicture* src, enum PixelFormat pix_fmt, int width, int height,
> > > >                       unsigned char *dest, int dest_size)
> > > >  {
> > > > -    const PixFmtInfo* pf = &pix_fmt_info[pix_fmt];
> > > > +    int i, j, nb_planes = 0, linesizes[4];
> > > >      const AVPixFmtDescriptor *desc = &av_pix_fmt_descriptors[pix_fmt];
> > > > -    int i, j, w, ow, h, oh, data_planes;
> > > > -    const unsigned char* s;
> > > >      int size = avpicture_get_size(pix_fmt, width, height);
> > > >  
> > > >      if (size > dest_size || size < 0)
> > > > -        return -1;
> > > > -
> > > > -    if (pf->pixel_type == FF_PIXEL_PACKED || pf->pixel_type == FF_PIXEL_PALETTE) {
> > > > -        if (pix_fmt == PIX_FMT_YUYV422 ||
> > > > -            pix_fmt == PIX_FMT_UYVY422 ||
> > > > -            pix_fmt == PIX_FMT_BGR565BE ||
> > > > -            pix_fmt == PIX_FMT_BGR565LE ||
> > > > -            pix_fmt == PIX_FMT_BGR555BE ||
> > > > -            pix_fmt == PIX_FMT_BGR555LE ||
> > > > -            pix_fmt == PIX_FMT_BGR444BE ||
> > > > -            pix_fmt == PIX_FMT_BGR444LE ||
> > > > -            pix_fmt == PIX_FMT_RGB565BE ||
> > > > -            pix_fmt == PIX_FMT_RGB565LE ||
> > > > -            pix_fmt == PIX_FMT_RGB555BE ||
> > > > -            pix_fmt == PIX_FMT_RGB555LE ||
> > > > -            pix_fmt == PIX_FMT_RGB444BE ||
> > > > -            pix_fmt == PIX_FMT_RGB444LE)
> > > > -            w = width * 2;
> > > > -        else if (pix_fmt == PIX_FMT_UYYVYY411)
> > > > -            w = width + width/2;
> > > > -        else if (pix_fmt == PIX_FMT_PAL8)
> > > > -            w = width;
> > > > -        else
> > > > -            w = width * (pf->depth * pf->nb_channels / 8);
> > > > +        return AVERROR(EINVAL);
> > > >  
> > > > -        data_planes = 1;
> > > > -        h = height;
> > > > -    } else {
> > > > -        data_planes = pf->nb_channels;
> > > > -        w = (width*pf->depth + 7)/8;
> > > > -        h = height;
> > > > -    }
> > > > +    for (i = 0; i < desc->nb_components; i++)
> > > > +        nb_planes = FFMAX(desc->comp[i].plane, nb_planes);
> > > > +    nb_planes++;
> > > >  
> > > > -    ow = w;
> > > > -    oh = h;
> > > > +    av_image_fill_linesizes(linesizes, pix_fmt, width);
> > > > +    for (i = 0; i < nb_planes; i++) {
> > > > +        int h, s = (i == 1 || i == 2) ? desc->log2_chroma_h : 0;
> > > > +        h = (height + (1 << s) - 1) >> s;
> > > >  
> > > > -    for (i=0; i<data_planes; i++) {
> > > > -        if (i == 1) {
> > > > -            w = (- ((-width) >> desc->log2_chroma_w) * pf->depth + 7) / 8;
> > > > -            h = -((-height) >> desc->log2_chroma_h);
> > > > -            if (pix_fmt == PIX_FMT_NV12 || pix_fmt == PIX_FMT_NV21)
> > > > -                w <<= 1;
> > > > -        } else if (i == 3) {
> > > > -            w = ow;
> > > > -            h = oh;
> > > > -        }
> > > >          s = src->data[i];
> > > > -        for(j=0; j<h; j++) {
> > > > -            memcpy(dest, s, w);
> > > > -            dest += w;
> > > > +        for (j = 0; j < h; j++) {
> > > > +            memcpy(dest, s, linesizes[i]);
> > > 
> > > if linesize > w then this copies more than is defined in the input i think
> > 
> > w = linesizes[i]
> > 
> > that is the linesizes[i] computed by av_fill_image_linesizes() is
> > (almost) the same computed by the old code, but the new code is more
> > complete and more correct (e.g. with yuyv, uyvy, uyyvyy411, nv12,
> > nv21).
> 
> you copy linesize bytes, you must only copy width*size of sample bytes.

I don't know what you mean, this function computes the linesizes for
dest and these new sizes are supposed to be minimal (while the
AVPicture in src could contain longer linesizes, but that doesn't
matter as the linesizes[] are computed for the *new* frame in dest).

That's also the behavior of the old code.

> > 
> > See result of the attached test code:
> 
> could you show a diff of this or limit output to changed cases?
> its practically unreadable as is

Attached.

> also the code affects the rawvideo decoder and encoders bitstream possibly
> this needs to be tested

Regression test passed, if you suggest a more *specific* test I can
give it a try.
-- 
FFmpeg = Fast & Faithless Mournful Powered Excellent Glue