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

[Fu, Linjie]

> -----Original Message-----
> From: Li, Zhong
> Sent: Monday, February 18, 2019 13:48
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Cc: Fu, Linjie <linjie.fu at 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 at ffmpeg.org] On
> Behalf
> > Of Linjie Fu
> > Sent: Saturday, February 16, 2019 3:50 AM
> > To: ffmpeg-devel at ffmpeg.org
> > Cc: Fu, Linjie <linjie.fu at 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 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