Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Le nonidi 29 floréal, an CCXXV, Muhammad Faiz a écrit :
> But that is what av_frame_get_buffer does (and maybe ff_get_buffer too).

Maybe it does that, but I do not think it is an alignment issue, and I
am not entirely convinced this is intentional.

Anyway, I have wasted enough of my time on this issue.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Muhammad Faiz]

On Thu, May 18, 2017 at 5:30 PM, Nicolas George  wrote:
> Le nonidi 29 floréal, an CCXXV, Muhammad Faiz a écrit :
>> I mean for rgb24 format, when alignment is 32, linesize should be
>> multiple of 96 (32 is not enough).
>
> I think you are wrong. I do not know any alignment requirement that is
> not a power of 2.

But that is what av_frame_get_buffer does (and maybe ff_get_buffer too).

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Le nonidi 29 floréal, an CCXXV, Muhammad Faiz a écrit :
> I mean for rgb24 format, when alignment is 32, linesize should be
> multiple of 96 (32 is not enough).

I think you are wrong. I do not know any alignment requirement that is
not a power of 2.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Muhammad Faiz]

On Thu, May 18, 2017 at 5:20 PM, Nicolas George  wrote:
> Le nonidi 29 floréal, an CCXXV, Muhammad Faiz a écrit :
>> Missing linesize check.
>
> No, check again.

Oh, I'm sorry. My fault.

>
>> I tested the behaviour of av_frame_get_buffer(frame, 32) and check
>> linesize[0], here the result of some different pixel formats:
>> yuv420p: 32
>> rgb24: 96
>> rgba: 32
>>
>> So linesize constraint acutually depends on pixel format.
>
> No, check again.

I mean for rgb24 format, when alignment is 32, linesize should be
multiple of 96 (32 is not enough).

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Le nonidi 29 floréal, an CCXXV, Hendrik Leppkes a écrit :
> We could put the check in another central place

Yes.

> This is not bikeshedding, its an actual concern.

Well, you win, I drop the baby, catch it or not, I do not care.

But I still oppose to bad fixed being committed in areas of code I am
working on.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Hendrik Leppkes]

On Thu, May 18, 2017 at 11:16 AM, Nicolas George  wrote:
> Le nonidi 29 floréal, an CCXXV, Hendrik Leppkes a écrit :
>> I think its a saner choice to design the API to try to avoid instant
>> heap corruption, instead of hoping every case sets the alignment
>> properly in all cases.
>> A single if condition shouldn't be too much trouble, we already flag
>> hardware pixel formats as such in the pixdesc to identify them.
>
> Giving an incorrect alignment value to this function will not cause an
> "instant heap corruption", you are burning a straw man.

If as a result of this function returning 0 some other code tries to
memcpy some opaque hardware pointer, there is a good chance of
something crazy happening, so its hardly a strawman.
We could put the check in another central place, but IMHO it needs to
be somewhere central, and not hope that every filter and encoder
remembers to override alignment in the hardware surface input case.

>
> This function performs a simple task: it checks the alignment of a
> frame, given as a parameter, against a required alignment value, given
> as a parameter. Simple, straightforward, predictable.
>
> Adding special cases to this function would break these properties.

This simple task relies on various properties of the input frame -
which in the hardware case are just not given, so its essentially
performing an undefined operation.

>
> Also, I am roughly one bikeshedding mail away from losing interest in
> this whole matter.
>

This is not bikeshedding, its an actual concern.

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Le nonidi 29 floréal, an CCXXV, Muhammad Faiz a écrit :
> Missing linesize check.

No, check again.

> I tested the behaviour of av_frame_get_buffer(frame, 32) and check
> linesize[0], here the result of some different pixel formats:
> yuv420p: 32
> rgb24: 96
> rgba: 32
> 
> So linesize constraint acutually depends on pixel format.

No, check again.

Regards,

-- 
  Nicola George


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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Muhammad Faiz]

