[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