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

Michael Niedermayer michael at niedermayer.cc
Sun Feb 14 16:31:05 CET 2016


On Sun, Feb 14, 2016 at 02:00:24PM +0100, Stefano Sabatini wrote:
> 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. 

Ive posted a patch that does this


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

the align expectation you wrote above also affects avpicture_*
the fix for it for the new API also changes the behavior of
avpicture_* as it uses the new API
If we want to fix any bugs in the new API we either have to accept
that the avpicture_* code will also slightly change in behavior or
we would need special cases to keep it behaving the same

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

Everything should be made as simple as possible, but not simpler.
-- Albert Einstein
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160214/99e734bf/attachment.sig>


More information about the ffmpeg-devel mailing list