[FFmpeg-devel] [PATCH v20 02/20] avutil/frame: Prepare AVFrame for subtitle handling

[Lynne]

5 Dec 2021, 17:23 by softworkz at hotmail.com: 

> @@ -491,6 +499,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;
>

Milliseconds? Our entire API's based around timestamps
with time bases. Plus, we all know what happened when
Matroska settled onto milliseconds and ruined a perfectly
complex but good container.
Make this relative to the PTS field, with the same timebase
as the PTS field.
There's even a new AVFrame->time_base field for you to
set so you wouldn't forget it.


> +    /**
> +     * Number of items in the @ref subtitle_areas array.
> +     */
> +    unsigned num_subtitle_areas;
> +
> +    /**
> +     * Array of subtitle areas, may be empty.
> +     */
> +    AVSubtitleArea **subtitle_areas;
>

There's no reason why this cannot be handled using the buffer
and data fields. If you need more space there, you're free to bump
the maximum number of pointers, plus this removes the horrid
malloc(1) hack. We've discussed this, and I couldn't follow why
this was bad in the email discussion you linked.


> +    /**
> +     * 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;
>

I'm not going to accept a field which is instantly deprecated.
As we've discussed multiple times, please merge this into
the regular frame PTS field. We already have _2_ necessary
stard/end fields.


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

Good, I like this.