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

[Anton Khirnov]

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).

>  
> +/**
> + * 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()

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

> diff --git a/libavutil/frame.h b/libavutil/frame.h
> index 753234792e..742f4ba07e 100644
> --- a/libavutil/frame.h
> +++ b/libavutil/frame.h
> @@ -34,6 +34,7 @@
>  #include "rational.h"
>  #include "samplefmt.h"
>  #include "pixfmt.h"
> +#include "subfmt.h"
>  #include "version.h"
>  
>  
> @@ -278,7 +279,7 @@ typedef struct AVRegionOfInterest {
>  } AVRegionOfInterest;
>  
>  /**
> - * This structure describes decoded (raw) audio or video data.
> + * This structure describes decoded (raw) audio, video or subtitle data.
>   *
>   * AVFrame must be allocated using av_frame_alloc(). Note that this only
>   * allocates the AVFrame itself, the buffers for the data must be managed
> @@ -309,6 +310,13 @@ typedef struct AVRegionOfInterest {
>   */
>  typedef struct AVFrame {
>  #define AV_NUM_DATA_POINTERS 8
> +    /**
> +     * Media type of the frame (audio, video, subtitles..)
> +     *
> +     * See AVMEDIA_TYPE_xxx
> +     */
> +    enum AVMediaType type;
> +
>      /**
>       * pointer to the picture/channel planes.
>       * This might be different from the first allocated byte. For video,
> @@ -390,7 +398,7 @@ typedef struct AVFrame {
>      /**
>       * format of the frame, -1 if unknown or unset
>       * Values correspond to enum AVPixelFormat for video frames,
> -     * enum AVSampleFormat for audio)
> +     * enum AVSampleFormat for audio, AVSubtitleType for subtitles)
>       */
>      int format;
>  
> @@ -481,6 +489,39 @@ typedef struct AVFrame {
>       */
>      uint64_t channel_layout;
>  
> +    /**
> +     * Display start time, relative to packet pts, in ms.
> +     */
> +    uint32_t subtitle_start_time;
> +
> +    /**
> +     * Display end time, relative to packet pts, in ms.
> +     */
> +    uint32_t subtitle_end_time;
> +
> +    /**
> +     * Number of items in the @ref subtitle_areas array.
> +     */
> +    unsigned num_subtitle_areas;
> +
> +    /**
> +     * Array of subtitle areas, may be empty.
> +     */
> +    AVSubtitleArea **subtitle_areas;
> +
> +    /**
> +     * Usually the same as packet pts, in AV_TIME_BASE.
> +     *
> +     * @deprecated This is kept for compatibility reasons and corresponds to
> +     * AVSubtitle->pts. Might be removed in the future.
> +     */
> +    int64_t subtitle_pts;
> +
> +    /**
> +     * Header containing style information for text subtitles.
> +     */
> +    AVBufferRef *subtitle_header;

This is breaking ABI.

-- 
Anton Khirnov