[FFmpeg-devel] [PATCH v15 01/16] global: Prepare AVFrame for subtitle handling

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Anton
> Khirnov
> Sent: Thursday, November 25, 2021 4:56 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v15 01/16] global: Prepare AVFrame for
> subtitle handling
> 
> Quoting Soft Works (2021-11-25 01:48:11)
> > Root commit for adding subtitle filtering capabilities.
> > In detail:
> >
> > - Add type (AVMediaType) field to AVFrame
> >   Replaces previous way of distinction which was based on checking
> >   width and height to determine whether a frame is audio or video
> > - Add subtitle fields to AVFrame
> > - Add new struct AVSubtitleArea, similar to AVSubtitleRect, but different
> >   allocation logic. Cannot and must not be used interchangeably, hence
> >   the new struct
> > - Move enum AVSubtitleType, AVSubtitle and AVSubtitleRect to avutil
> > - Add public-named members to enum AVSubtitleType (AV_SUBTITLE_FMT_)
> > - 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
> >
> > Signed-off-by: softworkz <softworkz at hotmail.com>
> > ---
> >  libavcodec/avcodec.c       |  19 ---
> >  libavcodec/avcodec.h       |  75 ++----------
> >  libavcodec/decode.c        |  53 ++++++--
> >  libavcodec/pgssubdec.c     |   1 +
> >  libavcodec/utils.c         |  11 ++
> >  libavfilter/vf_subtitles.c |  50 ++++++--
> >  libavformat/utils.c        |   1 +
> >  libavutil/Makefile         |   2 +
> >  libavutil/frame.c          | 194 ++++++++++++++++++++++++++---
> >  libavutil/frame.h          |  93 +++++++++++++-
> >  libavutil/subfmt.c         | 243 +++++++++++++++++++++++++++++++++++++
> >  libavutil/subfmt.h         | 185 ++++++++++++++++++++++++++++
> >  12 files changed, 802 insertions(+), 125 deletions(-)
> >  create mode 100644 libavutil/subfmt.c
> >  create mode 100644 libavutil/subfmt.h
> 
> First of all, this should be split. It does way way more than just
> "prepare AVFrame". It adds random things all over the place, some of
> them completely spurious (like the single include in pgssubdec).

Andreas hinted me that this could work without moving AVSubtitle symbols
at all. This allowed me to split these changes without breaking compilation.

Thanks.


> 
> >
> > +/**
> > + * Return subtitle format from a codec descriptor
> > + *
> > + * @param codec_descriptor codec descriptor
> > + * @return                 the subtitle type (e.g. bitmap, text)
> > + */
> > +enum AVSubtitleType av_get_subtitle_format_from_codecdesc(const
> AVCodecDescriptor *codec_descriptor);
> 
> New functions should be namespaced along the lines of av(_)<object>_<action>
> In this case, something like avcodec_descriptor_get_subtitle_format()

Thanks for the hint, I changed the name appropriately.


> Also, it seems to introduce an assumption that bitmap and text are
> mutually exclusive, while descriptors treat them as flags.

The assumption that bitmap and text subtitle formats are mutually exclusive
is not introduced here. It is already being made virtually everywhere in 
the code where it is dealing with subtitles.
Most simple example: AVSubtitle.format

The flags you are referring to are also used for indicating other things
(e.g. film grain) than subtitle formats, so you can't conclude that this 
is intending to imply the possibility of "hybrid" or "dual" subtitle formats.


> > +    /**
> > +     * Header containing style information for text subtitles.
> > +     */
> > +    AVBufferRef *subtitle_header;

> This is breaking ABI.

From earlier discussions, I think it's clear that the whole patchset cannot
go without breakage.


Kind regards,
softworkz