[FFmpeg-devel] [PATCH v2] avfilter: add QSV variants of the stack filters

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Xiang, Haihao
> Sent: Friday, 6 August 2021 07:15
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2] avfilter: add QSV variants of the
> stack filters
> 
> On Thu, 2021-08-05 at 15:53 +0000, Soft Works wrote:
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > > Xiang, Haihao
> > > Sent: Thursday, 5 August 2021 04:33
> > > To: ffmpeg-devel at ffmpeg.org
> > > Subject: Re: [FFmpeg-devel] [PATCH v2] avfilter: add QSV variants of
> > > the stack filters
> > >
> > > On Wed, 2021-08-04 at 09:17 +0000, Soft Works wrote:
> > > > > -----Original Message-----
> > > > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On
> Behalf
> > > > > Of Haihao Xiang
> > > > > Sent: Wednesday, 4 August 2021 10:33
> > > > > To: ffmpeg-devel at ffmpeg.org
> > > > > Cc: Haihao Xiang <haihao.xiang at intel.com>
> > > > > Subject: [FFmpeg-devel] [PATCH v2] avfilter: add QSV variants of
> > > > > the stack filters
> > > > >
> > > > > Include hstack_qsv, vstack_qsv and xstack_qsv, some code is copy
> > > > > and pasted from other filters
> > > > >
> > > > > Example:
> > > > > $> ffmpeg -hwaccel qsv -c:v hevc_qsv -i input.h265
> > > > > -filter_complex "[0:v][0:v]hstack_qsv" -f null -
> > > > > ---
> > > >
> > > > [...]
> > > >
> > > > > +
> > > > > +/*
> > > > > + * Callback for qsvvpp
> > > > > + * @Note: qsvvpp composition does not generate PTS for result
> frame.
> > > > > + *        so we assign the PTS from framesync to the output frame.
> > > > > + */
> > > > > +
> > > > > +static int filter_callback(AVFilterLink *outlink, AVFrame *frame) {
> > > > > +    QSVStackContext *sctx = outlink->src->priv;
> > > > > +
> > > > > +    frame->pts = av_rescale_q(sctx->fs.pts,
> > > > > +                              sctx->fs.time_base, outlink->time_base);
> > > > > +    return ff_filter_frame(outlink, frame); }
> > > >
> > > > If the surface.Data.TimeStamp gets overwritten by libMFX, why not
> > > > copy the PTS from the input frame in ff_qsvvpp_filter_frame ?
> > > >
> > > > That would apply the timestamp from the last input, though.
> > > > Preferably would it be taken from the first input instead. For
> > > > 2-n, you could perhaps clone the frames and assign the pts from
> > > > the first input's frame?
> > >
> > > Thanks for the comment and suggestion. This callback function was
> > > copy-
> > > and- pasted from overlay_qsv filter because MSDK composition is also
> > > used by this filter, I'd like to use the same way to generate pts
> > > for these filters, but I'll try your suggestion for these filters in
> > > the future.
> >
> > Yea I see - the overlay_qsv filter does it the same way. This has
> > probably been ok earlier because the callback happened synchronously.
> > This is no longer the case since the async_depth patch which
> > introduced the fifo processing. Now it can happen that the calback is
> > performed for an earlier frame than the one that is currently gated by
> > framesync.
> 
> async_depth is not enabled for overlay_qsv and stack qsv filters, s-
> >async_depth is 0, so the callback is still performed synchronously for these
> filters.

Yes I know. But with the newly added looping and read/write from the
fifos, I'm not sure whether it's always guaranteed that the callback will be
called for the submitted frame or whether it could happen that there's 
another out_frame left in the fifo.

Another detail that doesn't look solid to me is the acquisition of the out_frame
in cases when composition is used, i.e. ff_qsvvpp_filter_frame is called
multiple times for the same output frame:

- When VPP returns MFX_ERR_MORE_DATA (awaiting another input for
  overlay), we return without caring about the out_frame (no storing, no
  increasing of the queued property)
- Instead, each time, the out_frame is queried again via query_frame
  The code doesn't really make sure that it operates on the same frame 
  for output, this seems more like a coincidence at the current state
  (probably only the check for surface->Data->Locked in clear_unused_frames
  is preventing the output frame from being cleared meanwhile)

I think there's some room for improvement at least.. 

softworkz