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

Submitted by Nicolas George on May 18, 2017, 8:11 a.m.

Details

Message ID 20170518081114.21425-1-george@nsup.org
State New
Headers show

Commit Message

Nicolas George May 18, 2017, 8:11 a.m.
Signed-off-by: Nicolas George <george@nsup.org>
---
 doc/APIchanges    |  3 +++
 libavutil/frame.c | 17 +++++++++++++++++
 libavutil/frame.h |  8 ++++++++
 3 files changed, 28 insertions(+)


With the linesize check and without the 1<<.

Comments

Hendrik Leppkes May 18, 2017, 8:40 a.m.
On Thu, May 18, 2017 at 10:11 AM, Nicolas George <george@nsup.org> wrote:
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  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
Nicolas George May 18, 2017, 8:44 a.m.
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,
Hendrik Leppkes May 18, 2017, 9 a.m.
On Thu, May 18, 2017 at 10:44 AM, Nicolas George <george@nsup.org> 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
Nicolas George May 18, 2017, 9:16 a.m.
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,
Muhammad Faiz May 18, 2017, 10:18 a.m.
On Thu, May 18, 2017 at 3:11 PM, Nicolas George <george@nsup.org> wrote:
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  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 - xxxxxxxxxx - lavu 55.63.100 - frame.h
> +  Add av_frame_check_align().
> +
>  2017-05-15 - xxxxxxxxxx - 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.
Nicolas George May 18, 2017, 10:20 a.m.
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,
Hendrik Leppkes May 18, 2017, 10:21 a.m.
On Thu, May 18, 2017 at 11:16 AM, Nicolas George <george@nsup.org> 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
Nicolas George May 18, 2017, 10:25 a.m.
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,
Muhammad Faiz May 18, 2017, 10:28 a.m.
On Thu, May 18, 2017 at 5:20 PM, Nicolas George <george@nsup.org> 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.
Nicolas George May 18, 2017, 10:30 a.m.
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,
Muhammad Faiz May 18, 2017, 10:33 a.m.
On Thu, May 18, 2017 at 5:30 PM, Nicolas George <george@nsup.org> 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.
Nicolas George May 18, 2017, 10:37 a.m.
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,

Patch hide | download patch | download mbox

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 - xxxxxxxxxx - lavu 55.63.100 - frame.h
+  Add av_frame_check_align().
+
 2017-05-15 - xxxxxxxxxx - 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);
+
+/**
  * @}
  */