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

[Michael Niedermayer]

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.


> 
> 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
also the code affects the rawvideo decoder and encoders bitstream possibly
this needs to be tested

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20101115/78638639/attachment.pgp>