| Message ID | 20240304130657.30631-23-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/29] lavu/opt: factor per-type dispatch out of av_opt_get() | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Anton Khirnov: > From: Niklas Haas <git@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. - Andreas
Quoting Andreas Rheinhardt (2024-03-04 14:36:09) > Anton Khirnov: > > From: Niklas Haas <git@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.
Anton Khirnov: > Quoting Andreas Rheinhardt (2024-03-04 14:36:09) >> Anton Khirnov: >>> From: Niklas Haas <git@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
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@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.
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@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.
Quoting James Almer (2024-03-07 13:25:07) > I wouldn't. It adds an extra layer of abstraction for no real gain. The gain is simpler API. What's the gain in forcing decoder authors to call a different function to accomplish the same task, just because frame threading happens to be used. > And the name in this case is very specific and self explanatory in how > it's different from the alternative. From decoder POV it's not different at all, it does exactly the same thing.
On Tue, 05 Mar 2024 11:00:02 +0100 Anton Khirnov <anton@khirnov.net> wrote: > 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. Regardless of my personal preference, these (consistency) patches are strictly optional and do not gain us any functionality. Given that this is blocking the (by now fairly urgent) 7.0 release, I am in favor of dropping them if that helps speed the process along by avoiding unnecessary discussion where it is not of high priority. > -- > Anton Khirnov > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
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; }
From: Niklas Haas <git@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(-)