| Message ID | 20210730023924.11043-1-haihao.xiang@intel.com |
|---|---|
| State | Accepted |
| Commit | 43aeeab764c6fd89aea777767be9c0b16cedd78a |
| Headers | show |
| Series | [FFmpeg-devel] lavfi/qsvvpp: do not mix up FFmpeg and SDK error code | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Haihao Xiang > Sent: Friday, 30 July 2021 04:39 > To: ffmpeg-devel@ffmpeg.org > Cc: Haihao Xiang <haihao.xiang@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? > > 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. Also, shouldn't you call tmp->queued--; before exiting? > } > 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? softworkz
On Wed, 2021-08-04 at 06:34 +0000, Soft Works wrote: > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Haihao Xiang > > Sent: Friday, 30 July 2021 04:39 > > To: ffmpeg-devel@ffmpeg.org > > Cc: Haihao Xiang <haihao.xiang@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
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Xiang, Haihao > Sent: Thursday, 5 August 2021 07:24 > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH] lavfi/qsvvpp: do not mix up FFmpeg > and SDK error code > > On Wed, 2021-08-04 at 06:34 +0000, Soft Works wrote: > > > -----Original Message----- > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > Haihao Xiang > > > Sent: Friday, 30 July 2021 04:39 > > > To: ffmpeg-devel@ffmpeg.org > > > Cc: Haihao Xiang <haihao.xiang@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. OK. > > > > > > 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. My bad, I missed the eof condition. All details clarified. Patch LGTM. softworkz
On Thu, 2021-08-05 at 16:05 +0000, Soft Works wrote: > > -----Original Message----- > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > Xiang, Haihao > > Sent: Thursday, 5 August 2021 07:24 > > To: ffmpeg-devel@ffmpeg.org > > Subject: Re: [FFmpeg-devel] [PATCH] lavfi/qsvvpp: do not mix up FFmpeg > > and SDK error code > > > > On Wed, 2021-08-04 at 06:34 +0000, Soft Works wrote: > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > > Haihao Xiang > > > > Sent: Friday, 30 July 2021 04:39 > > > > To: ffmpeg-devel@ffmpeg.org > > > > Cc: Haihao Xiang <haihao.xiang@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. > > OK. > > > > > > > > > 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. > > My bad, I missed the eof condition. > > > All details clarified. Patch LGTM. Could someone merge this patch if no more comments, please? Thanks Haihao
On 8/9/2021 10:55 PM, Xiang, Haihao wrote: > On Thu, 2021-08-05 at 16:05 +0000, Soft Works wrote: >>> -----Original Message----- >>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >>> Xiang, Haihao >>> Sent: Thursday, 5 August 2021 07:24 >>> To: ffmpeg-devel@ffmpeg.org >>> Subject: Re: [FFmpeg-devel] [PATCH] lavfi/qsvvpp: do not mix up FFmpeg >>> and SDK error code >>> >>> On Wed, 2021-08-04 at 06:34 +0000, Soft Works wrote: >>>>> -----Original Message----- >>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of >>>>> Haihao Xiang >>>>> Sent: Friday, 30 July 2021 04:39 >>>>> To: ffmpeg-devel@ffmpeg.org >>>>> Cc: Haihao Xiang <haihao.xiang@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. >> >> OK. >> >>>>> >>>>> 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. >> >> My bad, I missed the eof condition. >> >> >> All details clarified. Patch LGTM. > > Could someone merge this patch if no more comments, please? > > Thanks > Haihao Pushed.
On Mon, 2021-08-09 at 22:58 -0300, James Almer wrote: > On 8/9/2021 10:55 PM, Xiang, Haihao wrote: > > On Thu, 2021-08-05 at 16:05 +0000, Soft Works wrote: > > > > -----Original Message----- > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > > Xiang, Haihao > > > > Sent: Thursday, 5 August 2021 07:24 > > > > To: ffmpeg-devel@ffmpeg.org > > > > Subject: Re: [FFmpeg-devel] [PATCH] lavfi/qsvvpp: do not mix up FFmpeg > > > > and SDK error code > > > > > > > > On Wed, 2021-08-04 at 06:34 +0000, Soft Works wrote: > > > > > > -----Original Message----- > > > > > > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > > > > > > Haihao Xiang > > > > > > Sent: Friday, 30 July 2021 04:39 > > > > > > To: ffmpeg-devel@ffmpeg.org > > > > > > Cc: Haihao Xiang <haihao.xiang@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. > > > > > > OK. > > > > > > > > > > > > > > > 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. > > > > > > My bad, I missed the eof condition. > > > > > > > > > All details clarified. Patch LGTM. > > > > Could someone merge this patch if no more comments, please? > > > > Thanks > > Haihao > > Pushed. Thanks James.
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 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--; 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; } 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--; @@ -874,5 +872,10 @@ int ff_qsvvpp_filter_frame(QSVVPPContext *s, AVFilterLink *inlink, AVFrame *picr } } while(ret == MFX_ERR_MORE_SURFACE); - return ret; + if (ret < 0) + return ff_qsvvpp_print_error(ctx, ret, "Error running VPP"); + else if (ret > 0) + ff_qsvvpp_print_warning(ctx, ret, "Warning in running VPP"); + + return 0; }