[FFmpeg-devel] [PATCH v8 06/13] avfilter/overlay_graphicsubs: Add overlay_graphicsubs and graphicsub2video filters

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Andreas
> Rheinhardt
> Sent: Wednesday, 22 September 2021 06:24
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v8 06/13] avfilter/overlay_graphicsubs:
> Add overlay_graphicsubs and graphicsub2video filters
> 
> Soft Works:
> > - overlay_graphicsubs (VS -> V)
> >   Overlay graphic subtitles onto a video stream
> >
> > - graphicsub2video {S -> V)
> >   Converts graphic subtitles to video frames (with alpha)
> >   Gets auto-inserted for retaining compatibility with
> >   sub2video command lines
> >
> > Signed-off-by: softworkz <softworkz at hotmail.com>
> > ---
> >  doc/filters.texi                     | 104 ++++
> >  libavfilter/Makefile                 |   2 +
> >  libavfilter/allfilters.c             |   2 +
> >  libavfilter/vf_overlay_graphicsubs.c | 730 +++++++++++++++++++++++++++
> >  4 files changed, 838 insertions(+)
> >  create mode 100644 libavfilter/vf_overlay_graphicsubs.c
> >
> > +
> > +/**
> > + * Blend image in src to destination buffer dst at position (x, y).
> > + */
> 
> This whole code looks quite duplicated from the ordinary overlay.

Yes - it looks like and it is derived from vf_overlay, but it's
not really a duplication.

The code in vf_overlay is for blending images of similar formats which
may only differ by an alpha component, while the code here is 
about blending a PAL8 images over multiple main formats.

As an example, when blending the PAL8 image over yuv420, I'm only 
converting the palette to yuv for better efficiency, and in case
of subsampling, there's a different stepping for main and overlay
data.

Mangling those specifics into the existing vf_overlay code, would
neither improve execution performance nor code readability and
ease maintenance.

Paul had initially criticised this initially as well, but eventually
agreed that it's better to keep it separate from vf_overlay.

This is the only comment of yours on which I didn’t take action,
and I wanted to explain why.

Kind regards,
softworkz