| Message ID | 20230520015057.4589-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v2] avcodec/av1dec: convert to receive_frame() | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting James Almer (2023-05-20 03:50:57) > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/av1dec.c | 75 +++++++++++++++++++++++++++++++++------------ > libavcodec/av1dec.h | 4 +++ > 2 files changed, 60 insertions(+), 19 deletions(-) The patch makes the code longer and introduces an evil backward goto. So some comment on why is this an improvement would be appropriate.
On 5/20/2023 1:12 PM, Anton Khirnov wrote: > Quoting James Almer (2023-05-20 03:50:57) >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/av1dec.c | 75 +++++++++++++++++++++++++++++++++------------ >> libavcodec/av1dec.h | 4 +++ >> 2 files changed, 60 insertions(+), 19 deletions(-) > > The patch makes the code longer and introduces an evil backward goto. > So some comment on why is this an improvement would be appropriate. It's an improvement because it removes the auto-inserted av1_frame_split from the process, making the decoder handle temporal units with more than one frame in them on its own. I can add that to the commit message. The extra code is inevitable because it's the decoder who now needs to fetch the packet instead of the generic code in decode.c
James Almer: > On 5/20/2023 1:12 PM, Anton Khirnov wrote: >> Quoting James Almer (2023-05-20 03:50:57) >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> libavcodec/av1dec.c | 75 +++++++++++++++++++++++++++++++++------------ >>> libavcodec/av1dec.h | 4 +++ >>> 2 files changed, 60 insertions(+), 19 deletions(-) >> >> The patch makes the code longer and introduces an evil backward goto. >> So some comment on why is this an improvement would be appropriate. > > It's an improvement because it removes the auto-inserted av1_frame_split > from the process, making the decoder handle temporal units with more > than one frame in them on its own. I can add that to the commit message. > > The extra code is inevitable because it's the decoder who now needs to > fetch the packet instead of the generic code in decode.c 1. Where is the improvement in that? What is so bad about using a BSF to preprocess packets in this way? 2. Anyway, you did not remove the decoder->bsf configure dependency. - Andreas
On 5/20/2023 1:59 PM, Andreas Rheinhardt wrote: > James Almer: >> On 5/20/2023 1:12 PM, Anton Khirnov wrote: >>> Quoting James Almer (2023-05-20 03:50:57) >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> libavcodec/av1dec.c | 75 +++++++++++++++++++++++++++++++++------------ >>>> libavcodec/av1dec.h | 4 +++ >>>> 2 files changed, 60 insertions(+), 19 deletions(-) >>> >>> The patch makes the code longer and introduces an evil backward goto. >>> So some comment on why is this an improvement would be appropriate. >> >> It's an improvement because it removes the auto-inserted av1_frame_split >> from the process, making the decoder handle temporal units with more >> than one frame in them on its own. I can add that to the commit message. >> >> The extra code is inevitable because it's the decoder who now needs to >> fetch the packet instead of the generic code in decode.c > > 1. Where is the improvement in that? What is so bad about using a BSF to > preprocess packets in this way? Much less overhead? One less instance of CBS, less function calls, less packet references generated and moved around, etc. > 2. Anyway, you did not remove the decoder->bsf configure dependency. Will amend. > > - 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-request@ffmpeg.org with subject "unsubscribe".
James Almer: > On 5/20/2023 1:59 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 5/20/2023 1:12 PM, Anton Khirnov wrote: >>>> Quoting James Almer (2023-05-20 03:50:57) >>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>> --- >>>>> libavcodec/av1dec.c | 75 >>>>> +++++++++++++++++++++++++++++++++------------ >>>>> libavcodec/av1dec.h | 4 +++ >>>>> 2 files changed, 60 insertions(+), 19 deletions(-) >>>> >>>> The patch makes the code longer and introduces an evil backward goto. >>>> So some comment on why is this an improvement would be appropriate. >>> >>> It's an improvement because it removes the auto-inserted av1_frame_split >>> from the process, making the decoder handle temporal units with more >>> than one frame in them on its own. I can add that to the commit message. >>> >>> The extra code is inevitable because it's the decoder who now needs to >>> fetch the packet instead of the generic code in decode.c >> >> 1. Where is the improvement in that? What is so bad about using a BSF to >> preprocess packets in this way? > > Much less overhead? One less instance of CBS, less function calls, less > packet references generated and moved around, etc. > This is AV1, not annex B H.2645. - Andreas
On 5/20/2023 2:07 PM, Andreas Rheinhardt wrote: > James Almer: >> On 5/20/2023 1:59 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> On 5/20/2023 1:12 PM, Anton Khirnov wrote: >>>>> Quoting James Almer (2023-05-20 03:50:57) >>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>> --- >>>>>> libavcodec/av1dec.c | 75 >>>>>> +++++++++++++++++++++++++++++++++------------ >>>>>> libavcodec/av1dec.h | 4 +++ >>>>>> 2 files changed, 60 insertions(+), 19 deletions(-) >>>>> >>>>> The patch makes the code longer and introduces an evil backward goto. >>>>> So some comment on why is this an improvement would be appropriate. >>>> >>>> It's an improvement because it removes the auto-inserted av1_frame_split >>>> from the process, making the decoder handle temporal units with more >>>> than one frame in them on its own. I can add that to the commit message. >>>> >>>> The extra code is inevitable because it's the decoder who now needs to >>>> fetch the packet instead of the generic code in decode.c >>> >>> 1. Where is the improvement in that? What is so bad about using a BSF to >>> preprocess packets in this way? >> >> Much less overhead? One less instance of CBS, less function calls, less >> packet references generated and moved around, etc. >> > > This is AV1, not annex B H.2645. I'm sorry, i don't understand what you're trying to say. The av1_frame_split bsf will split a TU into individual frames, and uses the CBS framework for this. It's an unnecessary overhead when the decoder can be made to handle this on its own by making it use the proper decode callback.
James Almer: > On 5/20/2023 2:07 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 5/20/2023 1:59 PM, Andreas Rheinhardt wrote: >>>> James Almer: >>>>> On 5/20/2023 1:12 PM, Anton Khirnov wrote: >>>>>> Quoting James Almer (2023-05-20 03:50:57) >>>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>>> --- >>>>>>> libavcodec/av1dec.c | 75 >>>>>>> +++++++++++++++++++++++++++++++++------------ >>>>>>> libavcodec/av1dec.h | 4 +++ >>>>>>> 2 files changed, 60 insertions(+), 19 deletions(-) >>>>>> >>>>>> The patch makes the code longer and introduces an evil backward goto. >>>>>> So some comment on why is this an improvement would be appropriate. >>>>> >>>>> It's an improvement because it removes the auto-inserted >>>>> av1_frame_split >>>>> from the process, making the decoder handle temporal units with more >>>>> than one frame in them on its own. I can add that to the commit >>>>> message. >>>>> >>>>> The extra code is inevitable because it's the decoder who now needs to >>>>> fetch the packet instead of the generic code in decode.c >>>> >>>> 1. Where is the improvement in that? What is so bad about using a >>>> BSF to >>>> preprocess packets in this way? >>> >>> Much less overhead? One less instance of CBS, less function calls, less >>> packet references generated and moved around, etc. >>> >> >> This is AV1, not annex B H.2645. > > I'm sorry, i don't understand what you're trying to say. The > av1_frame_split bsf will split a TU into individual frames, and uses the > CBS framework for this. It's an unnecessary overhead when the decoder > can be made to handle this on its own by making it use the proper decode > callback. I meant to say: Parsing AV1 is easy and fast, in contrast to H.2645 (in particular, annex B H.2645). - Andreas
On 5/20/2023 2:17 PM, Andreas Rheinhardt wrote: > James Almer: >> On 5/20/2023 2:07 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> On 5/20/2023 1:59 PM, Andreas Rheinhardt wrote: >>>>> James Almer: >>>>>> On 5/20/2023 1:12 PM, Anton Khirnov wrote: >>>>>>> Quoting James Almer (2023-05-20 03:50:57) >>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>>>>>> --- >>>>>>>> libavcodec/av1dec.c | 75 >>>>>>>> +++++++++++++++++++++++++++++++++------------ >>>>>>>> libavcodec/av1dec.h | 4 +++ >>>>>>>> 2 files changed, 60 insertions(+), 19 deletions(-) >>>>>>> >>>>>>> The patch makes the code longer and introduces an evil backward goto. >>>>>>> So some comment on why is this an improvement would be appropriate. >>>>>> >>>>>> It's an improvement because it removes the auto-inserted >>>>>> av1_frame_split >>>>>> from the process, making the decoder handle temporal units with more >>>>>> than one frame in them on its own. I can add that to the commit >>>>>> message. >>>>>> >>>>>> The extra code is inevitable because it's the decoder who now needs to >>>>>> fetch the packet instead of the generic code in decode.c >>>>> >>>>> 1. Where is the improvement in that? What is so bad about using a >>>>> BSF to >>>>> preprocess packets in this way? >>>> >>>> Much less overhead? One less instance of CBS, less function calls, less >>>> packet references generated and moved around, etc. >>>> >>> >>> This is AV1, not annex B H.2645. >> >> I'm sorry, i don't understand what you're trying to say. The >> av1_frame_split bsf will split a TU into individual frames, and uses the >> CBS framework for this. It's an unnecessary overhead when the decoder >> can be made to handle this on its own by making it use the proper decode >> callback. > > I meant to say: Parsing AV1 is easy and fast, in contrast to H.2645 (in > particular, annex B H.2645). Sure, but it's still an unnecessary overhead. Right now, the parser spawns a CBS instance, and the decoder two. I'm removing the one that's superfluous.
On Fri, May 19, 2023 at 10:50:57PM -0300, James Almer wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavcodec/av1dec.c | 75 +++++++++++++++++++++++++++++++++------------ > libavcodec/av1dec.h | 4 +++ > 2 files changed, 60 insertions(+), 19 deletions(-) Crashes intermittently (so maybe its not this one) [av1 @ 0x562d7e65db40] No sequence header available. [av1 @ 0x562d7e65db40] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment. [av1 @ 0x562d7e65db40] Failed to parse temporal unit. [av1 @ 0x562d7e65db40] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment. [av1 @ 0x562d7e65db40] Failed to read packet. Assertion i <= s->current_obu.nb_units failed at libavcodec/av1dec.c:1415 Aborted (core dumped) #0 0x00007fffed2e2e87 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 #1 0x00007fffed2e47f1 in __GI_abort () at abort.c:79 #2 0x0000555555bfcf4a in av1_receive_frame () #3 0x0000555555c7d814 in decode_receive_frame_internal () #4 0x0000555555c7e4c0 in avcodec_send_packet () #5 0x0000555555a5d633 in try_decode_frame () #6 0x0000555555a628ff in avformat_find_stream_info () #7 0x000055555574578e in ifile_open () #8 0x000055555575c99c in open_files.isra () #9 0x000055555575ddb5 in ffmpeg_parse_options () #10 0x000055555573df61 in main () if i build with full debug symbols it doesnt crash :) ill mail you the file [...]
On 5/20/2023 2:23 PM, Michael Niedermayer wrote: > On Fri, May 19, 2023 at 10:50:57PM -0300, James Almer wrote: >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavcodec/av1dec.c | 75 +++++++++++++++++++++++++++++++++------------ >> libavcodec/av1dec.h | 4 +++ >> 2 files changed, 60 insertions(+), 19 deletions(-) > > Crashes intermittently (so maybe its not this one) > > [av1 @ 0x562d7e65db40] No sequence header available. > [av1 @ 0x562d7e65db40] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment. > [av1 @ 0x562d7e65db40] Failed to parse temporal unit. > [av1 @ 0x562d7e65db40] Invalid OBU length: 1071323, but only 768 bytes remaining in fragment. > [av1 @ 0x562d7e65db40] Failed to read packet. > Assertion i <= s->current_obu.nb_units failed at libavcodec/av1dec.c:1415 > Aborted (core dumped) > > #0 0x00007fffed2e2e87 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51 > #1 0x00007fffed2e47f1 in __GI_abort () at abort.c:79 > #2 0x0000555555bfcf4a in av1_receive_frame () > #3 0x0000555555c7d814 in decode_receive_frame_internal () > #4 0x0000555555c7e4c0 in avcodec_send_packet () > #5 0x0000555555a5d633 in try_decode_frame () > #6 0x0000555555a628ff in avformat_find_stream_info () > #7 0x000055555574578e in ifile_open () > #8 0x000055555575c99c in open_files.isra () > #9 0x000055555575ddb5 in ffmpeg_parse_options () > #10 0x000055555573df61 in main () > > if i build with full debug symbols it doesnt crash :) > ill mail you the file "i" was uninitialized because the for loop was never reached. Fixed locally. > > > [...] > > > _______________________________________________ > 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/av1dec.c b/libavcodec/av1dec.c index d46ee48335..53ed37b817 100644 --- a/libavcodec/av1dec.c +++ b/libavcodec/av1dec.c @@ -32,6 +32,7 @@ #include "bytestream.h" #include "codec_internal.h" #include "decode.h" +#include "internal.h" #include "hwconfig.h" #include "profiles.h" #include "thread.h" @@ -760,6 +761,7 @@ static const CodedBitstreamUnitType decompose_unit_types[] = { static av_cold int av1_decode_init(AVCodecContext *avctx) { + AVCodecInternal *avci = avctx->internal; AV1DecContext *s = avctx->priv_data; AV1RawSequenceHeader *seq; int ret; @@ -767,6 +769,8 @@ static av_cold int av1_decode_init(AVCodecContext *avctx) s->avctx = avctx; s->pix_fmt = AV_PIX_FMT_NONE; + s->pkt = avci->in_pkt; + for (int i = 0; i < FF_ARRAY_ELEMS(s->ref); i++) { s->ref[i].f = av_frame_alloc(); if (!s->ref[i].f) { @@ -1041,11 +1045,11 @@ static int export_film_grain(AVCodecContext *avctx, AVFrame *frame) return 0; } -static int set_output_frame(AVCodecContext *avctx, AVFrame *frame, - const AVPacket *pkt, int *got_frame) +static int set_output_frame(AVCodecContext *avctx, AVFrame *frame) { AV1DecContext *s = avctx->priv_data; const AVFrame *srcframe = s->cur_frame.f; + AVPacket *pkt = s->pkt; int ret; // TODO: all layers @@ -1079,7 +1083,7 @@ FF_DISABLE_DEPRECATION_WARNINGS FF_ENABLE_DEPRECATION_WARNINGS #endif - *got_frame = 1; + av_packet_unref(pkt); return 0; } @@ -1145,22 +1149,32 @@ static int get_current_frame(AVCodecContext *avctx) return ret; } -static int av1_decode_frame(AVCodecContext *avctx, AVFrame *frame, - int *got_frame, AVPacket *pkt) +static int av1_receive_frame(AVCodecContext *avctx, AVFrame *frame) { AV1DecContext *s = avctx->priv_data; AV1RawTileGroup *raw_tile_group = NULL; - int ret; + int i, ret; - ret = ff_cbs_read_packet(s->cbc, &s->current_obu, pkt); - if (ret < 0) { - av_log(avctx, AV_LOG_ERROR, "Failed to read packet.\n"); - goto end; +again: + if (!s->current_obu.nb_units) { + av_packet_unref(s->pkt); + ret = ff_decode_get_packet(avctx, s->pkt); + if (ret < 0) + return ret; + + ret = ff_cbs_read_packet(s->cbc, &s->current_obu, s->pkt); + if (ret < 0) { + av_log(avctx, AV_LOG_ERROR, "Failed to read packet.\n"); + goto end; + } + + s->nb_unit = 0; + + av_log(avctx, AV_LOG_DEBUG, "Total OBUs on this packet:%d.\n", + s->current_obu.nb_units); } - av_log(avctx, AV_LOG_DEBUG, "Total obu for this frame:%d.\n", - s->current_obu.nb_units); - for (int i = 0; i < s->current_obu.nb_units; i++) { + for (i = s->nb_unit; i < s->current_obu.nb_units; i++) { CodedBitstreamUnit *unit = &s->current_obu.units[i]; AV1RawOBU *obu = unit->content; const AV1RawOBUHeader *header; @@ -1168,6 +1182,7 @@ static int av1_decode_frame(AVCodecContext *avctx, AVFrame *frame, if (!obu) continue; + ret = 0; header = &obu->header; av_log(avctx, AV_LOG_DEBUG, "Obu idx:%d, obu type:%d.\n", i, unit->type); @@ -1251,13 +1266,15 @@ static int av1_decode_frame(AVCodecContext *avctx, AVFrame *frame, goto end; } + ret = 0; if (s->cur_frame.f->buf[0]) { - ret = set_output_frame(avctx, frame, pkt, got_frame); + ret = set_output_frame(avctx, frame); if (ret < 0) av_log(avctx, AV_LOG_ERROR, "Set output frame error.\n"); } s->raw_frame_header = NULL; + i++; goto end; } @@ -1361,6 +1378,7 @@ static int av1_decode_frame(AVCodecContext *avctx, AVFrame *frame, } if (raw_tile_group && (s->tile_num == raw_tile_group->tg_end + 1)) { + int show_frame = s->raw_frame_header->show_frame; if (avctx->hwaccel && s->cur_frame.f->buf[0]) { ret = avctx->hwaccel->end_frame(avctx); if (ret < 0) { @@ -1375,8 +1393,9 @@ static int av1_decode_frame(AVCodecContext *avctx, AVFrame *frame, goto end; } + ret = 0; if (s->raw_frame_header->show_frame && s->cur_frame.f->buf[0]) { - ret = set_output_frame(avctx, frame, pkt, got_frame); + ret = set_output_frame(avctx, frame); if (ret < 0) { av_log(avctx, AV_LOG_ERROR, "Set output frame error\n"); goto end; @@ -1384,13 +1403,30 @@ static int av1_decode_frame(AVCodecContext *avctx, AVFrame *frame, } raw_tile_group = NULL; s->raw_frame_header = NULL; + if (show_frame) { + i++; + goto end; + } } + ret = AVERROR(EAGAIN); } end: - ff_cbs_fragment_reset(&s->current_obu); - if (ret < 0) + av_assert0(i <= s->current_obu.nb_units); + s->nb_unit = i; + + if (s->current_obu.nb_units == i) { + ff_cbs_fragment_reset(&s->current_obu); + s->nb_unit = 0; + } + if (ret == AVERROR(EAGAIN)) + goto again; + if (ret < 0) { s->raw_frame_header = NULL; + ff_cbs_fragment_reset(&s->current_obu); + s->nb_unit = 0; + } + return ret; } @@ -1404,6 +1440,7 @@ static void av1_decode_flush(AVCodecContext *avctx) av1_frame_unref(avctx, &s->cur_frame); s->operating_point_idc = 0; + s->nb_unit = 0; s->raw_frame_header = NULL; s->raw_seq = NULL; s->cll = NULL; @@ -1411,6 +1448,7 @@ static void av1_decode_flush(AVCodecContext *avctx) while (av_fifo_read(s->itut_t35_fifo, &itut_t35, 1) >= 0) av_buffer_unref(&itut_t35.payload_ref); + ff_cbs_fragment_reset(&s->current_obu); ff_cbs_flush(s->cbc); } @@ -1437,14 +1475,13 @@ const FFCodec ff_av1_decoder = { .priv_data_size = sizeof(AV1DecContext), .init = av1_decode_init, .close = av1_decode_free, - FF_CODEC_DECODE_CB(av1_decode_frame), + FF_CODEC_RECEIVE_FRAME_CB(av1_receive_frame), .p.capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_AVOID_PROBING, .caps_internal = FF_CODEC_CAP_INIT_CLEANUP | FF_CODEC_CAP_SETS_PKT_DTS, .flush = av1_decode_flush, .p.profiles = NULL_IF_CONFIG_SMALL(ff_av1_profiles), .p.priv_class = &av1_class, - .bsfs = "av1_frame_split", .hw_configs = (const AVCodecHWConfigInternal *const []) { #if CONFIG_AV1_DXVA2_HWACCEL HWACCEL_DXVA2(av1), diff --git a/libavcodec/av1dec.h b/libavcodec/av1dec.h index cef899f81f..59ffed1d9b 100644 --- a/libavcodec/av1dec.h +++ b/libavcodec/av1dec.h @@ -28,6 +28,7 @@ #include "libavutil/frame.h" #include "libavutil/pixfmt.h" #include "avcodec.h" +#include "packet.h" #include "cbs.h" #include "cbs_av1.h" @@ -68,6 +69,7 @@ typedef struct AV1DecContext { enum AVPixelFormat pix_fmt; CodedBitstreamContext *cbc; CodedBitstreamFragment current_obu; + AVPacket *pkt; AVBufferRef *seq_ref; AV1RawSequenceHeader *raw_seq; @@ -90,6 +92,8 @@ typedef struct AV1DecContext { AV1Frame ref[AV1_NUM_REF_FRAMES]; AV1Frame cur_frame; + int nb_unit; + // AVOptions int operating_point; } AV1DecContext;
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/av1dec.c | 75 +++++++++++++++++++++++++++++++++------------ libavcodec/av1dec.h | 4 +++ 2 files changed, 60 insertions(+), 19 deletions(-)