| Message ID | 20231231123018.59035-3-thilo.borgmann@mail.de |
|---|---|
| State | New |
| Headers | show |
| Series | webp: add support for animated WebP decoding | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Quoting Thilo Borgmann via ffmpeg-devel (2023-12-31 13:30:14) > --- > libavcodec/webp.c | 50 +++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 44 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/webp.c b/libavcodec/webp.c > index 4fd107aa0c..58a20b73da 100644 > --- a/libavcodec/webp.c > +++ b/libavcodec/webp.c > @@ -194,6 +194,7 @@ typedef struct WebPContext { > AVFrame *alpha_frame; /* AVFrame for alpha data decompressed from VP8L */ > AVPacket *pkt; /* AVPacket to be passed to the underlying VP8 decoder */ > AVCodecContext *avctx; /* parent AVCodecContext */ > + AVCodecContext *avctx_vp8; /* wrapper context for VP8 decoder */ Nested codec contexts are in general highly undesirable and should be avoided whenever possible.
Am 25.01.24 um 11:04 schrieb Anton Khirnov: > Quoting Thilo Borgmann via ffmpeg-devel (2023-12-31 13:30:14) >> --- >> libavcodec/webp.c | 50 +++++++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 44 insertions(+), 6 deletions(-) >> >> diff --git a/libavcodec/webp.c b/libavcodec/webp.c >> index 4fd107aa0c..58a20b73da 100644 >> --- a/libavcodec/webp.c >> +++ b/libavcodec/webp.c >> @@ -194,6 +194,7 @@ typedef struct WebPContext { >> AVFrame *alpha_frame; /* AVFrame for alpha data decompressed from VP8L */ >> AVPacket *pkt; /* AVPacket to be passed to the underlying VP8 decoder */ >> AVCodecContext *avctx; /* parent AVCodecContext */ >> + AVCodecContext *avctx_vp8; /* wrapper context for VP8 decoder */ > > Nested codec contexts are in general highly undesirable and should be > avoided whenever possible. AFAICT we do it that way in the other codecs as well (cri, ftr, imm5, tdsc, tiff). So what do you suggest to do to avoid having it nested? -Thilo
Quoting Thilo Borgmann via ffmpeg-devel (2024-01-25 16:39:19) > Am 25.01.24 um 11:04 schrieb Anton Khirnov: > > Quoting Thilo Borgmann via ffmpeg-devel (2023-12-31 13:30:14) > >> --- > >> libavcodec/webp.c | 50 +++++++++++++++++++++++++++++++++++++++++------ > >> 1 file changed, 44 insertions(+), 6 deletions(-) > >> > >> diff --git a/libavcodec/webp.c b/libavcodec/webp.c > >> index 4fd107aa0c..58a20b73da 100644 > >> --- a/libavcodec/webp.c > >> +++ b/libavcodec/webp.c > >> @@ -194,6 +194,7 @@ typedef struct WebPContext { > >> AVFrame *alpha_frame; /* AVFrame for alpha data decompressed from VP8L */ > >> AVPacket *pkt; /* AVPacket to be passed to the underlying VP8 decoder */ > >> AVCodecContext *avctx; /* parent AVCodecContext */ > >> + AVCodecContext *avctx_vp8; /* wrapper context for VP8 decoder */ > > > > Nested codec contexts are in general highly undesirable and should be > > avoided whenever possible. > > AFAICT we do it that way in the other codecs as well (cri, ftr, imm5, > tdsc, tiff). So what do you suggest to do to avoid having it nested? Integrating the two decoders directly, as is done now. With nesting it is very tricky to handle all the corner cases properly, especially passing through all the options to the innner decoder, like direct rendering, other user callbacks, etc. It should only be done as a last resort and there should be a strong argument to do it this way.
Am 28.01.24 um 11:29 schrieb Anton Khirnov: > Quoting Thilo Borgmann via ffmpeg-devel (2024-01-25 16:39:19) >> Am 25.01.24 um 11:04 schrieb Anton Khirnov: >>> Quoting Thilo Borgmann via ffmpeg-devel (2023-12-31 13:30:14) >>>> --- >>>> libavcodec/webp.c | 50 +++++++++++++++++++++++++++++++++++++++++------ >>>> 1 file changed, 44 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/libavcodec/webp.c b/libavcodec/webp.c >>>> index 4fd107aa0c..58a20b73da 100644 >>>> --- a/libavcodec/webp.c >>>> +++ b/libavcodec/webp.c >>>> @@ -194,6 +194,7 @@ typedef struct WebPContext { >>>> AVFrame *alpha_frame; /* AVFrame for alpha data decompressed from VP8L */ >>>> AVPacket *pkt; /* AVPacket to be passed to the underlying VP8 decoder */ >>>> AVCodecContext *avctx; /* parent AVCodecContext */ >>>> + AVCodecContext *avctx_vp8; /* wrapper context for VP8 decoder */ >>> >>> Nested codec contexts are in general highly undesirable and should be >>> avoided whenever possible. >> >> AFAICT we do it that way in the other codecs as well (cri, ftr, imm5, >> tdsc, tiff). So what do you suggest to do to avoid having it nested? > > Integrating the two decoders directly, as is done now. > > With nesting it is very tricky to handle all the corner cases properly, > especially passing through all the options to the innner decoder, like > direct rendering, other user callbacks, etc. It should only be done as a > last resort and there should be a strong argument to do it this way. IIUC that was what the patch still did some some versions ago. It brought us the data races in animated files, decoupling the decoder solving the issue. -Thilo
Thilo Borgmann via ffmpeg-devel: > Am 28.01.24 um 11:29 schrieb Anton Khirnov: >> Quoting Thilo Borgmann via ffmpeg-devel (2024-01-25 16:39:19) >>> Am 25.01.24 um 11:04 schrieb Anton Khirnov: >>>> Quoting Thilo Borgmann via ffmpeg-devel (2023-12-31 13:30:14) >>>>> --- >>>>> libavcodec/webp.c | 50 >>>>> +++++++++++++++++++++++++++++++++++++++++------ >>>>> 1 file changed, 44 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/libavcodec/webp.c b/libavcodec/webp.c >>>>> index 4fd107aa0c..58a20b73da 100644 >>>>> --- a/libavcodec/webp.c >>>>> +++ b/libavcodec/webp.c >>>>> @@ -194,6 +194,7 @@ typedef struct WebPContext { >>>>> AVFrame *alpha_frame; /* AVFrame for alpha >>>>> data decompressed from VP8L */ >>>>> AVPacket *pkt; /* AVPacket to be passed >>>>> to the underlying VP8 decoder */ >>>>> AVCodecContext *avctx; /* parent AVCodecContext */ >>>>> + AVCodecContext *avctx_vp8; /* wrapper context for VP8 >>>>> decoder */ >>>> >>>> Nested codec contexts are in general highly undesirable and should be >>>> avoided whenever possible. >>> >>> AFAICT we do it that way in the other codecs as well (cri, ftr, imm5, >>> tdsc, tiff). So what do you suggest to do to avoid having it nested? >> >> Integrating the two decoders directly, as is done now. >> >> With nesting it is very tricky to handle all the corner cases properly, >> especially passing through all the options to the innner decoder, like >> direct rendering, other user callbacks, etc. It should only be done as a >> last resort and there should be a strong argument to do it this way. > > IIUC that was what the patch still did some some versions ago. > It brought us the data races in animated files, decoupling the decoder > solving the issue. > If one keeps the codecs integrated, then one needs at the very least change the check for whether to call ff_thread_finish_setup() as I did: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-January/320490.html This will not be enough: E.g. changing the dimensions in VP8 code and then reverting that change in WebP (as has been done in the earlier version of your patch which made me propose that these decoders should be separated) will have to be avoided. - Andreas
On 03.02.24 14:53, Andreas Rheinhardt wrote: > Thilo Borgmann via ffmpeg-devel: >> Am 28.01.24 um 11:29 schrieb Anton Khirnov: >>> Quoting Thilo Borgmann via ffmpeg-devel (2024-01-25 16:39:19) >>>> Am 25.01.24 um 11:04 schrieb Anton Khirnov: >>>>> Quoting Thilo Borgmann via ffmpeg-devel (2023-12-31 13:30:14) >>>>>> --- >>>>>> libavcodec/webp.c | 50 >>>>>> +++++++++++++++++++++++++++++++++++++++++------ >>>>>> 1 file changed, 44 insertions(+), 6 deletions(-) >>>>>> >>>>>> diff --git a/libavcodec/webp.c b/libavcodec/webp.c >>>>>> index 4fd107aa0c..58a20b73da 100644 >>>>>> --- a/libavcodec/webp.c >>>>>> +++ b/libavcodec/webp.c >>>>>> @@ -194,6 +194,7 @@ typedef struct WebPContext { >>>>>> AVFrame *alpha_frame; /* AVFrame for alpha >>>>>> data decompressed from VP8L */ >>>>>> AVPacket *pkt; /* AVPacket to be passed >>>>>> to the underlying VP8 decoder */ >>>>>> AVCodecContext *avctx; /* parent AVCodecContext */ >>>>>> + AVCodecContext *avctx_vp8; /* wrapper context for VP8 >>>>>> decoder */ >>>>> >>>>> Nested codec contexts are in general highly undesirable and should be >>>>> avoided whenever possible. >>>> >>>> AFAICT we do it that way in the other codecs as well (cri, ftr, imm5, >>>> tdsc, tiff). So what do you suggest to do to avoid having it nested? >>> >>> Integrating the two decoders directly, as is done now. >>> >>> With nesting it is very tricky to handle all the corner cases properly, >>> especially passing through all the options to the innner decoder, like >>> direct rendering, other user callbacks, etc. It should only be done as a >>> last resort and there should be a strong argument to do it this way. >> >> IIUC that was what the patch still did some some versions ago. >> It brought us the data races in animated files, decoupling the decoder >> solving the issue. >> > > If one keeps the codecs integrated, then one needs at the very least > change the check for whether to call ff_thread_finish_setup() as I did: > https://ffmpeg.org/pipermail/ffmpeg-devel/2024-January/320490.html > This will not be enough: E.g. changing the dimensions in VP8 code and > then reverting that change in WebP (as has been done in the earlier > version of your patch which made me propose that these decoders should > be separated) will have to be avoided. I've a version of the animated webp decoder with coupled vp8 decoder doing that size change and tsan is happy for me. I had the impression ff_thread_finish_setup() blew it in the past which is now avoided - am I wrong? Once your patches landed I'll post v10 and we can check that again. -Thilo
diff --git a/libavcodec/webp.c b/libavcodec/webp.c index 4fd107aa0c..58a20b73da 100644 --- a/libavcodec/webp.c +++ b/libavcodec/webp.c @@ -194,6 +194,7 @@ typedef struct WebPContext { AVFrame *alpha_frame; /* AVFrame for alpha data decompressed from VP8L */ AVPacket *pkt; /* AVPacket to be passed to the underlying VP8 decoder */ AVCodecContext *avctx; /* parent AVCodecContext */ + AVCodecContext *avctx_vp8; /* wrapper context for VP8 decoder */ int initialized; /* set once the VP8 context is initialized */ int has_alpha; /* has a separate alpha chunk */ enum AlphaCompression alpha_compression; /* compression type for alpha chunk */ @@ -1298,12 +1299,13 @@ static int vp8_lossy_decode_frame(AVCodecContext *avctx, AVFrame *p, int ret; if (!s->initialized) { - ff_vp8_decode_init(avctx); + VP8Context *s_vp8 = s->avctx_vp8->priv_data; + s_vp8->actually_webp = 1; s->initialized = 1; - s->v.actually_webp = 1; } avctx->pix_fmt = s->has_alpha ? AV_PIX_FMT_YUVA420P : AV_PIX_FMT_YUV420P; s->lossless = 0; + s->avctx_vp8->pix_fmt = avctx->pix_fmt; if (data_size > INT_MAX) { av_log(avctx, AV_LOG_ERROR, "unsupported chunk size\n"); @@ -1314,14 +1316,32 @@ static int vp8_lossy_decode_frame(AVCodecContext *avctx, AVFrame *p, s->pkt->data = data_start; s->pkt->size = data_size; - ret = ff_vp8_decode_frame(avctx, p, got_frame, s->pkt); - if (ret < 0) + ret = avcodec_send_packet(s->avctx_vp8, s->pkt); + if (ret < 0) { + av_log(avctx, AV_LOG_ERROR, "Error submitting a packet for decoding\n"); return ret; + } - if (!*got_frame) + ret = avcodec_receive_frame(s->avctx_vp8, p); + if (ret < 0) { + av_log(avctx, AV_LOG_ERROR, "VP8 decoding error: %s.\n", av_err2str(ret)); return AVERROR_INVALIDDATA; + } + + ret = ff_decode_frame_props(avctx, p); + if (ret < 0) { + return ret; + } + + if (!p->private_ref) { + ret = ff_attach_decode_data(p); + if (ret < 0) { + return ret; + } + } - update_canvas_size(avctx, avctx->width, avctx->height); + *got_frame = 1; + update_canvas_size(avctx, s->avctx_vp8->width, s->avctx_vp8->height); if (s->has_alpha) { ret = vp8_lossy_decode_alpha(avctx, p, s->alpha_data, @@ -1533,11 +1553,28 @@ exif_end: static av_cold int webp_decode_init(AVCodecContext *avctx) { WebPContext *s = avctx->priv_data; + int ret; + const AVCodec *codec; s->pkt = av_packet_alloc(); if (!s->pkt) return AVERROR(ENOMEM); + + /* Prepare everything needed for VP8 decoding */ + codec = avcodec_find_decoder(AV_CODEC_ID_VP8); + if (!codec) + return AVERROR_BUG; + s->avctx_vp8 = avcodec_alloc_context3(codec); + if (!s->avctx_vp8) + return AVERROR(ENOMEM); + s->avctx_vp8->flags = avctx->flags; + s->avctx_vp8->flags2 = avctx->flags2; + s->avctx_vp8->pix_fmt = avctx->pix_fmt; + ret = avcodec_open2(s->avctx_vp8, codec, NULL); + if (ret < 0) { + return ret; + } return 0; } @@ -1546,6 +1583,7 @@ static av_cold int webp_decode_close(AVCodecContext *avctx) WebPContext *s = avctx->priv_data; av_packet_free(&s->pkt); + avcodec_free_context(&s->avctx_vp8); if (s->initialized) return ff_vp8_decode_free(avctx);