| Message ID | AM7PR03MB6660F538BEC127CA927447F68FA69@AM7PR03MB6660.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Commit | 1cdbccaa163eb3bd50403ecc75fc3da9d5d75c02 |
| Headers | show |
| Series | [FFmpeg-devel,1/6] avcodec/qsvenc: Fix leak and crash when encoding H.264 due to A53_CC | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
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@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 Haihao
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@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. - Andreas
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@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.
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@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 >
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; }
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@outlook.com> --- libavcodec/qsv_internal.h | 2 ++ libavcodec/qsvenc.c | 10 +++------- 2 files changed, 5 insertions(+), 7 deletions(-)