[FFmpeg-devel] [PATCH] avutil/imgutils: av_image_get_buffer_size(): do not insert padding between stride*height and palette

Stefano Sabatini stefasab at gmail.com
Sun Feb 14 14:00:24 CET 2016


On date Saturday 2016-02-13 23:08:50 +0100, wm4 encoded:
> On Sat, 13 Feb 2016 21:51:48 +0100
> Michael Niedermayer <michael at niedermayer.cc> wrote:
> 
> > On Sat, Feb 13, 2016 at 08:46:34PM +0100, wm4 wrote:
> > > On Sat, 13 Feb 2016 19:38:01 +0100
> > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > >   
> > > > This fixes the layout that is stored in pal8 nut with odd width*height
> > > > 
> > > > Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> > > > ---
> > > >  libavcodec/avpicture.c |    2 ++
> > > >  libavutil/imgutils.c   |    4 ++++
> > > >  2 files changed, 6 insertions(+)
> > > > 
> > > > diff --git a/libavcodec/avpicture.c b/libavcodec/avpicture.c
> > > > index 56435f4..cac9287 100644
> > > > --- a/libavcodec/avpicture.c
> > > > +++ b/libavcodec/avpicture.c
> > > > @@ -51,6 +51,8 @@ int avpicture_layout(const AVPicture* src, enum AVPixelFormat pix_fmt, int width
> > > >  
> > > >  int avpicture_get_size(enum AVPixelFormat pix_fmt, int width, int height)
> > > >  {
> > > > +    if (pix_fmt == AV_PIX_FMT_PAL8)
> > > > +        return FFALIGN(width*height, 4) + 1024;
> > > >      return av_image_get_buffer_size(pix_fmt, width, height, 1);
> > > >  }
> > > >  
> > > > diff --git a/libavutil/imgutils.c b/libavutil/imgutils.c
> > > > index adf6fdd..976bd23 100644
> > > > --- a/libavutil/imgutils.c
> > > > +++ b/libavutil/imgutils.c
> > > > @@ -372,6 +372,10 @@ int av_image_get_buffer_size(enum AVPixelFormat pix_fmt,
> > > >      if (desc->flags & AV_PIX_FMT_FLAG_PSEUDOPAL)
> > > >          return FFALIGN(width, align) * height;
> > > >  
> > > > +    // For paletted images we do not align the palette as in AVFrames
> > > > +    if (desc->flags & AV_PIX_FMT_FLAG_PAL)
> > > > +        return FFALIGN(width, align) * height + 1024;
> > > > +
> > > >      return av_image_fill_arrays(data, linesize, NULL, pix_fmt,
> > > >                                  width, height, align);
> > > >  }  
> > > 
> > > It seems wrong to litter the code with specific checks for an obscure
> > > pixel format, and change behavior, just to make these function's
> > > behavior line up with what NUT needs?  
> > 
> > I think my commit message was poorly worded
> > 
> > we need to either fix the code or fix/clarify the documentation
> > 
> > av_image_get_buffer_size() is documented as
> > 
> >  * Return the size in bytes of the amount of data required to store an
> >  * image with the given parameters.
> > 
> > it does not do this, after the patch it does.
> > 
> > the docs could be changed to something like:
> > 
> >  * Return the size in bytes of the amount of data required to store an
> >  * image with the given parameters. Except for PAL8 where the size is
> >  * rounded up to the next multiple of 4 even if the user asks for
> >  * less alignment
> > 
> > and likely there are other ways it could be documented
> 
> Why 4?
> 
> Also, why do av_image_get_buffer_size() and avpicture_get_size() do
> different things?

> I'd totally expect each line _and_ the start of the palette to be
> aligned to the requested slignment.

It's what I would expect as well. 

The plan was to drop the avpicture_ API I think, so we should not add
inconsistencies.

> > 
> > > 
> > > It's also possible that this is actually what the API user wants; there
> > > are many implications.  
> > 
> > yes, that is possible, should i add a av_image_get_buffer_size2() ?
> 
> Either that or a minor bump and a warning in APIchanges, depending on
> how this plays out.
> 
> > but iam not sure what use av_image_get_buffer_size() has then
> > in its current form
> > its not the AVFrame required size and its not the storage size in
> > any container (for all pixel formats) i think, either would have
> > exceptions
> 
> Maybe one could argue that it shouldn't include the palette? It's kind
> of a special case, and many uses do not store the palette directly along
> with the image data, but somewhere else.
> 
> > 
> > > 
> > > Also, is this consistent with av_image_fill_arrays etc.?  
> > 
> > no, and that also was not teh case before the patch. for example
> > av_image_fill_arrays() sets pointers up for pseudopal formats
> > that would be outside the array space given from
> > av_image_get_buffer_size()
> 
> I thought these functions build on each other: one to get the buffer
> size, and another one to get the offsets into the buffer. Does it make
> sense that they are not consistent?
> 
> > 

> > maybe stefano could comment as i think he wrote a lot of this

The av_image_* API was a port of the avpicture_* API, trying to remove
special cases of the imgconvert code and relying on the generic
pixdesc struct information.

It simplified the code (no need to update N places when a new format
was added) and allowed to specify the alignment value, which is
assumed for each line (and by extension for the palette, since it
resides in the data array).

Now, should we consider the palette buffer part of the buffer used to
store the image? (I guess the answer is yes in this case).
-- 
FFmpeg = Funny and Fundamental Mournful Perfectionist Exploitable Gadget


More information about the ffmpeg-devel mailing list