On Wed, 2021-08-04 at 06:34 +0000, Soft Works wrote:
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-boun...@ffmpeg.org> On Behalf Of
> > Haihao Xiang
> > Sent: Friday, 30 July 2021 04:39
> > To: ffmpeg-devel@ffmpeg.org
> > Cc: Haihao Xiang <haihao.xi...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
