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

Michael Niedermayer michaelni
Fri Nov 19 02:14:16 CET 2010


On Fri, Nov 19, 2010 at 01:14:24AM +0100, Stefano Sabatini wrote:
> 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).

The issue iam seeing is that the source picture contains something between
width and linesize and you copy this to destination.
Now while i do not have a concrete example of where this could be a real issue
it could in principle be part of a exploit leaking information of what is
displayed in the column to the right of the picture. If this is a problem i
dont know but it does not feel correct to me.
If fixing this slows it down then i dont mind us leaving it as is as long as
noone comes up with a real issue ...


> 
> That's also the behavior of the old code.

I know


> 
> > > 
> > > 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.

looks ok to me

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

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates
-------------- 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/20101119/58cd17fa/attachment.pgp>



More information about the ffmpeg-devel mailing list