On Thu, May 18, 2017 at 3:11 PM, Nicolas George  wrote:
> Signed-off-by: Nicolas George 
> ---
>  doc/APIchanges|  3 +++
>  libavutil/frame.c | 17 +
>  libavutil/frame.h |  8 
>  3 files changed, 28 insertions(+)
>
>
> With the linesize check and without the 1<<.
>
>
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 67a6142401..6d3b573c2d 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil: 2015-08-28
>
>  API changes, most recent first:
>
> +2017-05-18 - xx - lavu 55.63.100 - frame.h
> +  Add av_frame_check_align().
> +
>  2017-05-15 - xx - lavc 57.96.100 - avcodec.h
>VideoToolbox hardware-accelerated decoding now supports the new hwaccel 
> API,
>which can create the decoder context and allocate hardware frames 
> automatically.
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 24d5d5f184..aed3cd04ec 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -781,3 +781,20 @@ const char *av_frame_side_data_name(enum 
> AVFrameSideDataType type)
>  }
>  return NULL;
>  }
> +
> +int av_frame_check_align(const AVFrame *frame, unsigned align)
> +{
> +unsigned mask = align - 1;
> +unsigned i;
> +
> +for (i = 0; i < AV_NUM_DATA_POINTERS; i++)
> +if (((intptr_t)frame->data[i] & mask) ||
> +(frame->linesize[i] & mask))
> +return 0;
> +if (!frame->extended_data || frame->extended_data == frame->data)
> +return 1;
> +for (i = AV_NUM_DATA_POINTERS; i < frame->channels; i++)
> +if (((intptr_t)frame->extended_data[i] & mask))
> +return 0;

Missing linesize check.


> +return 1;
> +}
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 26261d7e40..1cbf7c7a5a 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -772,6 +772,14 @@ void av_frame_remove_side_data(AVFrame *frame, enum 
> AVFrameSideDataType type);
>  const char *av_frame_side_data_name(enum AVFrameSideDataType type);
>
>  /**
> + * Check if the data pointers of a frame are aligned enough.
> + * Test if all frame data pointers have the alignment lower bits cleared,
> + * i.e. are a multiple of alignment.
> + * @return  >0 if aligned, 0 if not
> + */
> +int av_frame_check_align(const AVFrame *frame, unsigned align);
> +
> +/**
>   * @}
>   */
>
> --
> 2.11.0
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

I tested the behaviour of av_frame_get_buffer(frame, 32) and check
linesize[0], here the result of some different pixel formats:
yuv420p: 32
rgb24: 96
rgba: 32

So linesize constraint acutually depends on pixel format.

IMHO, because the fact of crop filter, actually (probably all) video
filters and codecs accept unaligned data pointer but (some of them)
require aligned linesize.

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Le nonidi 29 floréal, an CCXXV, Hendrik Leppkes a écrit :
> I think its a saner choice to design the API to try to avoid instant
> heap corruption, instead of hoping every case sets the alignment
> properly in all cases.
> A single if condition shouldn't be too much trouble, we already flag
> hardware pixel formats as such in the pixdesc to identify them.

Giving an incorrect alignment value to this function will not cause an
"instant heap corruption", you are burning a straw man.

This function performs a simple task: it checks the alignment of a
frame, given as a parameter, against a required alignment value, given
as a parameter. Simple, straightforward, predictable.

Adding special cases to this function would break these properties.

Also, I am roughly one bikeshedding mail away from losing interest in
this whole matter.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Hendrik Leppkes]

On Thu, May 18, 2017 at 10:44 AM, Nicolas George  wrote:
> Le nonidi 29 floréal, an CCXXV, Hendrik Leppkes a écrit :
>> I wonder if there should be an exception in here somewhere for
>> hardware pixelformats, there is no reason to assume the hardware
>> pointers passed in AVFrame would be aligned, and aligning them would
>> cause all sorts of crashes.
>> Best case from an API standpoint would be if av_frame_check_align can
>> just claim hardware frames are aligned, so users don't have to
>> replicate that check.
>
> Possible, but I think setting the correct value for alignment is the
> best way of dealing with this kind of issue.
>

