Marton Balint:
>
>
> On Fri, 31 Dec 2021, Marton Balint wrote:
>
>>
>>
>> On Fri, 31 Dec 2021, Andreas Rheinhardt wrote:
>>
>>> Marton Balint:
>>>> This makes sure the error condition is kept in AVIOContext even if the
>>>> user
>>>> does not check the return value of avio_read_to_bprint or
>>>> ff_read_line_to_bprint.
>>>>
>>>> Signed-off-by: Marton Balint <c...@passwd.hu>
>>>> ---
>>>> libavformat/aviobuf.c | 8 ++++++--
>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
>>>> index 29d4bd7510..6f8a822ee3 100644
>>>> --- a/libavformat/aviobuf.c
>>>> +++ b/libavformat/aviobuf.c
>>>> @@ -875,8 +875,10 @@ static int64_t
>>>> read_string_to_bprint_overwrite(AVIOContext *s, AVBPrint *bp,
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> - if (!av_bprint_is_complete(bp))
>>>> + if (!av_bprint_is_complete(bp)) {
>>>> + s->error = AVERROR(ENOMEM);
>>>> return AVERROR(ENOMEM);
>>>> + }
>>>>
>>>> return bp->len;
>>>> }
>>>> @@ -1351,8 +1353,10 @@ int avio_read_to_bprint(AVIOContext *h,
>>>> AVBPrint
>>>> *pb, size_t max_size)
>>>> if (ret <= 0)
>>>> return ret;
>>>> av_bprint_append_data(pb, buf, ret);
>>>> - if (!av_bprint_is_complete(pb))
>>>> + if (!av_bprint_is_complete(pb)) {
>>>> + h->error = AVERROR(ENOMEM);
>>>> return AVERROR(ENOMEM);
>>>> + }
>>>> max_size -= ret;
>>>> }
>>>> return 0;
>>>>
>>>
>>> I don't really see the point of this: It is not a real read error that
>>> should stick to the AVIOContext (which can still be used afterwards
>>> without any issue).
>>> If the user does not check the errors, then the user
>>> has no one to blame but himself for missing errors.
>>
>> AVIO read/write behaviour is to store IO errors in the context so the
>> user does not have to check for them in every call. It is not well
>> documented which calls should be checked always, so the user might be
>> under the impression that errors during read/write may be checked
>> sometime later.
>>
>> Admittedly, ENOMEM is not an IO error, but I think it is better to
>> store that as well in the context to keep the behaviour consistent,
>> because in case of ENOMEM avio_read_to_bprint reads and drops
>> undefined amount of data, so the context will also be in an undefined
>> state.
>>
>> Other possibilities:
>> - make avio_read_to_bprint read all the data regardless of AVBPrint
>> fullness
>> - mark avio_read_to_bprint av_warn_unused_result.
>> - both :)
>>
>> But these also forces the user to check return values... So I kind of
>> like my original approach better, because it maintains avio_read/write
>> call behaviour that it is safe to check errors sometime later.
>
> Any more comments about this or the rest of the series? I plan to apply
> it tomorrow.
>
I still don't like storing ENOMEM in the AVIOContext and I don't see why
having to check the error is so burdensome. But if you want it so bad,
then go ahead.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
