[FFmpeg-devel] [PATCH 1/6] avcodec/qsvenc: Fix leak and crash when encoding H.264 due to A53_CC

[Xiang, Haihao]

On Mon, 2021-09-27 at 08:41 +0000, Xiang, Haihao wrote:
> On Mon, 2021-09-27 at 10:21 +0200, Andreas Rheinhardt wrote:
> > Xiang, Haihao:
> > > On Sun, 2021-09-26 at 08:32 +0200, Andreas Rheinhardt wrote:
> > > > Since commit 3bbe0c210b05fc6fbd7b1d4bbd8479db7f2cf957, the Payloads
> > > > array of every QSVFrame leaks as soon as the frame is reused;
> > > > the leak is small and not very noticeable, but if there is an attempt
> > > > to use said array the ensuing crash is much more noticeable.
> > > > This happens when encoding H.264 with A53 CC side data.
> > > > 
> > > > Furthermore, if said array can not be allocated at all, an AVFrame
> > > > leaks.
> > > > 
> > > > Fix all of this by not allocating the array separately at all; put it
> > > > in QSVFrame instead and restore the Payloads array upon reusing the
> > > > frame.
> > > > 
> > > > Finally, use av_freep() instead of av_free() to free the payload
> > > > entries.
> > > > 
> > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> > > > ---
> > > >  libavcodec/qsv_internal.h |  2 ++
> > > >  libavcodec/qsvenc.c       | 10 +++-------
> > > >  2 files changed, 5 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/libavcodec/qsv_internal.h b/libavcodec/qsv_internal.h
> > > > index 8090b748b3..fe9d5319c4 100644
> > > > --- a/libavcodec/qsv_internal.h
> > > > +++ b/libavcodec/qsv_internal.h
> > > > @@ -76,6 +76,8 @@ typedef struct QSVFrame {
> > > >      mfxExtDecodedFrameInfo dec_info;
> > > >      mfxExtBuffer *ext_param;
> > > >  
> > > > +    mfxPayload *payloads[QSV_MAX_ENC_PAYLOAD]; ///< used for
> > > > enc_ctrl.Payload
> > > > +
> > > >      int queued;
> > > >      int used;
> > > >  
> > > > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> > > > index 06f55604b5..66f79bb021 100644
> > > > --- a/libavcodec/qsvenc.c
> > > > +++ b/libavcodec/qsvenc.c
> > > > @@ -1259,7 +1259,7 @@ static void
> > > > free_encoder_ctrl_payloads(mfxEncodeCtrl*
> > > > enc_ctrl)
> > > >      if (enc_ctrl) {
> > > >          int i;
> > > >          for (i = 0; i < enc_ctrl->NumPayload && i <
> > > > QSV_MAX_ENC_PAYLOAD;
> > > > i++)
> > > > {
> > > > -            av_free(enc_ctrl->Payload[i]);
> > > > +            av_freep(&enc_ctrl->Payload[i]);
> > > >          }
> > > >          enc_ctrl->NumPayload = 0;
> > > >      }
> > > > @@ -1273,6 +1273,7 @@ static void clear_unused_frames(QSVEncContext *q)
> > > >              free_encoder_ctrl_payloads(&cur->enc_ctrl);
> > > >              //do not reuse enc_ctrl from previous frame
> > > >              memset(&cur->enc_ctrl, 0, sizeof(cur->enc_ctrl));
> > > > +            cur->enc_ctrl.Payload = cur->payloads;
> > > >              if (cur->frame->format == AV_PIX_FMT_QSV) {
> > > >                  av_frame_unref(cur->frame);
> > > >              }
> > > > @@ -1309,11 +1310,7 @@ static int get_free_frame(QSVEncContext *q,
> > > > QSVFrame
> > > > **f)
> > > >          av_freep(&frame);
> > > >          return AVERROR(ENOMEM);
> > > >      }
> > > > -    frame->enc_ctrl.Payload = av_mallocz(sizeof(mfxPayload*) *
> > > > QSV_MAX_ENC_PAYLOAD);
> > > > -    if (!frame->enc_ctrl.Payload) {
> > > > -        av_freep(&frame);
> > > > -        return AVERROR(ENOMEM);
> > > > -    }
> > > > +    frame->enc_ctrl.Payload = frame->payloads;
> > > >      *last = frame;
> > > >  
> > > >      *f = frame;
> > > > @@ -1615,7 +1612,6 @@ int ff_qsv_enc_close(AVCodecContext *avctx,
> > > > QSVEncContext *q)
> > > >      while (cur) {
> > > >          q->work_frames = cur->next;
> > > >          av_frame_free(&cur->frame);
> > > > -        av_free(cur->enc_ctrl.Payload);
> > > >          av_freep(&cur);
> > > >          cur = q->work_frames;
> > > >      }
> > > 
> > > I'm sorry commit 3bbe0c2 caused a regression, your patch looks good to me.
> > > 
> > 
> > Thanks for the review. Here is a sample that allows to reproduce the
> > crash: http://streams.videolan.org/streams/ts/CC/NewsStream-608-ac3.ts
> > in case you haven't already have one.
> 
> Thanks for info. 

Hi Andreas, 

Could you please apply your patches? Your patches fixed the issues for us.

Thanks
Haihao

>