| Message ID | 20170518081114.21425-1-george@nsup.org |
|---|---|
| State | New |
| Headers | show |
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
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,
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
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,
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.
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,
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
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,
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.
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,
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.
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,
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); + +/** * @} */
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<<.