[FFmpeg-devel] [PATCH] lavfi/qsvvpp: do not mix up FFmpeg and SDK error code

[Xiang, Haihao]

On Wed, 2021-08-04 at 06:34 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > Haihao Xiang
> > Sent: Friday, 30 July 2021 04:39
> > To: ffmpeg-devel at ffmpeg.org
> > Cc: Haihao Xiang <haihao.xiang at intel.com>
> > Subject: [FFmpeg-devel] [PATCH] lavfi/qsvvpp: do not mix up FFmpeg and
> > SDK error code
> > 
> > The function ff_qsvvpp_filter_frame should return a FFmpeg error code if
> > there is an error. However it might return a SDK error code without this
> > patch.
> > ---
> >  libavfilter/qsvvpp.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libavfilter/qsvvpp.c b/libavfilter/qsvvpp.c index
> > 4768f6208b..c7ef8a915f 100644
> > --- a/libavfilter/qsvvpp.c
> > +++ b/libavfilter/qsvvpp.c
> > @@ -807,8 +807,7 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> > AVFilterLink *inlink, AVFrame *picr
> >        if (MFXVideoCORE_SyncOperation(s->session, sync, 1000) < 0)
> >            av_log(ctx, AV_LOG_WARNING, "Sync failed.\n");
> 
> Why no looping and no checking for MFX_WRN_IN_EXECUTION like below?

Thanks for catching this, I think it should check for MFX_WRN_IN_EXECUTION, but
it should be fixed in another patch. 

> 
> > 
> >          filter_ret = s->filter_frame(outlink, tmp->frame);
> >          if (filter_ret < 0) {
> >              av_frame_free(&tmp->frame);
> > -            ret = filter_ret;
> > -            break;
> > +            return filter_ret;
> 
> The title is about not to mix error codes, but this is a behavioral change.
> After the patch, the input frame would no longer be processed in case 
> of a sync error.

The condition is 's->eof && qsv_fifo_size(s->async_fifo)'. When s->eof is true,
the input frame is actually NULL. So without this patch, this function returns 0
for this case, the error code is ignored. 

> 
> Also, shouldn't you call 
> 
> tmp->queued--;
> 
> before exiting?

I think it should be called because the data is freed from the AVFifoBuffer.

> 
> >          }
> >          tmp->queued--;
> >          s->got_frame = 1;
> > @@ -842,7 +841,7 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> > AVFilterLink *inlink, AVFrame *picr
> >          if (ret < 0 && ret != MFX_ERR_MORE_SURFACE) {
> >              /* Ignore more_data error */
> >              if (ret == MFX_ERR_MORE_DATA)
> > -                ret = AVERROR(EAGAIN);
> > +                return AVERROR(EAGAIN);
> >              break;
> >          }
> 
> I guess that makes sense. In case of overlay, we need to return immediately
> to let the caller submit the overlay surface.
> 
> >          out_frame->frame->pts = av_rescale_q(out_frame-
> > > surface.Data.TimeStamp,
> > 
> > @@ -864,8 +863,7 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s,
> > AVFilterLink *inlink, AVFrame *picr
> >              filter_ret = s->filter_frame(outlink, tmp->frame);
> >              if (filter_ret < 0) {
> >                  av_frame_free(&tmp->frame);
> > -                ret = filter_ret;
> > -                break;
> > +                return filter_ret;
> >              }
> > 
> >              tmp->queued--;
> 
> Same as above: Shouldn't this be called before returning?
> 

Yes, it should be called too.

Thanks
Haihao