I think its a saner choice to design the API to try to avoid instant
heap corruption, instead of hoping every case sets the alignment
properly in all cases.
A single if condition shouldn't be too much trouble, we already flag
hardware pixel formats as such in the pixdesc to identify them.

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Le nonidi 29 floréal, an CCXXV, Hendrik Leppkes a écrit :
> I wonder if there should be an exception in here somewhere for
> hardware pixelformats, there is no reason to assume the hardware
> pointers passed in AVFrame would be aligned, and aligning them would
> cause all sorts of crashes.
> Best case from an API standpoint would be if av_frame_check_align can
> just claim hardware frames are aligned, so users don't have to
> replicate that check.

Possible, but I think setting the correct value for alignment is the
best way of dealing with this kind of issue.

Regards,

-- 
  Nicolas George


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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Hendrik Leppkes]

On Thu, May 18, 2017 at 10:11 AM, Nicolas George  wrote:
> Signed-off-by: Nicolas George 
> ---
>  doc/APIchanges|  3 +++
>  libavutil/frame.c | 17 +
>  libavutil/frame.h |  8 
>  3 files changed, 28 insertions(+)
>
>

I wonder if there should be an exception in here somewhere for
hardware pixelformats, there is no reason to assume the hardware
pointers passed in AVFrame would be aligned, and aligning them would
cause all sorts of crashes.
Best case from an API standpoint would be if av_frame_check_align can
just claim hardware frames are aligned, so users don't have to
replicate that check.

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

[FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Signed-off-by: Nicolas George 
---
 doc/APIchanges|  3 +++
 libavutil/frame.c | 17 +
 libavutil/frame.h |  8 
 3 files changed, 28 insertions(+)


With the linesize check and without the 1<<.


diff --git a/doc/APIchanges b/doc/APIchanges
index 67a6142401..6d3b573c2d 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -15,6 +15,9 @@ libavutil: 2015-08-28
 
 API changes, most recent first:
 
+2017-05-18 - xx - lavu 55.63.100 - frame.h
+  Add av_frame_check_align().
+
 2017-05-15 - xx - lavc 57.96.100 - avcodec.h
   VideoToolbox hardware-accelerated decoding now supports the new hwaccel API,
   which can create the decoder context and allocate hardware frames 
automatically.
diff --git a/libavutil/frame.c b/libavutil/frame.c
index 24d5d5f184..aed3cd04ec 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -781,3 +781,20 @@ const char *av_frame_side_data_name(enum 
AVFrameSideDataType type)
 }
 return NULL;
 }
+
+int av_frame_check_align(const AVFrame *frame, unsigned align)
+{
+unsigned mask = align - 1;
+unsigned i;
+
+for (i = 0; i < AV_NUM_DATA_POINTERS; i++)
+if (((intptr_t)frame->data[i] & mask) ||
+(frame->linesize[i] & mask))
+return 0;
+if (!frame->extended_data || frame->extended_data == frame->data)
+return 1;
+for (i = AV_NUM_DATA_POINTERS; i < frame->channels; i++)
+if (((intptr_t)frame->extended_data[i] & mask))
+return 0;
+return 1;
+}
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 26261d7e40..1cbf7c7a5a 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -772,6 +772,14 @@ void av_frame_remove_side_data(AVFrame *frame, enum 
AVFrameSideDataType type);
 const char *av_frame_side_data_name(enum AVFrameSideDataType type);
 
 /**
+ * Check if the data pointers of a frame are aligned enough.
+ * Test if all frame data pointers have the alignment lower bits cleared,
+ * i.e. are a multiple of alignment.
+ * @return  >0 if aligned, 0 if not
+ */
+int av_frame_check_align(const AVFrame *frame, unsigned align);
+
+/**
  * @}
  */
 
-- 
2.11.0

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Muhammad Faiz]

On Thu, May 11, 2017 at 2:59 PM, Muhammad Faiz  wrote:
> On Tue, May 9, 2017 at 8:19 PM, Nicolas George  wrote:
>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>> index 24d5d5f184..e8467a1cd6 100644
>> --- a/libavutil/frame.c
>> +++ b/libavutil/frame.c
>> @@ -781,3 +781,21 @@ const char *av_frame_side_data_name(enum 
>> AVFrameSideDataType type)
>>  }
>>  return NULL;
>>  }
>> +
>> +int av_frame_check_align(const AVFrame *frame, unsigned align)
>> +{
>> +unsigned mask = (1 << align) - 1;
>> +unsigned i;
>> +int ret;
>> +
>> +av_assert1(align < 16);
>> +for (i = 0; i < AV_NUM_DATA_POINTERS; i++)
>> +if (((intptr_t)frame->data[i] & mask))
>> +return 0;
>> +if (!frame->extended_data || frame->extended_data == frame->data)
>> +return 1;
>> +for (i = AV_NUM_DATA_POINTERS; i < frame->channels; i++)
>> +if (((intptr_t)frame->extended_data[i] & mask))
>> +return 0;
>> +return 1;
>> +}
>
> Seem that you don't check linesize alignment. I don't know if it is
> required or not. Anyway the linesize constraint has been written in
> frame.h

Probably, data alignment and linesize alignment are treated differently.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Michael Niedermayer]

On Sun, May 14, 2017 at 12:07:59PM +0200, Nicolas George wrote:
> Le duodi 22 floréal, an CCXXV, Michael Niedermayer a écrit :
> > The direct value like 32 feels more natural to me too, but iam fine
> > with either.
> > 
> > The avoidance of log() might also favor the other in some cases btw
> > consider you have a 32 and need to call a fuction that needs a log2(32)
> 
> So, shall I push? With the current code or without the 1<

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Ronald S. Bultje]

Hi,

On Sun, May 14, 2017 at 6:07 AM, Nicolas George  wrote:

> Le duodi 22 floréal, an CCXXV, Michael Niedermayer a écrit :
> > The direct value like 32 feels more natural to me too, but iam fine
> > with either.
> >
> > The avoidance of log() might also favor the other in some cases btw
> > consider you have a 32 and need to call a fuction that needs a log2(32)
>
> So, shall I push?


No, Mohammad's point that you don't check linesize is not answered. Whether
or not the doxy says that it shall or shall not be bla bla doesn't really
matter, we're introducing API here so the user doesn't have to worry about
it, so we should possibly relax those constraints in the API and
check/reallocate here also.

With the current code or without the 1

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Le duodi 22 floréal, an CCXXV, Michael Niedermayer a écrit :
> The direct value like 32 feels more natural to me too, but iam fine
> with either.
> 
> The avoidance of log() might also favor the other in some cases btw
> consider you have a 32 and need to call a fuction that needs a log2(32)

So, shall I push? With the current code or without the 1<

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Muhammad Faiz]

On Tue, May 9, 2017 at 8:19 PM, Nicolas George  wrote:
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index 24d5d5f184..e8467a1cd6 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -781,3 +781,21 @@ const char *av_frame_side_data_name(enum 
> AVFrameSideDataType type)
>  }
>  return NULL;
>  }
> +
> +int av_frame_check_align(const AVFrame *frame, unsigned align)
> +{
> +unsigned mask = (1 << align) - 1;
> +unsigned i;
> +int ret;
> +
> +av_assert1(align < 16);
> +for (i = 0; i < AV_NUM_DATA_POINTERS; i++)
> +if (((intptr_t)frame->data[i] & mask))
> +return 0;
> +if (!frame->extended_data || frame->extended_data == frame->data)
> +return 1;
> +for (i = AV_NUM_DATA_POINTERS; i < frame->channels; i++)
> +if (((intptr_t)frame->extended_data[i] & mask))
> +return 0;
> +return 1;
> +}

