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

[Xiang, Haihao]

On Sat, 2021-08-07 at 03:24 +0000, Soft Works wrote:
> > -----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.

I think it is guaranteed because there is at most 1 qsv fifo item in the fifo
for these filters. 

> 
> 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.. 

Agree. According to the pseudo code in 
https://github.com/Intel-Media-SDK/MediaSDK/blob/master/doc/mediasdk-man.md#example-4-video-processing-pseudo-code
, we should reuse out_frame for MFX_ERR_MORE_DATA. I think it is better to
improve ff_qsvvpp_filter_frame() in another patch and this patch focuses on the
new filters.

Thanks
Haihao