Message ID | 20210430113432.308139-2-bradh@frogmouth.net |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/5] libavutil: add convenience accessors for frame side data | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
Apr 30, 2021, 13:34 by bradh@frogmouth.net: > Signed-off-by: Brad Hards <bradh@frogmouth.net> > --- > libavutil/frame.c | 31 +++++++++++++++++++++++++++++++ > libavutil/frame.h | 23 +++++++++++++++++++++++ > 2 files changed, 54 insertions(+) > > diff --git a/libavutil/frame.c b/libavutil/frame.c > index 2ec59b44b1..9f9953c2b4 100644 > --- a/libavutil/frame.c > +++ b/libavutil/frame.c > @@ -625,6 +625,37 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, > return NULL; > } > > +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame, > + enum AVFrameSideDataType type, > + const int side_data_instance) > +{ > + int i; > + int n = 0; > + > + for (i = 0; i < frame->nb_side_data; i++) { > + if (frame->side_data[i]->type == type) { > + if (n == side_data_instance) { > + return frame->side_data[i]; > + } else { > + n++; > + } > + } > + } > + return NULL; > +} > _follow_the_code_style_ We have a document! We have thousands of lines of code already written with it! We do not add brackets on one-line statements, and we allow for (int loops. I don't like this function's name, and I don't like the way it operates. If we do allow multiple side-data entries with the same type (my opinion is if you can't read them without workarounds they're just not allowed), I'd much rather have a linked list, which would allow us to keep the same style of _next(prev) iteration functions we've used everywhere else. Plus, it would mean you don't have to do a worst possible case iteration for each lookup when you have a million side data entries. Users have wanted to do this. I think we should just have a av_frame_get_side_data_next(prev), where prev will be NULL for the first call. Users can filter by data type themselves. That way av_frame_get_side_data_nb can be removed. I'd also like an APIchanges entry we're allowing multiple side data entries with the same type. This is not a small change in behavior that we're making official.
On 4/30/2021 10:36 AM, Lynne wrote: > Apr 30, 2021, 13:34 by bradh@frogmouth.net: > >> Signed-off-by: Brad Hards <bradh@frogmouth.net> >> --- >> libavutil/frame.c | 31 +++++++++++++++++++++++++++++++ >> libavutil/frame.h | 23 +++++++++++++++++++++++ >> 2 files changed, 54 insertions(+) >> >> diff --git a/libavutil/frame.c b/libavutil/frame.c >> index 2ec59b44b1..9f9953c2b4 100644 >> --- a/libavutil/frame.c >> +++ b/libavutil/frame.c >> @@ -625,6 +625,37 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, >> return NULL; >> } >> >> +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame, >> + enum AVFrameSideDataType type, >> + const int side_data_instance) >> +{ >> + int i; >> + int n = 0; >> + >> + for (i = 0; i < frame->nb_side_data; i++) { >> + if (frame->side_data[i]->type == type) { >> + if (n == side_data_instance) { >> + return frame->side_data[i]; >> + } else { >> + n++; >> + } >> + } >> + } >> + return NULL; >> +} >> > > _follow_the_code_style_ > We have a document! We have thousands of lines of code already written with it! > We do not add brackets on one-line statements, and we allow for (int loops. > > I don't like this function's name, and I don't like the way it operates. > If we do allow multiple side-data entries with the same type (my opinion is if you can't read them > without workarounds they're just not allowed), I'd much rather have a linked list, which would > allow us to keep the same style of _next(prev) iteration functions we've used everywhere else. > Plus, it would mean you don't have to do a worst possible case iteration for each lookup when you > have a million side data entries. Users have wanted to do this. > > I think we should just have a av_frame_get_side_data_next(prev), where prev will be > NULL for the first call. Users can filter by data type themselves. av_frame_get_side_data_iterate(void **opaque), if anything, following existing API like those iterating through AVCodecs. There's nothing in a returned AVFrameSideData that points to the next entry. In any case, this would require a warning about calling av_frame_remove_side_data() invalidating the iteration state. > That way av_frame_get_side_data_nb can be removed. > > I'd also like an APIchanges entry we're allowing multiple side data entries with the same type. > This is not a small change in behavior that we're making official. This would not be a change in behavior. We would only be adding an official way to fetch every entry beyond the first by introducing an iterator function, something already possible if you manually iterate through frame->side_data entries, something we afaik never really forbid. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On Fri, 30 Apr 2021, Lynne wrote: > Apr 30, 2021, 13:34 by bradh@frogmouth.net: > >> Signed-off-by: Brad Hards <bradh@frogmouth.net> >> --- >> libavutil/frame.c | 31 +++++++++++++++++++++++++++++++ >> libavutil/frame.h | 23 +++++++++++++++++++++++ >> 2 files changed, 54 insertions(+) >> >> diff --git a/libavutil/frame.c b/libavutil/frame.c >> index 2ec59b44b1..9f9953c2b4 100644 >> --- a/libavutil/frame.c >> +++ b/libavutil/frame.c >> @@ -625,6 +625,37 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, >> return NULL; >> } >> >> +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame, >> + enum AVFrameSideDataType type, >> + const int side_data_instance) >> +{ >> + int i; >> + int n = 0; >> + >> + for (i = 0; i < frame->nb_side_data; i++) { >> + if (frame->side_data[i]->type == type) { >> + if (n == side_data_instance) { >> + return frame->side_data[i]; >> + } else { >> + n++; >> + } >> + } >> + } >> + return NULL; >> +} >> > > _follow_the_code_style_ > We have a document! We have thousands of lines of code already written with it! > We do not add brackets on one-line statements, and we allow for (int loops. The developer docs also says that FFmpeg does not force an indentation style on developers. So strictly speaking patch authors are not obligated to follow the general practice if they don't want to. > > I don't like this function's name, and I don't like the way it operates. > If we do allow multiple side-data entries with the same type (my opinion is if you can't read them > without workarounds they're just not allowed), I'd much rather have a linked list, which would > allow us to keep the same style of _next(prev) iteration functions we've used everywhere else. > Plus, it would mean you don't have to do a worst possible case iteration for each lookup when you > have a million side data entries. Users have wanted to do this. > > I think we should just have a av_frame_get_side_data_next(prev), where prev will be > NULL for the first call. Users can filter by data type themselves. > That way av_frame_get_side_data_nb can be removed. > > I'd also like an APIchanges entry we're allowing multiple side data entries with the same type. > This is not a small change in behavior that we're making official. Note the discrepancy between AVPacket/AVStream and AVFrame side data. AVPacket and AVStream side data DOES NOT allow the same type multiple times (the helper functions creating a new side data entry ovewrite the old of the same type of it exists). AVFrame does allow it. But the fact that we have API like av_frame_get_side_data() which returns a single (the first) entry implies that - at least for some side data types - only a single entry makes sense. Therefore - as James mentioned - for side data types when multiple entries make sense the users typically iterate over the side data entries themselves using a simple for() loop. If we want to provide an API for getting multiple frame side data of the same type, then I think the more natural thing to do is extending the existing function, e.g.: "av_frame_get_side_data2(frame, type, prev)" This way the user can use the extended version (iterator-style) if having multiple side data makes sense, and the original version if it does not. Regards, Marton
Apr 30, 2021, 22:29 by cus@passwd.hu: > > > On Fri, 30 Apr 2021, Lynne wrote: > >> Apr 30, 2021, 13:34 by bradh@frogmouth.net: >> >>> Signed-off-by: Brad Hards <bradh@frogmouth.net> >>> --- >>> libavutil/frame.c | 31 +++++++++++++++++++++++++++++++ >>> libavutil/frame.h | 23 +++++++++++++++++++++++ >>> 2 files changed, 54 insertions(+) >>> >>> diff --git a/libavutil/frame.c b/libavutil/frame.c >>> index 2ec59b44b1..9f9953c2b4 100644 >>> --- a/libavutil/frame.c >>> +++ b/libavutil/frame.c >>> @@ -625,6 +625,37 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, >>> return NULL; >>> } >>> >>> +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame, >>> + enum AVFrameSideDataType type, >>> + const int side_data_instance) >>> +{ >>> + int i; >>> + int n = 0; >>> + >>> + for (i = 0; i < frame->nb_side_data; i++) { >>> + if (frame->side_data[i]->type == type) { >>> + if (n == side_data_instance) { >>> + return frame->side_data[i]; >>> + } else { >>> + n++; >>> + } >>> + } >>> + } >>> + return NULL; >>> +} >>> >> >> _follow_the_code_style_ >> We have a document! We have thousands of lines of code already written with it! >> We do not add brackets on one-line statements, and we allow for (int loops. >> > > The developer docs also says that FFmpeg does not force an indentation style on developers. So strictly speaking patch authors are not obligated to follow the general practice if they don't want to. > I already said what I had to say about this, so I'm just going to say "no" and end it at that. Unless you too agree that asterisk symbols look so much like insects we should definitely have a MUL() macro, then we can talk. > If we want to provide an API for getting multiple frame side data of the same type, then I think the more natural thing to do is extending the existing function, e.g.: "av_frame_get_side_data2(frame, type, prev)" > This way the user can use the extended version (iterator-style) if having multiple side data makes sense, and the original version if it does not. > I could agree with that, though I'd still prefer a type-less av_frame_get_side_data_next(frame, prev).
On 5/4/2021 12:44 PM, Lynne wrote: > Apr 30, 2021, 22:29 by cus@passwd.hu: > >> >> >> On Fri, 30 Apr 2021, Lynne wrote: >> >>> Apr 30, 2021, 13:34 by bradh@frogmouth.net: >>> >>>> Signed-off-by: Brad Hards <bradh@frogmouth.net> >>>> --- >>>> libavutil/frame.c | 31 +++++++++++++++++++++++++++++++ >>>> libavutil/frame.h | 23 +++++++++++++++++++++++ >>>> 2 files changed, 54 insertions(+) >>>> >>>> diff --git a/libavutil/frame.c b/libavutil/frame.c >>>> index 2ec59b44b1..9f9953c2b4 100644 >>>> --- a/libavutil/frame.c >>>> +++ b/libavutil/frame.c >>>> @@ -625,6 +625,37 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, >>>> return NULL; >>>> } >>>> >>>> +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame, >>>> + enum AVFrameSideDataType type, >>>> + const int side_data_instance) >>>> +{ >>>> + int i; >>>> + int n = 0; >>>> + >>>> + for (i = 0; i < frame->nb_side_data; i++) { >>>> + if (frame->side_data[i]->type == type) { >>>> + if (n == side_data_instance) { >>>> + return frame->side_data[i]; >>>> + } else { >>>> + n++; >>>> + } >>>> + } >>>> + } >>>> + return NULL; >>>> +} >>>> >>> >>> _follow_the_code_style_ >>> We have a document! We have thousands of lines of code already written with it! >>> We do not add brackets on one-line statements, and we allow for (int loops. >>> >> >> The developer docs also says that FFmpeg does not force an indentation style on developers. So strictly speaking patch authors are not obligated to follow the general practice if they don't want to. >> > > I already said what I had to say about this, so I'm just going to say "no" and > end it at that. > Unless you too agree that asterisk symbols look so much like insects we should > definitely have a MUL() macro, then we can talk. > > >> If we want to provide an API for getting multiple frame side data of the same type, then I think the more natural thing to do is extending the existing function, e.g.: "av_frame_get_side_data2(frame, type, prev)" >> This way the user can use the extended version (iterator-style) if having multiple side data makes sense, and the original version if it does not. >> > > I could agree with that, though I'd still prefer a type-less av_frame_get_side_data_next(frame, prev). Again, AVFrameSideData has no fields pointing to another entry. It's all stored as flat array of pointers in AVFrame.side_data. A next() implementation would have to iterate through the entire array from the first element until it finds prev, and then return the next one, on every call. An iterate() implementation using a caller owned state variable is much more efficient. Something like (assuming type-less) > AVFrameSideData *av_frame_side_data_iterate(const AVFrame *frame, void **opaque) > { > uintptr_t i = (uintptr_t)*opaque; > > if (i >= frame->nb_side_data) > return NULL; > > *opaque = (void*)(i + 1); > > return frame->side_data[i]; > } Which is how av_codec_iterate() and many others handle this. I would also need a warning that calling av_frame_remove_side_data() will invalidate the iterator state.
May 4, 2021, 19:00 by jamrial@gmail.com: > On 5/4/2021 12:44 PM, Lynne wrote: > >> Apr 30, 2021, 22:29 by cus@passwd.hu: >> >>> >>> >>> On Fri, 30 Apr 2021, Lynne wrote: >>> >>>> Apr 30, 2021, 13:34 by bradh@frogmouth.net: >>>> >>>>> Signed-off-by: Brad Hards <bradh@frogmouth.net> >>>>> --- >>>>> libavutil/frame.c | 31 +++++++++++++++++++++++++++++++ >>>>> libavutil/frame.h | 23 +++++++++++++++++++++++ >>>>> 2 files changed, 54 insertions(+) >>>>> >>>>> diff --git a/libavutil/frame.c b/libavutil/frame.c >>>>> index 2ec59b44b1..9f9953c2b4 100644 >>>>> --- a/libavutil/frame.c >>>>> +++ b/libavutil/frame.c >>>>> @@ -625,6 +625,37 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, >>>>> return NULL; >>>>> } >>>>> >>>>> +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame, >>>>> + enum AVFrameSideDataType type, >>>>> + const int side_data_instance) >>>>> +{ >>>>> + int i; >>>>> + int n = 0; >>>>> + >>>>> + for (i = 0; i < frame->nb_side_data; i++) { >>>>> + if (frame->side_data[i]->type == type) { >>>>> + if (n == side_data_instance) { >>>>> + return frame->side_data[i]; >>>>> + } else { >>>>> + n++; >>>>> + } >>>>> + } >>>>> + } >>>>> + return NULL; >>>>> +} >>>>> >>>> >>>> _follow_the_code_style_ >>>> We have a document! We have thousands of lines of code already written with it! >>>> We do not add brackets on one-line statements, and we allow for (int loops. >>>> >>> >>> The developer docs also says that FFmpeg does not force an indentation style on developers. So strictly speaking patch authors are not obligated to follow the general practice if they don't want to. >>> >> >> I already said what I had to say about this, so I'm just going to say "no" and >> end it at that. >> Unless you too agree that asterisk symbols look so much like insects we should >> definitely have a MUL() macro, then we can talk. >> >> >>> If we want to provide an API for getting multiple frame side data of the same type, then I think the more natural thing to do is extending the existing function, e.g.: "av_frame_get_side_data2(frame, type, prev)" >>> This way the user can use the extended version (iterator-style) if having multiple side data makes sense, and the original version if it does not. >>> >> >> I could agree with that, though I'd still prefer a type-less av_frame_get_side_data_next(frame, prev). >> > > Again, AVFrameSideData has no fields pointing to another entry. It's all stored as flat array of pointers in AVFrame.side_data. A next() implementation would have to iterate through the entire array from the first element until it finds prev, and then return the next one, on every call. > > An iterate() implementation using a caller owned state variable is much more efficient. Something like (assuming type-less) > >> AVFrameSideData *av_frame_side_data_iterate(const AVFrame *frame, void **opaque) >> { >> uintptr_t i = (uintptr_t)*opaque; >> >> if (i >= frame->nb_side_data) >> return NULL; >> >> *opaque = (void*)(i + 1); >> >> return frame->side_data[i]; >> } >> > > Which is how av_codec_iterate() and many others handle this. > > I would also need a warning that calling av_frame_remove_side_data() will invalidate the iterator state. > Yes, that's what I meant, sorry I wasn't more specific.
diff --git a/libavutil/frame.c b/libavutil/frame.c index 2ec59b44b1..9f9953c2b4 100644 --- a/libavutil/frame.c +++ b/libavutil/frame.c @@ -625,6 +625,37 @@ AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, return NULL; } +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame, + enum AVFrameSideDataType type, + const int side_data_instance) +{ + int i; + int n = 0; + + for (i = 0; i < frame->nb_side_data; i++) { + if (frame->side_data[i]->type == type) { + if (n == side_data_instance) { + return frame->side_data[i]; + } else { + n++; + } + } + } + return NULL; +} + +int av_frame_num_side_data(const AVFrame *frame, enum AVFrameSideDataType type) +{ + int i; + int num = 0; + for (i = 0; i < frame->nb_side_data; i++) { + if (frame->side_data[i]->type == type) { + num += 1; + } + } + return num; +} + static int frame_copy_video(AVFrame *dst, const AVFrame *src) { const uint8_t *src_data[4]; diff --git a/libavutil/frame.h b/libavutil/frame.h index ff2540a20f..8e94e3f679 100644 --- a/libavutil/frame.h +++ b/libavutil/frame.h @@ -839,11 +839,34 @@ AVFrameSideData *av_frame_new_side_data_from_buf(AVFrame *frame, AVFrameSideData *av_frame_get_side_data(const AVFrame *frame, enum AVFrameSideDataType type); +/** + * Find a specified instance of side data of a given type. + * + * @param frame a frame to find the side data in + * @param type type of the side data to find + * @param side_data_instance instance of the side data to return (0 base). + * + * @return a pointer to the n'th instance of side data of a given type on + * success, NULL if there are less than side_data_instance instances of the + * given type. + */ +AVFrameSideData *av_frame_get_side_data_n(const AVFrame *frame, + enum AVFrameSideDataType type, + const int side_data_instance); + /** * Remove and free all side data instances of the given type. */ void av_frame_remove_side_data(AVFrame *frame, enum AVFrameSideDataType type); +/** + * Get the number of instances of side data of a given type. + * + * @param frame a frame to find the side data in + * @param type type of the side data to find + * @return count of instances, which can be 0 + */ +int av_frame_num_side_data(const AVFrame *frame, enum AVFrameSideDataType type); /** * Flags for frame cropping.
Signed-off-by: Brad Hards <bradh@frogmouth.net> --- libavutil/frame.c | 31 +++++++++++++++++++++++++++++++ libavutil/frame.h | 23 +++++++++++++++++++++++ 2 files changed, 54 insertions(+)