[FFmpeg-devel] [PATCH 23/29] avcodec/mpeg12dec: use ff_frame_new_side_data

[Andreas Rheinhardt]

Anton Khirnov:
> Quoting Andreas Rheinhardt (2024-03-04 14:36:09)
>> Anton Khirnov:
>>> From: Niklas Haas <git at haasn.dev>
>>>
>>> For consistency, even though this cannot be overriden at the packet
>>> level.
>>> ---
>>>  libavcodec/mpeg12dec.c | 18 ++++++++++--------
>>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
>>> index 3a2f17e508..aa116336dd 100644
>>> --- a/libavcodec/mpeg12dec.c
>>> +++ b/libavcodec/mpeg12dec.c
>>> @@ -2531,15 +2531,17 @@ static int mpeg_decode_frame(AVCodecContext *avctx, AVFrame *picture,
>>>  
>>>          if (s->timecode_frame_start != -1 && *got_output) {
>>>              char tcbuf[AV_TIMECODE_STR_SIZE];
>>> -            AVFrameSideData *tcside = av_frame_new_side_data(picture,
>>> -                                                             AV_FRAME_DATA_GOP_TIMECODE,
>>> -                                                             sizeof(int64_t));
>>> -            if (!tcside)
>>> -                return AVERROR(ENOMEM);
>>> -            memcpy(tcside->data, &s->timecode_frame_start, sizeof(int64_t));
>>> +            AVFrameSideData *tcside;
>>> +            ret = ff_frame_new_side_data(avctx, picture, AV_FRAME_DATA_GOP_TIMECODE,
>>> +                                         sizeof(int64_t), &tcside);
>>> +            if (ret < 0)
>>> +                return ret;
>>> +            if (tcside) {
>>> +                memcpy(tcside->data, &s->timecode_frame_start, sizeof(int64_t));
>>>  
>>> -            av_timecode_make_mpeg_tc_string(tcbuf, s->timecode_frame_start);
>>> -            av_dict_set(&picture->metadata, "timecode", tcbuf, 0);
>>> +                av_timecode_make_mpeg_tc_string(tcbuf, s->timecode_frame_start);
>>> +                av_dict_set(&picture->metadata, "timecode", tcbuf, 0);
>>> +            }
>>>  
>>>              s->timecode_frame_start = -1;
>>>          }
>>
>> -1 to everything that is only done for consistency.
> 
> I prefer consistency here, otherwise the decoder authors have to choose
> which function to use, and they are often not aware of the precise
> implications of thise choice. Better to always use just one function.
> 

It adds unnecessary checks and given that internal API is updated more
frequently it is likely to lead to unnecessary further changes lateron.
Furthermore, mjpeg still emits an allocation failure error message
without knowing whether it was really allocation failure.
Finally, if we really believed decoder authors to be that uninformed, we
should remove ff_get_buffer() from decoders altogether and only use the
ff_thread_get_buffer() wrapper. Somehow I don't remember this difference
to ever be a problem.

- Andreas