Re: [libav-devel] [PATCH 3/5] frame: allow align=0 (meaning automatic) for av_frame_get_buffer()

[Anton Khirnov]

Quoting Vittorio Giovara (2017-02-08 14:40:45)
> On Wed, Feb 8, 2017 at 5:50 AM, Anton Khirnov  wrote:
> > This will avoid every caller from hardcoding some specific alignment,
> > which may break in the future with new instruction sets.
> > ---
> >  doc/APIchanges  | 4 
> >  libavutil/frame.c   | 4 
> >  libavutil/frame.h   | 4 +++-
> >  libavutil/version.h | 2 +-
> >  4 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index acd1536..fd751f3 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -13,6 +13,10 @@ libavutil: 2015-08-28
> >
> >  API changes, most recent first:
> >
> > +2017-02-xx - xxx - lavu 55.31.1 - frame.h
> > +  Allow passing the value of 0 (meaning "automatic") as the required 
> > alignment
> > +  to av_frame_get_buffer().
> > +
> >  2017-02-xx - xxx - lavu 55.31.0 - cpu.h
> >Add av_cpu_max_align() for querying maximum required data alignment.
> >
> > diff --git a/libavutil/frame.c b/libavutil/frame.c
> > index aafaa57..aa5820c 100644
> > --- a/libavutil/frame.c
> > +++ b/libavutil/frame.c
> > @@ -19,6 +19,7 @@
> >  #include "channel_layout.h"
> >  #include "buffer.h"
> >  #include "common.h"
> > +#include "cpu.h"
> >  #include "dict.h"
> >  #include "frame.h"
> >  #include "imgutils.h"
> > @@ -103,6 +104,9 @@ static int get_video_buffer(AVFrame *frame, int align)
> >  if (ret < 0)
> >  return ret;
> >
> > +if (align <= 0)
> > +align = av_cpu_max_align();
> 
> should you maybe be stricter here and check for == 0, failing for negative?

More code for little gain IMO.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/5] frame: allow align=0 (meaning automatic) for av_frame_get_buffer()

[Vittorio Giovara]

On Wed, Feb 8, 2017 at 5:50 AM, Anton Khirnov  wrote:
> This will avoid every caller from hardcoding some specific alignment,
> which may break in the future with new instruction sets.
> ---
>  doc/APIchanges  | 4 
>  libavutil/frame.c   | 4 
>  libavutil/frame.h   | 4 +++-
>  libavutil/version.h | 2 +-
>  4 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index acd1536..fd751f3 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -13,6 +13,10 @@ libavutil: 2015-08-28
>
>  API changes, most recent first:
>
> +2017-02-xx - xxx - lavu 55.31.1 - frame.h
> +  Allow passing the value of 0 (meaning "automatic") as the required 
> alignment
> +  to av_frame_get_buffer().
> +
>  2017-02-xx - xxx - lavu 55.31.0 - cpu.h
>Add av_cpu_max_align() for querying maximum required data alignment.
>
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index aafaa57..aa5820c 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -19,6 +19,7 @@
>  #include "channel_layout.h"
>  #include "buffer.h"
>  #include "common.h"
> +#include "cpu.h"
>  #include "dict.h"
>  #include "frame.h"
>  #include "imgutils.h"
> @@ -103,6 +104,9 @@ static int get_video_buffer(AVFrame *frame, int align)
>  if (ret < 0)
>  return ret;
>
> +if (align <= 0)
> +align = av_cpu_max_align();

should you maybe be stricter here and check for == 0, failing for negative?
-- 
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/5] frame: allow align=0 (meaning automatic) for av_frame_get_buffer()

[wm4]

On Wed, 08 Feb 2017 12:22:24 +0100
Anton Khirnov  wrote:

> Quoting wm4 (2017-02-08 12:08:18)
> > On Wed,  8 Feb 2017 11:50:58 +0100
> > Anton Khirnov  wrote:
> >   
> > > This will avoid every caller from hardcoding some specific alignment,
> > > which may break in the future with new instruction sets.
> > > ---  
> >   
> > >   * @param frame frame in which to store the new buffers.
> > > - * @param align required buffer size alignment
> > > + * @param align Required buffer size alignment. If equal to 0, alignment 
> > > will be
> > > + *  chosen automatically for the current CPU. It is highly
> > > + *  recommended to pass 0 here unless you know what you are 
> > > doing.
> > >   *
> > >   * @return 0 on success, a negative AVERROR on error.
> > >   */  
> > 
> > This implicitly changes the meaning of "0" from no alignment
> > (equivalent to 1) to some random alignment. I don't think that's very
> > safe. Why not make -1 the magic value that enables automatic alignment?  
> 
> 0 is not "no alignment", 0 is invalid for video and I think will do
> something random right now. For audio, 0 already means automatic. So I'm
> not changing the behaviour of any valid code.
> 

That's weird, but fine then I guess.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/5] frame: allow align=0 (meaning automatic) for av_frame_get_buffer()

[Anton Khirnov]

Quoting wm4 (2017-02-08 12:08:18)
> On Wed,  8 Feb 2017 11:50:58 +0100
> Anton Khirnov  wrote:
> 
> > This will avoid every caller from hardcoding some specific alignment,
> > which may break in the future with new instruction sets.
> > ---
> 
> >   * @param frame frame in which to store the new buffers.
> > - * @param align required buffer size alignment
> > + * @param align Required buffer size alignment. If equal to 0, alignment 
> > will be
> > + *  chosen automatically for the current CPU. It is highly
> > + *  recommended to pass 0 here unless you know what you are 
> > doing.
> >   *
> >   * @return 0 on success, a negative AVERROR on error.
> >   */
> 
> This implicitly changes the meaning of "0" from no alignment
> (equivalent to 1) to some random alignment. I don't think that's very
> safe. Why not make -1 the magic value that enables automatic alignment?

0 is not "no alignment", 0 is invalid for video and I think will do
something random right now. For audio, 0 already means automatic. So I'm
not changing the behaviour of any valid code.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/5] frame: allow align=0 (meaning automatic) for av_frame_get_buffer()

[wm4]

On Wed,  8 Feb 2017 11:50:58 +0100
Anton Khirnov  wrote:

> This will avoid every caller from hardcoding some specific alignment,
> which may break in the future with new instruction sets.
> ---

>   * @param frame frame in which to store the new buffers.
> - * @param align required buffer size alignment
> + * @param align Required buffer size alignment. If equal to 0, alignment 
> will be
> + *  chosen automatically for the current CPU. It is highly
> + *  recommended to pass 0 here unless you know what you are 
> doing.
>   *
>   * @return 0 on success, a negative AVERROR on error.
>   */

This implicitly changes the meaning of "0" from no alignment
(equivalent to 1) to some random alignment. I don't think that's very
safe. Why not make -1 the magic value that enables automatic alignment?
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel