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

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Lynne
> Sent: Sunday, December 5, 2021 5:40 PM
> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v20 02/20] avutil/frame: Prepare AVFrame
> for subtitle handling
> 
> 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.


Hi Lynne,

let me add another note on these two topics.

One of the reasons why this subtitle filtering patchset has proceeded
rather smoothly so far, causing only a small amount of regressions 
that were easy to find and fix, is that I have kept as much as possible
from the current AVSubtitle implementation, projecting it to the 
new frame-based subtitle implementation, while keeping it mostly 
compatible with the current logic.
Interestingly, an earlier attempt towards subtitle filtering has
gotten stuck right at the point about storing subtitle data in 
AVFrame buffers. I decided to focus on functionality rather than
academic kind of details. What has been in place and working
for such a long time can't be so wrong that it can't be accepted.

We also need to consider that it's not my patchset that is deliberately
deciding to store data in pointers associate with rects/area and 
to store start- and end-times in milliseconds: it's all the decoders
and encoders that are doing this. I'm just adapting to the status
quo.

These two things - changing the timebase of the ms-fields and 
changing the location of the buffer pointers - is a gigantic task 
that would require to change everything that this patchset covers 
and implements - plus even more code that this patchset hasn't even 
touched. It would require changes to everything everywhere where
subtitles are concerned.
It is a task that is risky as hell, will cause plenty of 
regressions under  guarantee, require extensive testing and might even 
drive this whole endeavor into a dead end.  

As I said before, please understand that I'm not ready to enter that
path.

Thanks,
softworkz