[FFmpeg-devel] [PATCH v4 1/3] avutil/hwcontext_qsv: derive QSV frames to D3D11VA frames

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Xiang, Haihao
> Sent: Saturday, May 7, 2022 7:25 AM
> To: ffmpeg-devel at ffmpeg.org
> Cc: Wu, Tong1 <tong1.wu at intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH v4 1/3] avutil/hwcontext_qsv:
> derive QSV frames to D3D11VA frames
> 
> On Sat, 2022-05-07 at 02:42 +0000, Soft Works wrote:
> > > -----Original Message-----
> > > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> > > Xiang, Haihao
> > > Sent: Saturday, May 7, 2022 4:36 AM
> > > To: ffmpeg-devel at ffmpeg.org
> > > Cc: Wu, Tong1 <tong1.wu at intel.com>
> > > Subject: Re: [FFmpeg-devel] [PATCH v4 1/3] avutil/hwcontext_qsv:
> > > derive QSV frames to D3D11VA frames
> > >
> > > On Fri, 2022-05-06 at 05:57 +0000, Tong Wu wrote:
> > > > Fixes:
> > > > $ ffmpeg.exe -y -hwaccel qsv -init_hw_device d3d11va=d3d11 \
> > > > -init_hw_device qsv=qsv at d3d11 -c:v h264_qsv -i input.h264 \
> > > > -vf "hwmap=derive_device=d3d11va,format=d3d11" -f null -
> > > >
> > > > Signed-off-by: Tong Wu <tong1.wu at intel.com>
> > > > ---
> > > >  libavutil/hwcontext_qsv.c | 16 +++++++++++++---
> > > >  1 file changed, 13 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/libavutil/hwcontext_qsv.c
> b/libavutil/hwcontext_qsv.c
> > > > index b28dcffe2a..bf150c8553 100644
> > > > --- a/libavutil/hwcontext_qsv.c
> > > > +++ b/libavutil/hwcontext_qsv.c
> > > > @@ -1281,12 +1281,22 @@ static int
> > >
> > > qsv_frames_derive_from(AVHWFramesContext
> > > > *dst_ctx,
> > > >  #if CONFIG_D3D11VA
> > > >      case AV_HWDEVICE_TYPE_D3D11VA:
> > > >          {
> > > > +            D3D11_TEXTURE2D_DESC texDesc;
> > > > +            dst_ctx->initial_pool_size = src_ctx-
> > > > initial_pool_size;
> > > >              AVD3D11VAFramesContext *dst_hwctx = dst_ctx->hwctx;
> > > > -            mfxHDLPair *pair = (mfxHDLPair*)src_hwctx-
> > > > > surfaces[i].Data.MemId;
> > > >
> > > > -            dst_hwctx->texture = (ID3D11Texture2D*)pair->first;
> > > > +            dst_hwctx->texture_infos = av_calloc(src_hwctx-
> > > > nb_surfaces,
> > > > +
> sizeof(*dst_hwctx-
> > > > > texture_infos));
> > >
> > > Please check whether the pointer is NULL
> > >
> > > >              if (src_hwctx->frame_type &
> > >
> > > MFX_MEMTYPE_SHARED_RESOURCE)
> > > >                  dst_hwctx->MiscFlags =
> D3D11_RESOURCE_MISC_SHARED;
> > > > -            dst_hwctx->BindFlags =
> > >
> > > qsv_get_d3d11va_bind_flags(src_hwctx-
> > > > > frame_type);
> > > >
> > > > +            for (i = 0; i < src_hwctx->nb_surfaces; i++) {
> > > > +                mfxHDLPair *pair = (mfxHDLPair*)src_hwctx-
> > > > > surfaces[i].Data.MemId;
> > > >
> > > > +                dst_hwctx->texture_infos[i].texture =
> > >
> > > (ID3D11Texture2D*)pair-
> > > > > first;
> > > >
> > > > +                dst_hwctx->texture_infos[i].index = pair-
> >second ==
> > > > (mfxMemId)MFX_INFINITE ? (intptr_t)0 : (intptr_t)pair->second;
> > > > +                if (i == 0) {
> > > > +                    ID3D11Texture2D_GetDesc(dst_hwctx-
> > > > > texture_infos[i].texture, &texDesc);
> > > >
> > > > +                }
> > >
> > > Move this out of the for-loop ? You may call
> ID3D11Texture2D_GetDesc()
> > > below:
> > >
> > > ID3D11Texture2D_GetDesc(dst_hwctx->texture_infos[0].texture,
> > > &texDesc);
> >
> > This could crash when src_hwctx->nb_surfaces is 0
> 
> src_hwctx->nb_surface should be greater than 0, see qsv_init_pool()
> and
> qsv_frames_derive_to().
> 
> On the other hand, dst_hwctx->texture_infos is NULL if src_hwctx-
> >nb_surfaces is
> 0. I commented that we should check whether the pointer is NULL. In
> addition,
> texDesc.BindFlags would be uninitialized if src_hwctx->nb_surfaces is
> 0, so we
> couldn't use it in the following assignment.
> 
> dst_hwctx->BindFlags = texDesc.BindFlags;

Yea that’s right. That would have to go into the if block, but
you're right, the whole thing would fail if nb_surfaces is 0
rather it wouldn't even get that far that this function would be 
called, so it really doesn't matter, yet the getdesc and the
BindFlags assignment should better be together (wherever).

Thanks,
softworkz