| Message ID | 20190215195018.7811-1-linjie.fu@intel.com |
|---|---|
| State | New |
| Headers | show |
2019-02-15 20:50 GMT+01:00, Linjie Fu <linjie.fu@intel.com>: > Currently, BFF or TFF will not be set if the frame is progressive. > This will break the Input PicStruct check in MSDK Was this always documented? If not - given that this is completely counterintuitive - this should be fixed in the driver. Carl Eugen
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Carl Eugen Hoyos > Sent: Friday, February 15, 2019 20:46 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH, RFC] lavf/deinterlace_qsv: set TFF or > BFF together with progressive > > 2019-02-15 20:50 GMT+01:00, Linjie Fu <linjie.fu@intel.com>: > > Currently, BFF or TFF will not be set if the frame is progressive. > > This will break the Input PicStruct check in MSDK > > Was this always documented? > > If not - given that this is completely counterintuitive - this should > be fixed in the driver. > I agree that this should be fixed first in the driver or MSDK. However, in order to be more robust, I think it might be possible to make this modification in ffmpeg qsv too. This will help if the user is using an earlier version of the MSDK/driver. --- Linjie
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Linjie Fu > Sent: Saturday, February 16, 2019 3:50 AM > To: ffmpeg-devel@ffmpeg.org > Cc: Fu, Linjie <linjie.fu@intel.com> > Subject: [FFmpeg-devel] [PATCH, RFC] lavf/deinterlace_qsv: set TFF or BFF > together with progressive > > Currently, BFF or TFF will not be set if the frame is progressive. > This will break the Input PicStruct check in MSDK because of absence of the > specific PicStruct check for: > MFX_PICSTRUCT_PROGRESSIVE | MFX_PICSTRUCT_FIELD_REPEATED > > Set PicStruct to MFX_PICSTRUCT_FIELD_TFF or MFX_PICSTRUCT_FIELD_BFF > to match the CheckInputPicStruct in MSDK. > > Fix #7701. After checking this ticket, I believe this is not a MSDK issue, and current fix in this patch is not correct. If I understand correctly, this issue only happens when H264 pic_struct = 5 or 6. In this case, MFX_PICSTRUCT_FIELD_TFF or MFX_PICSTRUCT_FIELD_BFF should be set, else we don't know which filed should be repeated. > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > --- > Is it acceptable to add the TFF or BFF to PicStruct according to the attribute > of the input frames, even if it's not interlaced? > Or it should be fixed in MSDK level to support more PicStruct? > > libavfilter/vf_deinterlace_qsv.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/libavfilter/vf_deinterlace_qsv.c b/libavfilter/vf_deinterlace_qsv.c > index d6b02e98c5..f03d65f029 100644 > --- a/libavfilter/vf_deinterlace_qsv.c > +++ b/libavfilter/vf_deinterlace_qsv.c > @@ -417,8 +417,9 @@ static int submit_frame(AVFilterContext *ctx, > AVFrame *frame, > qf->surface.Info.CropH = qf->frame->height; > > qf->surface.Info.PicStruct = !qf->frame->interlaced_frame ? > MFX_PICSTRUCT_PROGRESSIVE : > - (qf->frame->top_field_first ? > MFX_PICSTRUCT_FIELD_TFF : > - > MFX_PICSTRUCT_FIELD_BFF); > + > MFX_PICSTRUCT_UNKNOWN; > + qf->surface.Info.PicStruct |= qf->frame->top_field_first ? > MFX_PICSTRUCT_FIELD_TFF : > + > + MFX_PICSTRUCT_FIELD_BFF; > if (qf->frame->repeat_pict == 1) > qf->surface.Info.PicStruct |= MFX_PICSTRUCT_FIELD_REPEATED; I believe the correct fix should be here (previous code change is not needed and should be removed): if (qf->frame->repeat_pict == 1) { qf->surface.Info.PicStruct |= MFX_PICSTRUCT_FIELD_REPEATED; qf->surface.Info.PicStruct |= qf->frame->top_field_first ? MFX_PICSTRUCT_FIELD_TFF : MFX_PICSTRUCT_FIELD_BFF; } > else if (qf->frame->repeat_pict == 2)
> -----Original Message----- > From: Li, Zhong > Sent: Monday, February 18, 2019 13:48 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Cc: Fu, Linjie <linjie.fu@intel.com> > Subject: RE: [FFmpeg-devel] [PATCH, RFC] lavf/deinterlace_qsv: set TFF or > BFF together with progressive > > > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On > Behalf > > Of Linjie Fu > > Sent: Saturday, February 16, 2019 3:50 AM > > To: ffmpeg-devel@ffmpeg.org > > Cc: Fu, Linjie <linjie.fu@intel.com> > > Subject: [FFmpeg-devel] [PATCH, RFC] lavf/deinterlace_qsv: set TFF or BFF > > together with progressive > > > > Currently, BFF or TFF will not be set if the frame is progressive. > > This will break the Input PicStruct check in MSDK because of absence of the > > specific PicStruct check for: > > MFX_PICSTRUCT_PROGRESSIVE | MFX_PICSTRUCT_FIELD_REPEATED > > > > Set PicStruct to MFX_PICSTRUCT_FIELD_TFF or > MFX_PICSTRUCT_FIELD_BFF > > to match the CheckInputPicStruct in MSDK. > > > > Fix #7701. > > After checking this ticket, I believe this is not a MSDK issue, and current fix in > this patch is not correct. > If I understand correctly, this issue only happens when H264 pic_struct = 5 or > 6. > In this case, MFX_PICSTRUCT_FIELD_TFF or MFX_PICSTRUCT_FIELD_BFF > should be set, else we don't know which filed should be repeated. > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > --- > > Is it acceptable to add the TFF or BFF to PicStruct according to the attribute > > of the input frames, even if it's not interlaced? > > Or it should be fixed in MSDK level to support more PicStruct? > > > > libavfilter/vf_deinterlace_qsv.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/libavfilter/vf_deinterlace_qsv.c > b/libavfilter/vf_deinterlace_qsv.c > > index d6b02e98c5..f03d65f029 100644 > > --- a/libavfilter/vf_deinterlace_qsv.c > > +++ b/libavfilter/vf_deinterlace_qsv.c > > @@ -417,8 +417,9 @@ static int submit_frame(AVFilterContext *ctx, > > AVFrame *frame, > > qf->surface.Info.CropH = qf->frame->height; > > > > qf->surface.Info.PicStruct = !qf->frame->interlaced_frame ? > > MFX_PICSTRUCT_PROGRESSIVE : > > - (qf->frame->top_field_first ? > > MFX_PICSTRUCT_FIELD_TFF : > > - > > MFX_PICSTRUCT_FIELD_BFF); > > + > > MFX_PICSTRUCT_UNKNOWN; > > + qf->surface.Info.PicStruct |= qf->frame->top_field_first ? > > MFX_PICSTRUCT_FIELD_TFF : > > + > > + MFX_PICSTRUCT_FIELD_BFF; > > if (qf->frame->repeat_pict == 1) > > qf->surface.Info.PicStruct |= MFX_PICSTRUCT_FIELD_REPEATED; > > I believe the correct fix should be here (previous code change is not needed > and should be removed): > if (qf->frame->repeat_pict == 1) { > qf->surface.Info.PicStruct |= MFX_PICSTRUCT_FIELD_REPEATED; > qf->surface.Info.PicStruct |= qf->frame->top_field_first ? > MFX_PICSTRUCT_FIELD_TFF : MFX_PICSTRUCT_FIELD_BFF; > } > > > else if (qf->frame->repeat_pict == 2) Thanks, it seems more reasonable in the repeated_first_field cases in #7701. But I still have concerns: Progressive| TFF or Progressive | BFF frames will never be set after the modification. A discussion on " Is progresive TFF or BFF in mpeg2" : https://forum.doom9.org/showthread.php?t=165176 I think it is a more common way to suit all probable cases (not only in #7701): Allow FFmpeg qsv to set all attributes got from each frame, and pass down to MSDK/driver. It depends on MSDK/driver to decide whether the TFF and BFF flag is valid and useful in progressive mode. -- Linjie
> > > Currently, BFF or TFF will not be set if the frame is progressive. > > > This will break the Input PicStruct check in MSDK because of absence > > > of the specific PicStruct check for: > > > MFX_PICSTRUCT_PROGRESSIVE | MFX_PICSTRUCT_FIELD_REPEATED > > > > > > Set PicStruct to MFX_PICSTRUCT_FIELD_TFF or > > MFX_PICSTRUCT_FIELD_BFF > > > to match the CheckInputPicStruct in MSDK. > > > > > > Fix #7701. > > > > After checking this ticket, I believe this is not a MSDK issue, and > > current fix in this patch is not correct. > > If I understand correctly, this issue only happens when H264 > > pic_struct = 5 or 6. > > In this case, MFX_PICSTRUCT_FIELD_TFF or MFX_PICSTRUCT_FIELD_BFF > > should be set, else we don't know which filed should be repeated. > > > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > > --- > > > Is it acceptable to add the TFF or BFF to PicStruct according to the > > > attribute of the input frames, even if it's not interlaced? > > > Or it should be fixed in MSDK level to support more PicStruct? > > > > > > libavfilter/vf_deinterlace_qsv.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/libavfilter/vf_deinterlace_qsv.c > > b/libavfilter/vf_deinterlace_qsv.c > > > index d6b02e98c5..f03d65f029 100644 > > > --- a/libavfilter/vf_deinterlace_qsv.c > > > +++ b/libavfilter/vf_deinterlace_qsv.c > > > @@ -417,8 +417,9 @@ static int submit_frame(AVFilterContext *ctx, > > > AVFrame *frame, > > > qf->surface.Info.CropH = qf->frame->height; > > > > > > qf->surface.Info.PicStruct = !qf->frame->interlaced_frame ? > > > MFX_PICSTRUCT_PROGRESSIVE : > > > - (qf->frame->top_field_first ? > > > MFX_PICSTRUCT_FIELD_TFF : > > > - > > > MFX_PICSTRUCT_FIELD_BFF); > > > + > > > MFX_PICSTRUCT_UNKNOWN; > > > + qf->surface.Info.PicStruct |= qf->frame->top_field_first ? > > > MFX_PICSTRUCT_FIELD_TFF : > > > + > > > + MFX_PICSTRUCT_FIELD_BFF; > > > if (qf->frame->repeat_pict == 1) > > > qf->surface.Info.PicStruct |= > MFX_PICSTRUCT_FIELD_REPEATED; > > > > I believe the correct fix should be here (previous code change is not > > needed and should be removed): > > if (qf->frame->repeat_pict == 1) { > > qf->surface.Info.PicStruct |= MFX_PICSTRUCT_FIELD_REPEATED; > > qf->surface.Info.PicStruct |= qf->frame->top_field_first ? > > MFX_PICSTRUCT_FIELD_TFF : MFX_PICSTRUCT_FIELD_BFF; } > > > > > else if (qf->frame->repeat_pict == 2) > > Thanks, it seems more reasonable in the repeated_first_field cases in #7701. > > But I still have concerns: > Progressive| TFF or Progressive | BFF frames will never be set after the > modification. > > A discussion on " Is progresive TFF or BFF in mpeg2" : > https://forum.doom9.org/showthread.php?t=165176 > > I think it is a more common way to suit all probable cases (not only in > #7701): > Allow FFmpeg qsv to set all attributes got from each frame, and pass down > to MSDK/driver. > It depends on MSDK/driver to decide whether the TFF and BFF flag is valid > and useful in progressive mode. > > -- > Linjie top_field_first is meaningless as ffmpeg decoder documentation and AVFrame comments. Thus means probably you are change MFX_PICSTRUCT_PROGRESSIVE to MFX_PICSTRUCT_PROGRESSIVE| MFX_PICSTRUCT_FIELD_BFF for all cases, is it as expectation? For MFX_PICSTRUCT_FIELD_REPEATED case, it is a different story, you must tell which filed should be repeated.
diff --git a/libavfilter/vf_deinterlace_qsv.c b/libavfilter/vf_deinterlace_qsv.c index d6b02e98c5..f03d65f029 100644 --- a/libavfilter/vf_deinterlace_qsv.c +++ b/libavfilter/vf_deinterlace_qsv.c @@ -417,8 +417,9 @@ static int submit_frame(AVFilterContext *ctx, AVFrame *frame, qf->surface.Info.CropH = qf->frame->height; qf->surface.Info.PicStruct = !qf->frame->interlaced_frame ? MFX_PICSTRUCT_PROGRESSIVE : - (qf->frame->top_field_first ? MFX_PICSTRUCT_FIELD_TFF : - MFX_PICSTRUCT_FIELD_BFF); + MFX_PICSTRUCT_UNKNOWN; + qf->surface.Info.PicStruct |= qf->frame->top_field_first ? MFX_PICSTRUCT_FIELD_TFF : + MFX_PICSTRUCT_FIELD_BFF; if (qf->frame->repeat_pict == 1) qf->surface.Info.PicStruct |= MFX_PICSTRUCT_FIELD_REPEATED; else if (qf->frame->repeat_pict == 2)
Currently, BFF or TFF will not be set if the frame is progressive. This will break the Input PicStruct check in MSDK because of absence of the specific PicStruct check for: MFX_PICSTRUCT_PROGRESSIVE | MFX_PICSTRUCT_FIELD_REPEATED Set PicStruct to MFX_PICSTRUCT_FIELD_TFF or MFX_PICSTRUCT_FIELD_BFF to match the CheckInputPicStruct in MSDK. Fix #7701. Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- Is it acceptable to add the TFF or BFF to PicStruct according to the attribute of the input frames, even if it's not interlaced? Or it should be fixed in MSDK level to support more PicStruct? libavfilter/vf_deinterlace_qsv.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)