| Message ID | 20161224174149.8995-9-george@nsup.org |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, Dec 24, 2016 at 06:41:40PM +0100, Nicolas George wrote: > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavfilter/avfilter.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++ > libavfilter/internal.h | 47 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 110 insertions(+) [...] > +/** > + * Take a frame from the link's FIFO and update the link's stats. > + * Must only be called if a frame is available (assert1). > + * @note May trigger process_command() and/or update is_disabled. > + */ > +int ff_link_consume_frame_sure(AVFilterLink *link, AVFrame **rframe); > + > +/** > + * Take samples from the link's FIFO and update the link's stats. > + * Must only be called if samples are available (assert1). > + * @note May trigger process_command() and/or update is_disabled. > + */ > +int ff_link_consume_samples_sure(AVFilterLink *link, unsigned min, unsigned max, > + AVFrame **rframe); > + > +/** > + * Take a frame from the link's FIFO and update the link's stats. > + * @note May trigger process_command() and/or update is_disabled. > + * @return >0 if a frame is available, > + * 0 and sets rframe to NULL if no frame available, > + * or AVERROR code > + */ > +int ff_link_consume_frame(AVFilterLink *link, AVFrame **rframe); > + > +/** > + * Take samples from the link's FIFO and update the link's stats. > + * @note May trigger process_command() and/or update is_disabled. > + * @return >0 if a frame is available, > + * 0 and sets rframe to NULL if no frame available, > + * or AVERROR code > + */ > +int ff_link_consume_samples(AVFilterLink *link, unsigned min, unsigned max, > + AVFrame **rframe); > + Why do these functions exist twice ? the API would be simpler with just one set [...]
Le quintidi 5 nivôse, an CCXXV, Michael Niedermayer a écrit : > Why do these functions exist twice ? > the API would be simpler with just one set You mean the _samples / _frame versions, or the normal / _sure versions? The _samples / _frame case seemed easier than distinguishing using special values of the arguments. The normal / _sure are useful to express that availability check has already done. That way, if the check is or becomes invalid (maybe because someone adds a special case between the test and the use) it is detected by an assert failure during FATE rather than become an ignored error code. I caught quite a few mistakes that way. But of course, it can be discussed. Regards,
On Sun, Dec 25, 2016 at 02:00:13AM +0100, Nicolas George wrote: > Le quintidi 5 nivôse, an CCXXV, Michael Niedermayer a écrit : > > Why do these functions exist twice ? > > the API would be simpler with just one set > > You mean the _samples / _frame versions, or the normal / _sure versions? i meant the _sure versions > > The _samples / _frame case seemed easier than distinguishing using > special values of the arguments. > > The normal / _sure are useful to express that availability check has > already done. > > That way, if the check is or becomes invalid (maybe because someone adds > a special case between the test and the use) it is detected by an assert > failure during FATE rather than become an ignored error code. I caught > quite a few mistakes that way. Whats your oppinion on using a explicit av_assert1() in the calling code for this ? (i assume it can be done easily&cleanly) It would explicitly in C code say what is meant, while a "_sure" requires additional knowledge specific to lavfi [...]
Le quintidi 5 nivôse, an CCXXV, Michael Niedermayer a écrit : > Whats your oppinion on using a explicit av_assert1() in the calling > code for this ? (i assume it can be done easily&cleanly) > > It would explicitly in C code say what is meant, while a > "_sure" requires additional knowledge specific to lavfi You mean, in the caller, instead of: ret = ff_link_consume_frame_sure(link, &frame); write: ret = ff_link_consume_frame(link, &frame); av_assert1(ret >= 0); Well, it loses us the property that ff_link_consume_frame_sure() cannot fail at all (ff_link_consume_samples_sure() can, because it allocates memory) and thus do not require getting the return value at all. But I was not sure I wanted to make this a promise anyway. Also, it adds extra tests: one in the code, one in consume() instead of just one in the code (not counting the asserts, only present in debug builds). But that is just my premature optimizer side talking. Apart from that I am ok with it. It just requires documentation that this is the recommended idiom. Regards,
On Sun, Dec 25, 2016 at 11:44:07AM +0100, Nicolas George wrote: > Le quintidi 5 nivôse, an CCXXV, Michael Niedermayer a écrit : > > Whats your oppinion on using a explicit av_assert1() in the calling > > code for this ? (i assume it can be done easily&cleanly) > > > > It would explicitly in C code say what is meant, while a > > "_sure" requires additional knowledge specific to lavfi > > You mean, in the caller, instead of: > > ret = ff_link_consume_frame_sure(link, &frame); > > write: > > ret = ff_link_consume_frame(link, &frame); > av_assert1(ret >= 0); yes, something like that (or a av_assert1(ret != AVERROR_OUT_OF_FUEL) > > Well, it loses us the property that ff_link_consume_frame_sure() cannot > fail at all (ff_link_consume_samples_sure() can, because it allocates > memory) and thus do not require getting the return value at all. But I > was not sure I wanted to make this a promise anyway. > > Also, it adds extra tests: one in the code, one in consume() instead of > just one in the code (not counting the asserts, only present in debug > builds). But that is just my premature optimizer side talking. its premature too but if we have expensive checks we could cache the result in the link [...]
diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c index 22a1fedd17..e2eb2980be 100644 --- a/libavfilter/avfilter.c +++ b/libavfilter/avfilter.c @@ -1498,6 +1498,69 @@ int ff_link_acknowledge_status(AVFilterLink *link) return 1; } +int ff_link_check_available_frame(AVFilterLink *link) +{ + return ff_framequeue_queued_frames(&link->fifo) > 0; +} + +int ff_link_check_available_samples(AVFilterLink *link, unsigned min) +{ + uint64_t samples = ff_framequeue_queued_samples(&link->fifo); + av_assert1(min); + return samples >= min || (link->status_in && samples); +} + +static void consume_update(AVFilterLink *link, const AVFrame *frame) +{ + ff_link_process_commands(link, frame); + ff_link_process_timeline(link, frame); + link->frame_count_out++; +} + +int ff_link_consume_frame_sure(AVFilterLink *link, AVFrame **rframe) +{ + AVFrame *frame; + + av_assert1(ff_link_check_available_frame(link)); + frame = ff_framequeue_take(&link->fifo); + consume_update(link, frame); + *rframe = frame; + return 1; +} + +int ff_link_consume_samples_sure(AVFilterLink *link, unsigned min, unsigned max, + AVFrame **rframe) +{ + AVFrame *frame; + int ret; + + av_assert1(min); + av_assert1(ff_link_check_available_samples(link, min)); + if (link->status_in) + min = FFMIN(min, ff_framequeue_queued_samples(&link->fifo)); + ret = take_samples(link, min, link->max_samples, &frame); + if (ret < 0) + return ret; + consume_update(link, frame); + *rframe = frame; + return 1; +} + +int ff_link_consume_frame(AVFilterLink *link, AVFrame **rframe) +{ + *rframe = NULL; + return !ff_link_check_available_frame(link) ? 0 : + ff_link_consume_frame_sure(link, rframe); +} + +int ff_link_consume_samples(AVFilterLink *link, unsigned min, unsigned max, + AVFrame **rframe) +{ + *rframe = NULL; + return !ff_link_check_available_samples(link, min) ? 0 : + ff_link_consume_samples_sure(link, min, max, rframe); +} + int ff_link_make_frame_writable(AVFilterLink *link, AVFrame **rframe) { AVFrame *frame = *rframe; diff --git a/libavfilter/internal.h b/libavfilter/internal.h index 4488821fe0..9aaf585eca 100644 --- a/libavfilter/internal.h +++ b/libavfilter/internal.h @@ -625,6 +625,53 @@ void ff_filter_set_ready(AVFilterContext *filter, unsigned priority); int ff_link_acknowledge_status(AVFilterLink *link); /** + * Test if a frame is available on the link. + * @return >0 if a frame is available + */ +int ff_link_check_available_frame(AVFilterLink *link); + +/** + * Test if enough samples are available on the link. + * @return >0 if enough samples are available + * @note on EOF and error, min becomes 1 + */ +int ff_link_check_available_samples(AVFilterLink *link, unsigned min); + +/** + * Take a frame from the link's FIFO and update the link's stats. + * Must only be called if a frame is available (assert1). + * @note May trigger process_command() and/or update is_disabled. + */ +int ff_link_consume_frame_sure(AVFilterLink *link, AVFrame **rframe); + +/** + * Take samples from the link's FIFO and update the link's stats. + * Must only be called if samples are available (assert1). + * @note May trigger process_command() and/or update is_disabled. + */ +int ff_link_consume_samples_sure(AVFilterLink *link, unsigned min, unsigned max, + AVFrame **rframe); + +/** + * Take a frame from the link's FIFO and update the link's stats. + * @note May trigger process_command() and/or update is_disabled. + * @return >0 if a frame is available, + * 0 and sets rframe to NULL if no frame available, + * or AVERROR code + */ +int ff_link_consume_frame(AVFilterLink *link, AVFrame **rframe); + +/** + * Take samples from the link's FIFO and update the link's stats. + * @note May trigger process_command() and/or update is_disabled. + * @return >0 if a frame is available, + * 0 and sets rframe to NULL if no frame available, + * or AVERROR code + */ +int ff_link_consume_samples(AVFilterLink *link, unsigned min, unsigned max, + AVFrame **rframe); + +/** * Make sure a frame is writable. * This is similar to av_frame_make_writable() except it uses the link's * buffer allocation callback, and therefore allows direct rendering.
Signed-off-by: Nicolas George <george@nsup.org> --- libavfilter/avfilter.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++ libavfilter/internal.h | 47 +++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+)