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

[wm4]

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