[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 08:11
> 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 1:21 AM Soft Works <softworkz at hotmail.com>
> wrote:
> >
> > Hendrik,
> >
> > I can't avoid to note that your reply pattern is a bit similar to
> > what I've seen before here: long texts trying to point out one's
> > authority, naming rejection conditions, trying to discourage
> instead
> > of encourage and finally telling I would need to follow advice
> > and guidance.
> > Yet, each time I'm asking for advice and guidance - those are
> > remaining silent that were telling me to follow.
> 
> Or perhaps some people have a day job, a life and other obligations
> that prevent them from spending time on FFmpeg every day, especially
> outside the weekend.

Oh, I apologize. I hadn’t taken into account that you might not
have responded due to time constraints. My fault! 


> 
> As for the actual subject:
> - Part1, add subtitles to AVFrame in avutil. 

Check.

> You can move
> enums/structs to avutil as needed (eg AVSubtitleRect or whatever), as
> long as they are still available the same way (eg. avcodec.h should
> include any new avutil header with them, so user code works without
> changes), 

Check.

> To dive a bit deeper into this, redundant fields should be removed,
> actual subtitle data should be refcounted (with AVBuffers in
> AVFrame->buf), all frame functionality need to properly account for
> the new data type.

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 guess I don't need to name the reasons why it might not be
the best idea.


In the earlier approach I had to refcount the whole AVSubtitleRect
elements, which wasn't ideal, now it's only AVSubtitleArea->buf[]
which are handled in the same way as AVFrame->buf[].



> 
> - Part2, avcodec support for subtitles in AVFrames. Not that long ago
> we added a generic decoding API,
> avcodec_send_packet/avcodec_receive_frame and encoding API
> avcodec_send_frame/avcodec_receive_packet. Those are
> mediatype-agnostic, so they can also handle subtitles. 

I have added avcodec_decode_subtitle3 which takes AVFrames.
It calls avcodec_decode_subtitle2 with a temp AVSubtitle 
and copies it into AVFrame.

I will look into doing this in the main decode function.

> No need for a
> new API, just new internal plumbing. The important part here is that
> the old API must remain untouched and functional, 

Check.

which means the
> easiest way for the time being is adding a compatibility shim that
> uses the old internal API and then converts to an AVFrame (or
> vice-versa for encoding).

Check (see above)

 This can get inverted later, eg. all
> de/encoders updated to a new internal API, and the old external API
> being run through a shim, but I would recommend to actually do this
> later, as it reduces the friction.

Yup, makes sense.


> - Part3, avfilter support for subtitles in AVFrames. At this point we
> have a defined structure to store subtitles in AVFrames, and actual
> code that can generate or consume them. When approaching this, the
> same rules apply as before, existing subtitle functionality, as crude
> as it may be, has to remain functional as exposed to the user.
> Internal under-the-hood changes are fine, as long as the user does
> not
> notice.

All good at this side, I think.

> - Part4, update ffmpeg.c etc to make use of these new features
> 
> All these parts should be made up of as small incremental atomic
> changes as possible, to ease reviewing and bisecting for issues
> later.

That's a bit of a problem, because in this case it's difficult to 
have both: "small and incremental" vs. "atomic".

In the upcoming patchset, the encoder/decoder changes are gone, 
so that is out of the way. I have merged some commits together,
but there's still a period in the patchset where build or fate
doesn't run successfully.
I kept it that way for better clarity and easier reviewing.
(merging them together is a matter of seconds, but not the 
other way round ;-)


> In every case the old subtitle functionality has to remain functional
> as exposed to the user, once the new one is in place it can get
> deprecated and removed in ~2 years time. 

The sub2video functionality is still working (by auto-inserting
the graphicsub2video filter). It has never been exposed in terms
of command line parameters - it just worked and it will keep 
working without the need to ever remove something.
Though, there's a better alternative now with vf_overlay_graphicsubs.

vf_subtitles will continue to work, but superseded by 
vf_overlay_textsubs. vf_subtitles can be declared deprecated or
kept as is - I don't really have an opinion about it.


> developing something parallel, sometimes keeping the old one alive
> with a compatibility shim, or basing the new one on a shim for now,
> which can get inverted later.
> 
> Hope some of that helps.
> 
> - Hendrik


Let me apologize again, I'm very sorry for the false interpretation,
this was one of the most useful, productive and accurate answers 
that I've ever got here!

Kind regards,
softworkz