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

[Anton Khirnov]

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.

-- 
Anton Khirnov