[FFmpeg-devel] [PATCH 03/24] avcodec/subtitles: Introduce new frame-based subtitle decoding API
Soft Works
softworkz at hotmail.com
Sat Jan 15 09:59:36 EET 2022
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Friday, January 14, 2022 6:54 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 03/24] avcodec/subtitles: Introduce new
> frame-based subtitle decoding API
>
> ffmpegagent:
> > From: softworkz <softworkz at hotmail.com>
> >
> > - Add avcodec_decode_subtitle3 which takes subtitle frames,
> > serving as compatibility shim to legacy subtitle decoding
> > - Add additional methods for conversion between old and new API
>
> This commit message is completely wrong.
>
> >
> > Signed-off-by: softworkz <softworkz at hotmail.com>
> > ---
> > libavcodec/avcodec.h | 8 +-
> > libavcodec/codec_desc.c | 11 +++
> > libavcodec/codec_desc.h | 8 ++
> > libavcodec/decode.c | 56 ++++++++++--
> > libavcodec/internal.h | 22 +++++
> > libavcodec/utils.c | 184 ++++++++++++++++++++++++++++++++++++++++
> > 6 files changed, 280 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index fe5a83cf85..9d59f6e840 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -1675,7 +1675,7 @@ typedef struct AVCodecContext {
> >
> > /**
> > * Header containing style information for text subtitles.
> > - * For SUBTITLE_ASS subtitle type, it should contain the whole ASS
> > + * For AV_SUBTITLE_FMT_ASS subtitle type, it should contain the whole
> ASS
> > * [Script Info] and [V4+ Styles] section, plus the [Events] line and
> > * the Format line following. It shouldn't include any Dialogue line.
> > * - encoding: Set/allocated/freed by user (before avcodec_open2())
> > @@ -2415,7 +2415,10 @@ int avcodec_close(AVCodecContext *avctx);
> > * Free all allocated data in the given subtitle struct.
> > *
> > * @param sub AVSubtitle to free.
> > + *
> > + * @deprecated Use the regular frame based encode and decode APIs instead.
> > */
> > +attribute_deprecated
> > void avsubtitle_free(AVSubtitle *sub);
> >
> > /**
> > @@ -2508,7 +2511,10 @@ enum AVChromaLocation avcodec_chroma_pos_to_enum(int
> xpos, int ypos);
> > * must be freed with avsubtitle_free if *got_sub_ptr is
> set.
> > * @param[in,out] got_sub_ptr Zero if no subtitle could be decompressed,
> otherwise, it is nonzero.
> > * @param[in] avpkt The input AVPacket containing the input buffer.
> > + *
> > + * @deprecated Use the new decode API (avcodec_send_packet,
> avcodec_receive_frame) instead.
> > */
> > +attribute_deprecated
> > int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
> > int *got_sub_ptr,
> > AVPacket *avpkt);
> > diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
> > index 0974ee03de..e48e4532ba 100644
> > --- a/libavcodec/codec_desc.c
> > +++ b/libavcodec/codec_desc.c
> > @@ -3548,3 +3548,14 @@ enum AVMediaType avcodec_get_type(enum AVCodecID
> codec_id)
> > const AVCodecDescriptor *desc = avcodec_descriptor_get(codec_id);
> > return desc ? desc->type : AVMEDIA_TYPE_UNKNOWN;
> > }
> > +
> > +enum AVSubtitleType avcodec_descriptor_get_subtitle_format(const
> AVCodecDescriptor *codec_descriptor)
> > +{
> > + if(codec_descriptor->props & AV_CODEC_PROP_BITMAP_SUB)
> > + return AV_SUBTITLE_FMT_BITMAP;
> > +
> > + if(codec_descriptor->props & AV_CODEC_PROP_TEXT_SUB)
> > + return AV_SUBTITLE_FMT_ASS;
> > +
> > + return AV_SUBTITLE_FMT_UNKNOWN;
> > +}
> > diff --git a/libavcodec/codec_desc.h b/libavcodec/codec_desc.h
> > index 126b52df47..ba68d24e0e 100644
> > --- a/libavcodec/codec_desc.h
> > +++ b/libavcodec/codec_desc.h
> > @@ -121,6 +121,14 @@ const AVCodecDescriptor *avcodec_descriptor_next(const
> AVCodecDescriptor *prev);
> > */
> > const AVCodecDescriptor *avcodec_descriptor_get_by_name(const char *name);
> >
> > +/**
> > + * Return subtitle format from a codec descriptor
> > + *
> > + * @param codec_descriptor codec descriptor
> > + * @return the subtitle type (e.g. bitmap, text)
> > + */
> > +enum AVSubtitleType avcodec_descriptor_get_subtitle_format(const
> AVCodecDescriptor *codec_descriptor);
> > +
> > /**
> > * @}
> > */
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index 0912f86a14..ab8a6ea6ff 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -576,6 +576,39 @@ static int
> decode_receive_frame_internal(AVCodecContext *avctx, AVFrame *frame)
> > return ret;
> > }
> >
> > +static int decode_subtitle2_priv(AVCodecContext *avctx, AVSubtitle *sub,
> > + int *got_sub_ptr, AVPacket *avpkt);
> > +
> > +static int decode_subtitle_shim(AVCodecContext *avctx, AVFrame *frame,
> AVPacket *avpkt)
> > +{
> > + int ret, got_sub_ptr = 0;
> > + AVSubtitle subtitle = { 0 };
> > +
> > + if (frame->buf[0])
> > + return AVERROR(EAGAIN);
> > +
> > + av_frame_unref(frame);
> > +
> > + ret = decode_subtitle2_priv(avctx, &subtitle, &got_sub_ptr, avpkt);
> > +
> > + if (ret >= 0 && got_sub_ptr) {
> > + frame->type = AVMEDIA_TYPE_SUBTITLE;
> > + frame->format = subtitle.format;
> > + ret = av_frame_get_buffer2(frame, 0);
> > +
> > + if (ret >= 0)
> > + ret = ff_frame_put_subtitle(frame, &subtitle);
> > +
> > + frame->width = avctx->width;
> > + frame->height = avctx->height;
> > + frame->pkt_dts = avpkt->dts;
> > + }
> > +
> > + avsubtitle_free(&subtitle);
> > +
> > + return ret;
> > +}
> > +
> > int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const
> AVPacket *avpkt)
> > {
> > AVCodecInternal *avci = avctx->internal;
> > @@ -590,6 +623,9 @@ int attribute_align_arg
> avcodec_send_packet(AVCodecContext *avctx, const AVPacke
> > if (avpkt && !avpkt->size && avpkt->data)
> > return AVERROR(EINVAL);
> >
> > + if (avctx->codec_type == AVMEDIA_TYPE_SUBTITLE)
> > + return decode_subtitle_shim(avctx, avci->buffer_frame, avpkt);
>
> With the avcodec_send_packet/avcodec_receive_frame API the user is
> supposed to send the packet only once (unless we have the EAGAIN case).
> Libavcodec takes care to buffer the packet in case this is necessary
> even if the packet leads to multiple returned frames. In contrast to
> this, the old API required the user to send the packet again, but with
> data offsetted and size decremented to reflect this.
This is only meant to be an interim solution until the decoders are
migrated to the frame based api.
> Some decoders (at least pgssubdec*) seem to be designed to only consume
> partial packets; if that is the case, then your approach above won't
> work and you will need to buffer the packet. In other words: The
> subtitle decoding codepath will become more like the codepaths for audio
> and video.
I (and many others) have run dozens of files with PGS subs with this code.
So, your assumption in the referenced commit (should not adversely affect the
decoder) is most likely right.
> The old API is btw even more broken, in particular in case the subtitles
> are recoded: Typically the size of the recoded UTF-8 is returned which
> is an API abuse on part of lavc. See
> https://github.com/mkver/FFmpeg/commit/ba1564c532654888015d67b70bf73d117c2d9f
> 75
> (part of https://github.com/mkver/FFmpeg/commits/subs) for more.
> *: It seems that libzvbi currently always fully consumes the AVPacket;
> yet looking at the history I don't know whether this is actually
> intended and correct.
I would suggest we focus on the future implementation (after migration) and
try to get that right.
>
> > +
> > av_packet_unref(avci->buffer_pkt);
> > if (avpkt && (avpkt->data || avpkt->side_data_elems)) {
> > ret = av_packet_ref(avci->buffer_pkt, avpkt);
> > @@ -651,7 +687,9 @@ int attribute_align_arg
> avcodec_receive_frame(AVCodecContext *avctx, AVFrame *fr
> >
> > if (avci->buffer_frame->buf[0]) {
> > av_frame_move_ref(frame, avci->buffer_frame);
> > - } else {
> > + } else if (avctx->codec_type == AVMEDIA_TYPE_SUBTITLE)
> > + return AVERROR(EAGAIN);
> > + else {
> > ret = decode_receive_frame_internal(avctx, frame);
> > if (ret < 0)
> > return ret;
> > @@ -802,9 +840,8 @@ static int utf8_check(const uint8_t *str)
> > return 1;
> > }
> >
> > -int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
> > - int *got_sub_ptr,
> > - AVPacket *avpkt)
> > +static int decode_subtitle2_priv(AVCodecContext *avctx, AVSubtitle *sub,
> > + int *got_sub_ptr, AVPacket *avpkt)
> > {
> > int ret = 0;
> >
> > @@ -850,10 +887,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx,
> AVSubtitle *sub,
> > avctx->pkt_timebase, ms);
> > }
> >
> > - if (avctx->codec_descriptor->props & AV_CODEC_PROP_BITMAP_SUB)
> > - sub->format = 0;
> > - else if (avctx->codec_descriptor->props & AV_CODEC_PROP_TEXT_SUB)
> > - sub->format = 1;
> > + sub->format = avcodec_descriptor_get_subtitle_format(avctx-
> >codec_descriptor);
> >
> > for (unsigned i = 0; i < sub->num_rects; i++) {
> > if (avctx->sub_charenc_mode != FF_SUB_CHARENC_MODE_IGNORE &&
> > @@ -874,6 +908,12 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx,
> AVSubtitle *sub,
> > return ret;
> > }
> >
> > +int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
> > + int *got_sub_ptr, AVPacket *avpkt)
> > +{
> > + return decode_subtitle2_priv(avctx, sub, got_sub_ptr, avpkt);
> > +}
> > +
> > enum AVPixelFormat avcodec_default_get_format(struct AVCodecContext
> *avctx,
> > const enum AVPixelFormat
> *fmt)
> > {
> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > index 72ca1553f6..ff63974f7f 100644
> > --- a/libavcodec/internal.h
> > +++ b/libavcodec/internal.h
> > @@ -363,4 +363,26 @@ int ff_int_from_list_or_default(void *ctx, const char
> * val_name, int val,
> >
> > void ff_dvdsub_parse_palette(uint32_t *palette, const char *p);
> >
> > +/**
> > + * Copies subtitle data from AVSubtitle to AVFrame.
> > + *
> > + * @deprecated This is a compatibility method for interoperability with
> > + * the legacy subtitle API.
> > + */
> > +int ff_frame_put_subtitle(AVFrame* frame, const AVSubtitle* sub);
> > +
> > +/**
> > + * Copies subtitle data from AVFrame to AVSubtitle.
> > + *
> > + * @deprecated This is a compatibility method for interoperability with
> > + * the legacy subtitle API.
> > + */
> > +int ff_frame_get_subtitle(AVSubtitle* sub, AVFrame* frame);
> > +
> > +#if defined(_WIN32) && CONFIG_SHARED && !defined(BUILDING_avcodec)
> > +# define av_export_avcodec __declspec(dllimport)
> > +#else
> > +# define av_export_avcodec
> > +#endif
>
> This seems like a rebase problem.
>
> > +
> > #endif /* AVCODEC_INTERNAL_H */
>
Yup, seems so - thanks.
sw
More information about the ffmpeg-devel
mailing list