Seem that you don't check linesize alignment. I don't know if it is
required or not. Anyway the linesize constraint has been written in
frame.h
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Michael Niedermayer]

On Wed, May 10, 2017 at 11:10:48PM +0200, Nicolas George wrote:
> Le primidi 21 floréal, an CCXXV, Hendrik Leppkes a écrit :
> > Everywhere I found where the align value is used, its used as (1 <<
> > alignment). In that case, I would prefer to pass the actual alignment
> > here (ie. 32 instead of 5), which is an easier value to understand and
> > matches the various alignment constants/values we already had before.
> 
> I can live with that, but here is my rationale for doing it that way:
> log2(align_mult) is a non-trivial function while 1< built-in.
> 
> I suggest we get a third opinion on the matter.

The direct value like 32 feels more natural to me too, but iam fine
with either.

The avoidance of log() might also favor the other in some cases btw
consider you have a 32 and need to call a fuction that needs a log2(32)

[...]


-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator


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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Clément Bœsch]

On Wed, May 10, 2017 at 11:11:58PM +0200, Hendrik Leppkes wrote:
> On Wed, May 10, 2017 at 11:10 PM, Nicolas George  wrote:
> > Le primidi 21 floréal, an CCXXV, Hendrik Leppkes a écrit :
> >> Everywhere I found where the align value is used, its used as (1 <<
> >> alignment). In that case, I would prefer to pass the actual alignment
> >> here (ie. 32 instead of 5), which is an easier value to understand and
> >> matches the various alignment constants/values we already had before.
> >
> > I can live with that, but here is my rationale for doing it that way:
> > log2(align_mult) is a non-trivial function while 1< > built-in.
> >
> >
> 
> Perhaps, but its not used like that anywhere

it's used at least for the fft API afaik

-- 
Clément B.


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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Hendrik Leppkes]

On Wed, May 10, 2017 at 11:10 PM, Nicolas George  wrote:
> Le primidi 21 floréal, an CCXXV, Hendrik Leppkes a écrit :
>> Everywhere I found where the align value is used, its used as (1 <<
>> alignment). In that case, I would prefer to pass the actual alignment
>> here (ie. 32 instead of 5), which is an easier value to understand and
>> matches the various alignment constants/values we already had before.
>
> I can live with that, but here is my rationale for doing it that way:
> log2(align_mult) is a non-trivial function while 1< built-in.
>
>

Perhaps, but its not used like that anywhere, and I can't imagine
why/where we would. Anything you had in mind when that might be
needed?

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Nicolas George]

Le primidi 21 floréal, an CCXXV, Hendrik Leppkes a écrit :
> Everywhere I found where the align value is used, its used as (1 <<
> alignment). In that case, I would prefer to pass the actual alignment
> here (ie. 32 instead of 5), which is an easier value to understand and
> matches the various alignment constants/values we already had before.

I can live with that, but here is my rationale for doing it that way:
log2(align_mult) is a non-trivial function while 1<

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

Re: [FFmpeg-devel] [PATCH 1/3] lavu/frame: add av_frame_check_align().

[Hendrik Leppkes]

On Tue, May 9, 2017 at 3:19 PM, Nicolas George  wrote:
> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 26261d7e40..196d311e29 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -772,6 +772,14 @@ void av_frame_remove_side_data(AVFrame *frame, enum 
> AVFrameSideDataType type);
>  const char *av_frame_side_data_name(enum AVFrameSideDataType type);
>
>  /**
> + * Check if the data pointers of a frame are aligned enough.
> + * Test if all frame data pointers have the alignment lower bits cleared,
> + * i.e. are a multiple of 1< + * @return  >0 if aligned, 0 if not
> + */
> +int av_frame_check_align(const AVFrame *frame, unsigned align);
> +

Everywhere I found where the align value is used, its used as (1 <<
alignment). In that case, I would prefer to pass the actual alignment
here (ie. 32 instead of 5), which is an easier value to understand and
matches the various alignment constants/values we already had before.

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