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

[James Almer]

On 3/7/2024 9:18 AM, Anton Khirnov wrote:
> Quoting Andreas Rheinhardt (2024-03-07 12:19:28)
>> 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.
> 
> "Could not allocate frame side data" seems appropriate to me, it really
> is what happened, whatever the actual reason is.
> 
>> 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.
> 
> I'd be in favor, fewer functions is better.

I wouldn't. It adds an extra layer of abstraction for no real gain. And 
the name in this case is very specific and self explanatory in how it's 
different from the alternative.