| Message ID | 20170509131944.27166-1-george@nsup.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, May 9, 2017 at 3:19 PM, Nicolas George <george@nsup.org> 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<<alignment. > + * @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
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<<align_log is a CPU built-in. I suggest we get a third opinion on the matter. Regards,
On Wed, May 10, 2017 at 11:10 PM, Nicolas George <george@nsup.org> 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<<align_log is a CPU > 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
On Wed, May 10, 2017 at 11:11:58PM +0200, Hendrik Leppkes wrote: > On Wed, May 10, 2017 at 11:10 PM, Nicolas George <george@nsup.org> 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<<align_log is a CPU > > built-in. > > > > > > Perhaps, but its not used like that anywhere it's used at least for the fft API afaik
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<<align_log is a CPU > 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) [...]
On Tue, May 9, 2017 at 8:19 PM, Nicolas George <george@nsup.org> 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
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<<? Regards,
Hi, On Sun, May 14, 2017 at 6:07 AM, Nicolas George <george@nsup.org> 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<<? In log2 units (so 4, 5 instead of 16, 32) is fine.
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<<? I am fine with and withhout 1<<. direct numbers seem more consistent to existing API like FFALIGN. It seems though theres still open questions or objections IIUC so i suspect this will more likely be in 3.3.2 ... thx [...]
On Thu, May 11, 2017 at 2:59 PM, Muhammad Faiz <mfcc64@gmail.com> wrote: > On Tue, May 9, 2017 at 8:19 PM, Nicolas George <george@nsup.org> 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.
diff --git a/doc/APIchanges b/doc/APIchanges index 09b1a49798..939d7d5f69 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,9 @@ libavutil: 2015-08-28 API changes, most recent first: +2017-xx-xx - xxxxxxxxxx - lavu 55.63.100 - frame.h + Add av_frame_check_align(). + 2017-xx-xx - xxxxxxx - lavc 57.95.100 / 57.31.0 - avcodec.h Add AVCodecContext.apply_cropping to control whether cropping is handled by libavcodec or the caller. 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; +} 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<<alignment. + * @return >0 if aligned, 0 if not + */ +int av_frame_check_align(const AVFrame *frame, unsigned align); + +/** * @} */ diff --git a/libavutil/version.h b/libavutil/version.h index 6762bf300a..fb61dcc666 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -80,7 +80,7 @@ #define LIBAVUTIL_VERSION_MAJOR 55 -#define LIBAVUTIL_VERSION_MINOR 62 +#define LIBAVUTIL_VERSION_MINOR 63 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \
Signed-off-by: Nicolas George <george@nsup.org> --- doc/APIchanges | 3 +++ libavutil/frame.c | 18 ++++++++++++++++++ libavutil/frame.h | 8 ++++++++ libavutil/version.h | 2 +- 4 files changed, 30 insertions(+), 1 deletion(-) Added "const" and the version bump.