[FFmpeg-devel] [PATCH, RFC] lavf/deinterlace_qsv: set TFF or BFF together with progressive

[Li, Zhong]

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