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

2016-02-14 Thread Michael Niedermayer 
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  wrote:
> > 
> > > On Sat, Feb 13, 2016 at 08:46:34PM +0100, wm4 wrote:
> > > > On Sat, 13 Feb 2016 19:38:01 +0100
> > > > Michael Niedermayer  wrote:
> > > >   
> > > > > This fixes the layout that is stored in pal8 nut with odd width*height
> > > > > 
> > > > > Signed-off-by: Michael Niedermayer 
> > > > > ---
> > > > >  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


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2016-02-14 Thread Stefano Sabatini 
On date Saturday 2016-02-13 23:08:50 +0100, wm4 encoded:
> On Sat, 13 Feb 2016 21:51:48 +0100
> Michael Niedermayer  wrote:
> 
> > On Sat, Feb 13, 2016 at 08:46:34PM +0100, wm4 wrote:
> > > On Sat, 13 Feb 2016 19:38:01 +0100
> > > Michael Niedermayer  wrote:
> > >   
> > > > This fixes the layout that is stored in pal8 nut with odd width*height
> > > > 
> > > > Signed-off-by: Michael Niedermayer 
> > > > ---
> > > >  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

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

2016-02-13 Thread Michael Niedermayer 
This fixes the layout that is stored in pal8 nut with odd width*height

Signed-off-by: Michael Niedermayer 
---
 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);
 }
-- 
1.7.9.5

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2016-02-13 Thread wm4 
On Sat, 13 Feb 2016 19:38:01 +0100
Michael Niedermayer  wrote:

> This fixes the layout that is stored in pal8 nut with odd width*height
> 
> Signed-off-by: Michael Niedermayer 
> ---
>  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?

It's also possible that this is actually what the API user wants; there
are many implications.

Also, is this consistent with av_image_fill_arrays etc.?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2016-02-13 Thread Michael Niedermayer 
On Sat, Feb 13, 2016 at 08:46:34PM +0100, wm4 wrote:
> On Sat, 13 Feb 2016 19:38:01 +0100
> Michael Niedermayer  wrote:
> 
> > This fixes the layout that is stored in pal8 nut with odd width*height
> > 
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  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


> 
> 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() ?
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

> 
> 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()

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

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2016-02-13 Thread wm4 
On Sat, 13 Feb 2016 21:51:48 +0100
Michael Niedermayer  wrote:

> On Sat, Feb 13, 2016 at 08:46:34PM +0100, wm4 wrote:
> > On Sat, 13 Feb 2016 19:38:01 +0100
> > Michael Niedermayer  wrote:
> >   
> > > This fixes the layout that is stored in pal8 nut with odd width*height
> > > 
> > > Signed-off-by: Michael Niedermayer 
> > > ---
> > >  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
> 
> [...]
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel