| Message ID | 3ed2d4ae-1790-50f8-c95d-cbd9618501d4@jkqxz.net |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 14 Sep 2017, Mark Thompson wrote: > --- > There are more around, but this marks all of the obvious ones in avcodec.h. > [...] > +av_warn_unused_result > int avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt); > */ > +av_warn_unused_result > int avcodec_send_frame(AVCodecContext *avctx, const AVFrame *frame); I am not sure about these, these can only fail with ENOMEM if the user always gets all available frames/packets, but even if there is no memory, no state will be changed, the ownership of the packet/frame will remain with the caller, no null pointer will be returned, so if ignoring these is what the user wants, then so be it, no "undefined" behaviour will occur and we are not breaking the API contract by continuing like nothing happened. Regards, Marton
On 14/09/17 22:54, Marton Balint wrote: > > On Thu, 14 Sep 2017, Mark Thompson wrote: > >> --- >> There are more around, but this marks all of the obvious ones in avcodec.h. >> > > [...] > >> +av_warn_unused_result >> int avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt); > >> */ >> +av_warn_unused_result >> int avcodec_send_frame(AVCodecContext *avctx, const AVFrame *frame); > > I am not sure about these, these can only fail with ENOMEM if the user always gets all available frames/packets, but even if there is no memory, no state will be changed, the ownership of the packet/frame will remain with the caller, no null pointer will be returned, so if ignoring these is what the user wants, then so be it, no "undefined" behaviour will occur and we are not breaking the API contract by continuing like nothing happened. I was primarily thinking of the EAGAIN case, where the input data will just have been silently discarded if the user ignores the return value. I admit that isn't quite the same as the failed-allocation ones which result in undefined behaviour, but I don't think ignoring the return value of any of the send/receive functions is sane on the part of the user. What do you think? (I could also split the patch into ENOMEM and other cases if that helps.) - Mark
On Thu, 14 Sep 2017, Mark Thompson wrote: > On 14/09/17 22:54, Marton Balint wrote: >> >> On Thu, 14 Sep 2017, Mark Thompson wrote: >> >>> --- >>> There are more around, but this marks all of the obvious ones in avcodec.h. >>> >> >> [...] >> >>> +av_warn_unused_result >>> int avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt); >> >>> */ >>> +av_warn_unused_result >>> int avcodec_send_frame(AVCodecContext *avctx, const AVFrame *frame); >> >> I am not sure about these, these can only fail with ENOMEM if the user >> always gets all available frames/packets, but even if there is no >> memory, no state will be changed, the ownership of the packet/frame >> will remain with the caller, no null pointer will be returned, so if >> ignoring these is what the user wants, then so be it, no "undefined" >> behaviour will occur and we are not breaking the API contract by >> continuing like nothing happened. > > I was primarily thinking of the EAGAIN case, where the input data will > just have been silently discarded if the user ignores the return value. > I admit that isn't quite the same as the failed-allocation ones which > result in undefined behaviour, but I don't think ignoring the return > value of any of the send/receive functions is sane on the part of the > user. > > What do you think? (I could also split the patch into ENOMEM and other > cases if that helps.) If the send/receive API is used in a way that receive is called in a loop until EAGAIN is returned after each send, that ensures that no EAGAIN can occur in send. ffplay does exactly that and the only reason it does not ignore the return value of send_packet is to be able to warn the user about API violations, which - in theory - should not happen. So I'd rather not tag these functions as warn_unused_result, but if others disagree, I don't mind too much if the patch goes in as is. Regards, Marton
On 14/09/17 23:35, Marton Balint wrote: > > On Thu, 14 Sep 2017, Mark Thompson wrote: > >> On 14/09/17 22:54, Marton Balint wrote: >>> >>> On Thu, 14 Sep 2017, Mark Thompson wrote: >>> >>>> --- >>>> There are more around, but this marks all of the obvious ones in avcodec.h. >>>> >>> >>> [...] >>> >>>> +av_warn_unused_result >>>> int avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt); >>> >>>> */ >>>> +av_warn_unused_result >>>> int avcodec_send_frame(AVCodecContext *avctx, const AVFrame *frame); >>> >>> I am not sure about these, these can only fail with ENOMEM if the user always gets all available frames/packets, but even if there is no memory, no state will be changed, the ownership of the packet/frame will remain with the caller, no null pointer will be returned, so if ignoring these is what the user wants, then so be it, no "undefined" behaviour will occur and we are not breaking the API contract by continuing like nothing happened. >> >> I was primarily thinking of the EAGAIN case, where the input data will just have been silently discarded if the user ignores the return value. I admit that isn't quite the same as the failed-allocation ones which result in undefined behaviour, but I don't think ignoring the return value of any of the send/receive functions is sane on the part of the user. >> >> What do you think? (I could also split the patch into ENOMEM and other cases if that helps.) > > If the send/receive API is used in a way that receive is called in a loop until EAGAIN is returned after each send, that ensures that no EAGAIN can occur in send. > > ffplay does exactly that and the only reason it does not ignore the return value of send_packet is to be able to warn the user about API violations, which - in theory - should not happen. > > So I'd rather not tag these functions as warn_unused_result, but if others disagree, I don't mind too much if the patch goes in as is. Ok, that's fair. I've removed the annotation from the the send functions - these two and also av_bsf_send_packet(). Thanks, - Mark
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index 5c84974e03..758686af23 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4483,6 +4483,7 @@ int avcodec_parameters_to_context(AVCodecContext *codec, * @see avcodec_alloc_context3(), avcodec_find_decoder(), avcodec_find_encoder(), * av_dict_set(), av_opt_find(). */ +av_warn_unused_result int avcodec_open2(AVCodecContext *avctx, const AVCodec *codec, AVDictionary **options); /** @@ -4568,6 +4569,7 @@ void av_init_packet(AVPacket *pkt); * @param size wanted payload size * @return 0 if OK, AVERROR_xxx otherwise */ +av_warn_unused_result int av_new_packet(AVPacket *pkt, int size); /** @@ -4584,6 +4586,7 @@ void av_shrink_packet(AVPacket *pkt, int size); * @param pkt packet * @param grow_by number of bytes by which to increase the size of the packet */ +av_warn_unused_result int av_grow_packet(AVPacket *pkt, int grow_by); /** @@ -4736,6 +4739,7 @@ void av_packet_free_side_data(AVPacket *pkt); * * @return 0 on success, a negative AVERROR on error. */ +av_warn_unused_result int av_packet_ref(AVPacket *dst, const AVPacket *src); /** @@ -4814,6 +4818,7 @@ AVCodec *avcodec_find_decoder_by_name(const char *name); * it can be called by custom get_buffer2() implementations for decoders without * AV_CODEC_CAP_DR1 set. */ +av_warn_unused_result int avcodec_default_get_buffer2(AVCodecContext *s, AVFrame *frame, int flags); #if FF_API_EMU_EDGE @@ -5060,6 +5065,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub, * AVERROR(ENOMEM): failed to add packet to internal queue, or similar * other errors: legitimate decoding errors */ +av_warn_unused_result int avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt); /** @@ -5080,6 +5086,7 @@ int avcodec_send_packet(AVCodecContext *avctx, const AVPacket *avpkt); * AVERROR(EINVAL): codec not opened, or it is an encoder * other negative values: legitimate decoding errors */ +av_warn_unused_result int avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame); /** @@ -5117,6 +5124,7 @@ int avcodec_receive_frame(AVCodecContext *avctx, AVFrame *frame); * AVERROR(ENOMEM): failed to add packet to internal queue, or similar * other errors: legitimate decoding errors */ +av_warn_unused_result int avcodec_send_frame(AVCodecContext *avctx, const AVFrame *frame); /** @@ -5134,6 +5142,7 @@ int avcodec_send_frame(AVCodecContext *avctx, const AVFrame *frame); * AVERROR(EINVAL): codec not opened, or it is an encoder * other errors: legitimate decoding errors */ +av_warn_unused_result int avcodec_receive_packet(AVCodecContext *avctx, AVPacket *avpkt); @@ -6091,12 +6100,14 @@ const AVBitStreamFilter *av_bsf_next(void **opaque); * * @return 0 on success, a negative AVERROR code on failure */ +av_warn_unused_result int av_bsf_alloc(const AVBitStreamFilter *filter, AVBSFContext **ctx); /** * Prepare the filter for use, after all the parameters and options have been * set. */ +av_warn_unused_result int av_bsf_init(AVBSFContext *ctx); /** @@ -6114,6 +6125,7 @@ int av_bsf_init(AVBSFContext *ctx); * * @return 0 on success, a negative AVERROR on error. */ +av_warn_unused_result int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt); /** @@ -6140,6 +6152,7 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt); * output fewer packets than were sent to it, so this function may return * AVERROR(EAGAIN) immediately after a successful av_bsf_send_packet() call. */ +av_warn_unused_result int av_bsf_receive_packet(AVBSFContext *ctx, AVPacket *pkt); /**