Re: [FFmpeg-devel] [PATCH v9 02/25] avutil/frame: Prepare AVFrame for subtitle handling

[Hendrik Leppkes <h.leppkes@gmail.com>]

On Tue, Oct 25, 2022 at 11:14 AM softworkz <ffmpegagent@gmail.com> wrote:
>
> @@ -712,6 +712,53 @@ typedef struct AVFrame {
>       * Duration of the frame, in the same units as pts. 0 if unknown.
>       */
>      int64_t duration;
> +
> +    /**
> +     * Media type of the frame (audio, video, subtitles..)
> +     *
> +     * See AVMEDIA_TYPE_xxx
> +     */
> +    enum AVMediaType type;
> +
> +    /**
> +     * Number of items in the @ref subtitle_areas array.
> +     */
> +    unsigned num_subtitle_areas;
> +
> +    /**
> +     * Array of subtitle areas, may be empty.
> +     */
> +    AVSubtitleArea **subtitle_areas;
> +
> +    /**
> +     * Header containing style information for text subtitles.
> +     */
> +    AVBufferRef *subtitle_header;
> +
> +    /**
> +     * Indicates that a subtitle frame is a repeated frame for arbitrating flow
> +     * in a filter graph.
> +     * The field subtitle_timing.start_pts always indicates the original presentation
> +     * time, while the frame's pts field may be different.
> +     */
> +    int repeat_sub;
> +
> +    struct SubtitleTiming
> +    {
> +        /**
> +         * The display start time, in AV_TIME_BASE.
> +         *
> +         * For subtitle frames, AVFrame.pts is populated from the packet pts value,
> +         * which is not always the same as this value.
> +         */
> +        int64_t start_pts;

There is still no explanation here why they are not the same, why they
could not just be the same, and which field a user should look at.
The cover letter talks about clarity why this is needed is important,
but then provides none of that.

"Its required" is not an argument. So please enlighten us once again
why we absolutely need two timestamps for subtitles, instead of just
one. As far as I can see, subtitle frames only have one relevant time
- when its supposed to be shown on the screen. What would the other
time ever be good for to a user?
Similarly for the duration, of course. I can even see the
AVFrame.duration field in this patch snippet just above the additions
that would seem to fully replace this one.

- Hendrik



> +
> +        /**
> +         * Display duration, in AV_TIME_BASE.
> +         */
> +        int64_t duration;
> +
> +    } subtitle_timing;
>  } AVFrame;
>
>
> @@ -788,6 +835,8 @@ void av_frame_move_ref(AVFrame *dst, AVFrame *src);
>  /**
>   * Allocate new buffer(s) for audio or video data.
>   *
> + * Note: For subtitle data, use av_frame_get_buffer2
> + *
>   * The following fields must be set on frame before calling this function:
>   * - format (pixel format for video, sample format for audio)
>   * - width and height for video
> @@ -807,9 +856,39 @@ void av_frame_move_ref(AVFrame *dst, AVFrame *src);
>   *              recommended to pass 0 here unless you know what you are doing.
>   *
>   * @return 0 on success, a negative AVERROR on error.
> + *
> + * @deprecated Use @ref av_frame_get_buffer2 instead and set @ref AVFrame.type
> + * before calling.
>   */
> +attribute_deprecated
>  int av_frame_get_buffer(AVFrame *frame, int align);
>
> +/**
> + * Allocate new buffer(s) for audio, video or subtitle data.
> + *
> + * The following fields must be set on frame before calling this function:
> + * - format (pixel format for video, sample format for audio)
> + * - width and height for video
> + * - nb_samples and channel_layout for audio
> + * - type (AVMediaType)
> + *
> + * This function will fill AVFrame.data and AVFrame.buf arrays and, if
> + * necessary, allocate and fill AVFrame.extended_data and AVFrame.extended_buf.
> + * For planar formats, one buffer will be allocated for each plane.
> + *
> + * @warning: if frame already has been allocated, calling this function will
> + *           leak memory. In addition, undefined behavior can occur in certain
> + *           cases.
> + *
> + * @param frame frame in which to store the new buffers.
> + * @param align Required buffer size alignment. If equal to 0, alignment will be
> + *              chosen automatically for the current CPU. It is highly
> + *              recommended to pass 0 here unless you know what you are doing.
> + *
> + * @return 0 on success, a negative AVERROR on error.
> + */
> +int av_frame_get_buffer2(AVFrame *frame, int align);
> +
>  /**
>   * Check if the frame data is writable.
>   *
> diff --git a/libavutil/subfmt.c b/libavutil/subfmt.c
> new file mode 100644
> index 0000000000..c72ebe2a43
> --- /dev/null
> +++ b/libavutil/subfmt.c
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (c) 2021 softworkz
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +#include <string.h>
> +#include "common.h"
> +#include "subfmt.h"
> +
> +static const char sub_fmt_info[AV_SUBTITLE_FMT_NB][24] = {
> +    [AV_SUBTITLE_FMT_UNKNOWN] = "Unknown subtitle format",
> +    [AV_SUBTITLE_FMT_BITMAP]  = "Graphical subtitles",
> +    [AV_SUBTITLE_FMT_TEXT]    = "Text subtitles (plain)",
> +    [AV_SUBTITLE_FMT_ASS]     = "Text subtitles (ass)",
> +};
> +
> +const char *av_get_subtitle_fmt_name(enum AVSubtitleType sub_fmt)
> +{
> +    if (sub_fmt < 0 || sub_fmt >= AV_SUBTITLE_FMT_NB)
> +        return NULL;
> +    return sub_fmt_info[sub_fmt];
> +}
> +
> +enum AVSubtitleType av_get_subtitle_fmt(const char *name)
> +{
> +    for (int i = 0; i < AV_SUBTITLE_FMT_NB; i++)
> +        if (!strcmp(sub_fmt_info[i], name))
> +            return i;
> +    return AV_SUBTITLE_FMT_NONE;
> +}
> diff --git a/libavutil/subfmt.h b/libavutil/subfmt.h
> index 791b45519f..3b8a0613b2 100644
> --- a/libavutil/subfmt.h
> +++ b/libavutil/subfmt.h
> @@ -21,6 +21,9 @@
>  #ifndef AVUTIL_SUBFMT_H
>  #define AVUTIL_SUBFMT_H
>
> +#include <stdint.h>
> +
> +#include "buffer.h"
>  #include "version.h"
>
>  enum AVSubtitleType {
> @@ -65,4 +68,48 @@ enum AVSubtitleType {
>      AV_SUBTITLE_FMT_NB,         ///< number of subtitle formats, DO NOT USE THIS if you want to link with shared libav* because the number of formats might differ between versions.
>  };
>
> +typedef struct AVSubtitleArea {
> +#define AV_NUM_BUFFER_POINTERS 1
> +
> +    enum AVSubtitleType type;
> +    int flags;
> +
> +    int x;         ///< top left corner  of area.
> +    int y;         ///< top left corner  of area.
> +    int w;         ///< width            of area.
> +    int h;         ///< height           of area.
> +    int nb_colors; ///< number of colors in bitmap palette (@ref pal).
> +
> +    /**
> +     * Buffers and line sizes for the bitmap of this subtitle.
> +     *
> +     * @{
> +     */
> +    AVBufferRef *buf[AV_NUM_BUFFER_POINTERS];
> +    int linesize[AV_NUM_BUFFER_POINTERS];
> +    /**
> +     * @}
> +     */
> +
> +    uint32_t pal[256]; ///< RGBA palette for the bitmap.
> +
> +    char *text;        ///< 0-terminated plain UTF-8 text
> +    char *ass;         ///< 0-terminated ASS/SSA compatible event line.
> +
> +} AVSubtitleArea;
> +
> +/**
> + * Return the name of sub_fmt, or NULL if sub_fmt is not
> + * recognized.
> + */
> +const char *av_get_subtitle_fmt_name(enum AVSubtitleType sub_fmt);
> +
> +/**
> + * Return a subtitle format corresponding to name, or AV_SUBTITLE_FMT_NONE
> + * on error.
> + *
> + * @param name Subtitle format name.
> + */
> +enum AVSubtitleType av_get_subtitle_fmt(const char *name);
> +
>  #endif /* AVUTIL_SUBFMT_H */
> --
> ffmpeg-codebot
>
> _______________________________________________
> 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".
_______________________________________________
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".