[FFmpeg-devel] [PATCH v5 00/12] Subtitle Filtering

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Wednesday, 15 September 2021 11:21
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v5 00/12] Subtitle Filtering
> 
> On Wed, Sep 15, 2021 at 9:39 AM Soft Works <softworkz at hotmail.com>
> wrote:
> >
> > In my upcoming patchset, I have added a new struct AVSubtitleArea
> > which is similar to AVSubtitleRect with the difference that
> >
> > - It's not AVSubtitleRect (which can be deprecated then)
> > - It uses refcounted buffers for the image data
> >
> > AVFrame gets an array of AVSubtitleArea structs. The image data
> > is attached to AVSubtitleArea (data, buf, linesize).
> >
> > I'm not sure whether it would make sense to store the image data in
> > buf/data of AVFrame like
> >
> > AVSubtitleArea[0] <=> buf[0], buf[1]
> > AVSubtitleArea[1] <=> buf[2], buf[3]
> >
> 
> I suppose the main problem is that there can be more areas then we
> have buf entries?

Yes, that's one, and another one being that it might become limiting
at one day when a subtitle bitmap would need to have three buffers
or there would emerge a reason to have a buffer for a text subtitle
or something.
Once another issue is that it's not a great as an API to use. For
example, you want to have a function that does something with 
a subtitle rect. Obviously, you would want to have a function
that takes AVSubtitleRect as a single parameters.
If the buffer data reference is stored outside of AVSubtitleRect
(in AVFrame), you would need to pass 

- AVSubtitleRect
- AVFrame
- The index of AVSubtitleRect in its containing array
  (or some other means to make a relation between the AVSubtitleRect
   and its data)

to that function that manipulates the AVSubtitleRect


Or - other example: assume a filter that manipulates subtitles, let's
say that a frame has three subtitle rects and it wants to delete 
the second one. 
For doing so, the filter would need to have all the logic for relating
subtitle rects and AVFrame->buf[] data, for being able to re-locate
the buffers in AVFrame to close the gap that exists after removing
the second rect.
Having that logic in filters would mean that it cannot be changed
in the future.

When the buffers are members of AVSubtitleRect, removing one of 
them will be as easy as recreating the array and freeing the
removed AVSubtitleRect.



> There is just one small wrinkle, there is an expectation in various
> areas of the code that an empty frame has buf[0] zero, and conversely
> a filled one does not.

Yea I've seen that, it is being used to determine whether a frame 
is ref-counted or not.

My current idea would be

- to introduce an av_frame_is_refcounted function
  plus
- an AVFrame of type subtitle is always refcounted

This should work fine due to the fact that there doesn't exist 
any external code for handling frames of type subtitles,
so the ->buf[0] check will continue working for these.

The internal code can be changed to use av_frame_is_refcounted
instead of the buf[0] check.

What do you think about it?

 
> If the presence of the buf is also the only difference to
> AVSubtitleRect, there is no reason to not continue using it and
> moving
> it to avutil, and just have AVSubtitle itself be deprecated in the
> long-term.

The problem with that is that it is introducing ambiguous logic
regarding AVSubtitleRect:

Existing code expects that it can release memory by calling
av_freep(&rect->data[0]). That would be fatal in case that
the data is managed by buf[0] and data[0] is just a convenience
pointer.

There are probably other cases as well, which brought me to
the conclusion that it would be much cleaner to have a new 
struct for the new implementation and deprecate AVSubtitleRect.
This would avoid confusion and errors, especially when migrating
existing codecs to the new API.


Thanks a lot for taking the time to help.

Kind regards,
softworkz