| Message ID | 20200409230903.1516-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/bsf: mention that av_bsf_send_packet() returning EAGAIN is not an error | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On 10/04/2020 00:09, James Almer wrote: > EAGAIN is returned when input is provided but can't be consumed. The filtering > process is unaffected in this case, and the function will be able to consume > new input after retrieving filtered packets with av_bsf_receive_packet(). > > Remove the line about empty packets never failing added in > 41b05b849f215b03eeb9e3608571ba47de64182a while at it. Even if it's currently > the case, it unnecessarily constrains the API and could be changed in the future > in case it needs to be extended. > The user should always check for errors and never expect a call to never fail. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/avcodec.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Sounds good to me. - Derek
On 4/10/2020 11:07 AM, Derek Buitenhuis wrote: > On 10/04/2020 00:09, James Almer wrote: >> EAGAIN is returned when input is provided but can't be consumed. The filtering >> process is unaffected in this case, and the function will be able to consume >> new input after retrieving filtered packets with av_bsf_receive_packet(). >> >> Remove the line about empty packets never failing added in >> 41b05b849f215b03eeb9e3608571ba47de64182a while at it. Even if it's currently >> the case, it unnecessarily constrains the API and could be changed in the future >> in case it needs to be extended. >> The user should always check for errors and never expect a call to never fail. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) > > Sounds good to me. > > - Derek Will push, thanks. That aside, I'm planning on restructuring these bsf functions internally, making it behave more in line with the send_packet/receive_frame API for decoding, and there are two options for this: One, i keep the behavior described in the doxy after this patch regarding return values from av_bsf_send_packet() (every AVERROR code except EAGAIN being an error). Or two, make it return EOF instead of 0 when extra unnecessary flush packets are passed to it, thus making both bsf and decode APIs behave exactly the same. The latter would be ideal, but i don't know if it could be considered an API change or not. It would have no effect on existing usage for example in decode.c and the CLI. The latter in fact considers EOF from av_bsf_send_packet() a possibility, so it may have been an oversight back when the bsf API was first written. What do you think?
Quoting James Almer (2020-04-10 17:42:23) > On 4/10/2020 11:07 AM, Derek Buitenhuis wrote: > > On 10/04/2020 00:09, James Almer wrote: > >> EAGAIN is returned when input is provided but can't be consumed. The filtering > >> process is unaffected in this case, and the function will be able to consume > >> new input after retrieving filtered packets with av_bsf_receive_packet(). > >> > >> Remove the line about empty packets never failing added in > >> 41b05b849f215b03eeb9e3608571ba47de64182a while at it. Even if it's currently > >> the case, it unnecessarily constrains the API and could be changed in the future > >> in case it needs to be extended. > >> The user should always check for errors and never expect a call to never fail. > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavcodec/avcodec.h | 5 +++-- > >> 1 file changed, 3 insertions(+), 2 deletions(-) > > > > Sounds good to me. > > > > - Derek > > Will push, thanks. > > That aside, I'm planning on restructuring these bsf functions > internally, making it behave more in line with the > send_packet/receive_frame API for decoding, and there are two options > for this: One, i keep the behavior described in the doxy after this > patch regarding return values from av_bsf_send_packet() (every AVERROR > code except EAGAIN being an error). Or two, make it return EOF instead > of 0 when extra unnecessary flush packets are passed to it, thus making > both bsf and decode APIs behave exactly the same. > > The latter would be ideal, but i don't know if it could be considered an > API change or not. I don't really see how it makes things better. I can imagine valid uses for sending multiple flush packets - e.g. when the caller would otherwise have to keep an extra variable saying "flush packet has been sent". And we don't really gain anything from forbidding extra flush packets. > It would have no effect on existing usage for example in decode.c and > the CLI. The latter in fact considers EOF from av_bsf_send_packet() a > possibility, so it may have been an oversight back when the bsf API > was first written. I think the check for AVERROR_EOF there is intended for the value from receive_packet.
On 4/11/2020 5:33 AM, Anton Khirnov wrote: > Quoting James Almer (2020-04-10 17:42:23) >> On 4/10/2020 11:07 AM, Derek Buitenhuis wrote: >>> On 10/04/2020 00:09, James Almer wrote: >>>> EAGAIN is returned when input is provided but can't be consumed. The filtering >>>> process is unaffected in this case, and the function will be able to consume >>>> new input after retrieving filtered packets with av_bsf_receive_packet(). >>>> >>>> Remove the line about empty packets never failing added in >>>> 41b05b849f215b03eeb9e3608571ba47de64182a while at it. Even if it's currently >>>> the case, it unnecessarily constrains the API and could be changed in the future >>>> in case it needs to be extended. >>>> The user should always check for errors and never expect a call to never fail. >>>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/avcodec.h | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> Sounds good to me. >>> >>> - Derek >> >> Will push, thanks. >> >> That aside, I'm planning on restructuring these bsf functions >> internally, making it behave more in line with the >> send_packet/receive_frame API for decoding, and there are two options >> for this: One, i keep the behavior described in the doxy after this >> patch regarding return values from av_bsf_send_packet() (every AVERROR >> code except EAGAIN being an error). Or two, make it return EOF instead >> of 0 when extra unnecessary flush packets are passed to it, thus making >> both bsf and decode APIs behave exactly the same. >> >> The latter would be ideal, but i don't know if it could be considered an >> API change or not. > > I don't really see how it makes things better. I can imagine valid uses > for sending multiple flush packets - e.g. when the caller would > otherwise have to keep an extra variable saying "flush packet has been > sent". And we don't really gain anything from forbidding extra flush > packets. Not necessarily better, just in line with the decode API. It would be nice to have all our decoupled input/output APIs behaving the same, instead of each featuring one small difference here and there. And ok, will ensure it keeps returning 0 on extra flush packets, then. > >> It would have no effect on existing usage for example in decode.c and >> the CLI. The latter in fact considers EOF from av_bsf_send_packet() a >> possibility, so it may have been an oversight back when the bsf API >> was first written. > > I think the check for AVERROR_EOF there is intended for the value from > receive_packet. >
Quoting James Almer (2020-04-11 15:21:24) > > Not necessarily better, just in line with the decode API. It would be > nice to have all our decoupled input/output APIs behaving the same, > instead of each featuring one small difference here and there. We could also change the decode API to be in line with this one :)
On 4/11/2020 11:04 AM, Anton Khirnov wrote: > Quoting James Almer (2020-04-11 15:21:24) >> >> Not necessarily better, just in line with the decode API. It would be >> nice to have all our decoupled input/output APIs behaving the same, >> instead of each featuring one small difference here and there. > > We could also change the decode API to be in line with this one :) The doxy for the decode API already defines EOF in this specific scenario, so that's clearly not an option.
On 4/10/2020 11:07 AM, Derek Buitenhuis wrote: > On 10/04/2020 00:09, James Almer wrote: >> EAGAIN is returned when input is provided but can't be consumed. The filtering >> process is unaffected in this case, and the function will be able to consume >> new input after retrieving filtered packets with av_bsf_receive_packet(). >> >> Remove the line about empty packets never failing added in >> 41b05b849f215b03eeb9e3608571ba47de64182a while at it. Even if it's currently >> the case, it unnecessarily constrains the API and could be changed in the future >> in case it needs to be extended. >> The user should always check for errors and never expect a call to never fail. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/avcodec.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) > > Sounds good to me. > > - Derek Pushed, thanks.
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 8fc0ad92c9..ba35cb695b 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -6020,8 +6020,9 @@ int av_bsf_init(AVBSFContext *ctx); * sending more empty packets does nothing) and will cause the filter to output * any packets it may have buffered internally. * - * @return 0 on success, a negative AVERROR on error. This function never fails if - * pkt is empty. + * @return 0 on success. AVERROR(EAGAIN) if packets need to be retrieved from the + * filter (using av_bsf_receive_packet()) before new input can be consumed. Another + * negative AVERROR value if an error occurs. */ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt);
EAGAIN is returned when input is provided but can't be consumed. The filtering process is unaffected in this case, and the function will be able to consume new input after retrieving filtered packets with av_bsf_receive_packet(). Remove the line about empty packets never failing added in 41b05b849f215b03eeb9e3608571ba47de64182a while at it. Even if it's currently the case, it unnecessarily constrains the API and could be changed in the future in case it needs to be extended. The user should always check for errors and never expect a call to never fail. Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/avcodec.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)