[FFmpeg-devel,V2] lavc/qsvenc: add quality status to side_data

Submitted by Zhong Li on Aug. 10, 2018, 8:10 a.m.

Details

Message ID 1533888634-9533-1-git-send-email-zhong.li@intel.com
State New
Headers show

Commit Message

Zhong Li Aug. 10, 2018, 8:10 a.m.
Add fix a memory leak issue as James's comments.

V2: use a local pict_type since coded_frame is deprecated.

Signed-off-by: Zhong Li <zhong.li@intel.com>
---
 libavcodec/qsvenc.c      | 32 ++++++++++++++++++++++----------
 libavcodec/qsvenc.h      |  2 --
 libavcodec/qsvenc_h264.c |  5 -----
 3 files changed, 22 insertions(+), 17 deletions(-)

Comments

James Almer Aug. 10, 2018, 1:13 p.m.
On 8/10/2018 5:10 AM, Zhong Li wrote:
> Add fix a memory leak issue as James's comments.
> 
> V2: use a local pict_type since coded_frame is deprecated.
> 
> Signed-off-by: Zhong Li <zhong.li@intel.com>
> ---
>  libavcodec/qsvenc.c      | 32 ++++++++++++++++++++++----------
>  libavcodec/qsvenc.h      |  2 --
>  libavcodec/qsvenc_h264.c |  5 -----
>  3 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index 65dae31..0a14e6a 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1200,8 +1200,10 @@ static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
>      if (!sync) {
>          av_freep(&bs);
>   #if QSV_VERSION_ATLEAST(1, 26)
> -        if (avctx->codec_id == AV_CODEC_ID_H264)
> +        if (avctx->codec_id == AV_CODEC_ID_H264) {
>              av_freep(&enc_info);
> +            av_freep(&enc_buf);
> +        }
>   #endif
>          av_packet_unref(&new_pkt);
>          return AVERROR(ENOMEM);
> @@ -1220,8 +1222,10 @@ static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
>          av_packet_unref(&new_pkt);
>          av_freep(&bs);
>  #if QSV_VERSION_ATLEAST(1, 26)
> -        if (avctx->codec_id == AV_CODEC_ID_H264)
> +        if (avctx->codec_id == AV_CODEC_ID_H264) {
>              av_freep(&enc_info);
> +            av_freep(&enc_buf);
> +        }
>  #endif
>          av_freep(&sync);
>          return (ret == MFX_ERR_MORE_DATA) ?
> @@ -1240,8 +1244,10 @@ static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
>          av_packet_unref(&new_pkt);
>          av_freep(&bs);
>  #if QSV_VERSION_ATLEAST(1, 26)
> -        if (avctx->codec_id == AV_CODEC_ID_H264)
> +        if (avctx->codec_id == AV_CODEC_ID_H264) {
>              av_freep(&enc_info);
> +            av_freep(&enc_buf);
> +        }
>  #endif
>      }
>  
> @@ -1264,7 +1270,9 @@ int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q,
>          mfxSyncPoint *sync;
>  #if QSV_VERSION_ATLEAST(1, 26)
>          mfxExtAVCEncodedFrameInfo *enc_info;
> +        mfxExtBuffer **enc_buf;
>  #endif
> +        enum AVPictureType pict_type;
>  
>          av_fifo_generic_read(q->async_fifo, &new_pkt, sizeof(new_pkt), NULL);
>          av_fifo_generic_read(q->async_fifo, &sync,    sizeof(sync),    NULL);
> @@ -1282,23 +1290,27 @@ int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q,
>              bs->FrameType & MFX_FRAMETYPE_xIDR)
>              new_pkt.flags |= AV_PKT_FLAG_KEY;
>  
> -#if FF_API_CODED_FRAME
> -FF_DISABLE_DEPRECATION_WARNINGS
>          if (bs->FrameType & MFX_FRAMETYPE_I || bs->FrameType & MFX_FRAMETYPE_xI)
> -            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;
> +            pict_type = AV_PICTURE_TYPE_I;
>          else if (bs->FrameType & MFX_FRAMETYPE_P || bs->FrameType & MFX_FRAMETYPE_xP)
> -            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_P;
> +            pict_type = AV_PICTURE_TYPE_P;
>          else if (bs->FrameType & MFX_FRAMETYPE_B || bs->FrameType & MFX_FRAMETYPE_xB)
> -            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_B;
> +            pict_type = AV_PICTURE_TYPE_B;
> +
> +#if FF_API_CODED_FRAME
> +FF_DISABLE_DEPRECATION_WARNINGS
> +        avctx->coded_frame->pict_type = pict_type;
>  FF_ENABLE_DEPRECATION_WARNINGS
>  #endif
>  
>  #if QSV_VERSION_ATLEAST(1, 26)
>          if (avctx->codec_id == AV_CODEC_ID_H264) {
> +            enc_buf = bs->ExtParam;
>              enc_info = (mfxExtAVCEncodedFrameInfo *)(*bs->ExtParam);
> -            av_log(avctx, AV_LOG_DEBUG, "QP is %d\n", enc_info->QP);
> -            q->sum_frame_qp += enc_info->QP;
> +            ff_side_data_set_encoder_stats(&new_pkt,
> +                enc_info->QP * FF_QP2LAMBDA, NULL, 0, pict_type);
>              av_freep(&enc_info);
> +            av_freep(&enc_buf);
>          }
>  #endif
>          av_freep(&bs);
> diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h
> index 3784a82..b2d6355 100644
> --- a/libavcodec/qsvenc.h
> +++ b/libavcodec/qsvenc.h
> @@ -102,8 +102,6 @@ typedef struct QSVEncContext {
>      int width_align;
>      int height_align;
>  
> -    int sum_frame_qp;
> -
>      mfxVideoParam param;
>      mfxFrameAllocRequest req;
>  
> diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c
> index b87bef6..5c262e5 100644
> --- a/libavcodec/qsvenc_h264.c
> +++ b/libavcodec/qsvenc_h264.c
> @@ -95,11 +95,6 @@ static av_cold int qsv_enc_close(AVCodecContext *avctx)
>  {
>      QSVH264EncContext *q = avctx->priv_data;
>  
> -#if QSV_VERSION_ATLEAST(1, 26)
> -    av_log(avctx, AV_LOG_VERBOSE, "encoded %d frames, avarge qp is %.2f\n",
> -        avctx->frame_number,(double)q->qsv.sum_frame_qp / avctx->frame_number);
> -#endif
> -
>      return ff_qsv_enc_close(avctx, &q->qsv);
>  }

Can't test, but looks ok.
Zhong Li Aug. 13, 2018, 11:07 a.m.
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of James Almer

> Sent: Friday, August 10, 2018 9:13 PM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH V2] lavc/qsvenc: add quality status to

> side_data

> 

> On 8/10/2018 5:10 AM, Zhong Li wrote:

> > Add fix a memory leak issue as James's comments.

> >

> > V2: use a local pict_type since coded_frame is deprecated.

> >

> > Signed-off-by: Zhong Li <zhong.li@intel.com>

> > ---

> >  libavcodec/qsvenc.c      | 32 ++++++++++++++++++++++----------

> >  libavcodec/qsvenc.h      |  2 --

> >  libavcodec/qsvenc_h264.c |  5 -----

> >  3 files changed, 22 insertions(+), 17 deletions(-)

> >

> > diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c index

> > 65dae31..0a14e6a 100644

> > --- a/libavcodec/qsvenc.c

> > +++ b/libavcodec/qsvenc.c

> > @@ -1200,8 +1200,10 @@ static int encode_frame(AVCodecContext

> *avctx, QSVEncContext *q,

> >      if (!sync) {

> >          av_freep(&bs);

> >   #if QSV_VERSION_ATLEAST(1, 26)

> > -        if (avctx->codec_id == AV_CODEC_ID_H264)

> > +        if (avctx->codec_id == AV_CODEC_ID_H264) {

> >              av_freep(&enc_info);

> > +            av_freep(&enc_buf);

> > +        }

> >   #endif

> >          av_packet_unref(&new_pkt);

> >          return AVERROR(ENOMEM);

> > @@ -1220,8 +1222,10 @@ static int encode_frame(AVCodecContext

> *avctx, QSVEncContext *q,

> >          av_packet_unref(&new_pkt);

> >          av_freep(&bs);

> >  #if QSV_VERSION_ATLEAST(1, 26)

> > -        if (avctx->codec_id == AV_CODEC_ID_H264)

> > +        if (avctx->codec_id == AV_CODEC_ID_H264) {

> >              av_freep(&enc_info);

> > +            av_freep(&enc_buf);

> > +        }

> >  #endif

> >          av_freep(&sync);

> >          return (ret == MFX_ERR_MORE_DATA) ?

> > @@ -1240,8 +1244,10 @@ static int encode_frame(AVCodecContext

> *avctx, QSVEncContext *q,

> >          av_packet_unref(&new_pkt);

> >          av_freep(&bs);

> >  #if QSV_VERSION_ATLEAST(1, 26)

> > -        if (avctx->codec_id == AV_CODEC_ID_H264)

> > +        if (avctx->codec_id == AV_CODEC_ID_H264) {

> >              av_freep(&enc_info);

> > +            av_freep(&enc_buf);

> > +        }

> >  #endif

> >      }

> >

> > @@ -1264,7 +1270,9 @@ int ff_qsv_encode(AVCodecContext *avctx,

> QSVEncContext *q,

> >          mfxSyncPoint *sync;

> >  #if QSV_VERSION_ATLEAST(1, 26)

> >          mfxExtAVCEncodedFrameInfo *enc_info;

> > +        mfxExtBuffer **enc_buf;

> >  #endif

> > +        enum AVPictureType pict_type;

> >

> >          av_fifo_generic_read(q->async_fifo, &new_pkt,

> sizeof(new_pkt), NULL);

> >          av_fifo_generic_read(q->async_fifo, &sync,    sizeof(sync),

> NULL);

> > @@ -1282,23 +1290,27 @@ int ff_qsv_encode(AVCodecContext *avctx,

> QSVEncContext *q,

> >              bs->FrameType & MFX_FRAMETYPE_xIDR)

> >              new_pkt.flags |= AV_PKT_FLAG_KEY;

> >

> > -#if FF_API_CODED_FRAME

> > -FF_DISABLE_DEPRECATION_WARNINGS

> >          if (bs->FrameType & MFX_FRAMETYPE_I || bs->FrameType &

> MFX_FRAMETYPE_xI)

> > -            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;

> > +            pict_type = AV_PICTURE_TYPE_I;

> >          else if (bs->FrameType & MFX_FRAMETYPE_P ||

> bs->FrameType & MFX_FRAMETYPE_xP)

> > -            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_P;

> > +            pict_type = AV_PICTURE_TYPE_P;

> >          else if (bs->FrameType & MFX_FRAMETYPE_B ||

> bs->FrameType & MFX_FRAMETYPE_xB)

> > -            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_B;

> > +            pict_type = AV_PICTURE_TYPE_B;

> > +

> > +#if FF_API_CODED_FRAME

> > +FF_DISABLE_DEPRECATION_WARNINGS

> > +        avctx->coded_frame->pict_type = pict_type;

> >  FF_ENABLE_DEPRECATION_WARNINGS

> >  #endif

> >

> >  #if QSV_VERSION_ATLEAST(1, 26)

> >          if (avctx->codec_id == AV_CODEC_ID_H264) {

> > +            enc_buf = bs->ExtParam;

> >              enc_info = (mfxExtAVCEncodedFrameInfo

> *)(*bs->ExtParam);

> > -            av_log(avctx, AV_LOG_DEBUG, "QP is %d\n",

> enc_info->QP);

> > -            q->sum_frame_qp += enc_info->QP;

> > +            ff_side_data_set_encoder_stats(&new_pkt,

> > +                enc_info->QP * FF_QP2LAMBDA, NULL, 0, pict_type);

> >              av_freep(&enc_info);

> > +            av_freep(&enc_buf);

> >          }

> >  #endif

> >          av_freep(&bs);

> > diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h index

> > 3784a82..b2d6355 100644

> > --- a/libavcodec/qsvenc.h

> > +++ b/libavcodec/qsvenc.h

> > @@ -102,8 +102,6 @@ typedef struct QSVEncContext {

> >      int width_align;

> >      int height_align;

> >

> > -    int sum_frame_qp;

> > -

> >      mfxVideoParam param;

> >      mfxFrameAllocRequest req;

> >

> > diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c index

> > b87bef6..5c262e5 100644

> > --- a/libavcodec/qsvenc_h264.c

> > +++ b/libavcodec/qsvenc_h264.c

> > @@ -95,11 +95,6 @@ static av_cold int qsv_enc_close(AVCodecContext

> > *avctx)  {

> >      QSVH264EncContext *q = avctx->priv_data;

> >

> > -#if QSV_VERSION_ATLEAST(1, 26)

> > -    av_log(avctx, AV_LOG_VERBOSE, "encoded %d frames, avarge qp

> is %.2f\n",

> > -        avctx->frame_number,(double)q->qsv.sum_frame_qp /

> avctx->frame_number);

> > -#endif

> > -

> >      return ff_qsv_enc_close(avctx, &q->qsv);  }

> 

> Can't test, but looks ok.

Thanks. Will apply after my regression test done.

Patch hide | download patch | download mbox

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index 65dae31..0a14e6a 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1200,8 +1200,10 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
     if (!sync) {
         av_freep(&bs);
  #if QSV_VERSION_ATLEAST(1, 26)
-        if (avctx->codec_id == AV_CODEC_ID_H264)
+        if (avctx->codec_id == AV_CODEC_ID_H264) {
             av_freep(&enc_info);
+            av_freep(&enc_buf);
+        }
  #endif
         av_packet_unref(&new_pkt);
         return AVERROR(ENOMEM);
@@ -1220,8 +1222,10 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
         av_packet_unref(&new_pkt);
         av_freep(&bs);
 #if QSV_VERSION_ATLEAST(1, 26)
-        if (avctx->codec_id == AV_CODEC_ID_H264)
+        if (avctx->codec_id == AV_CODEC_ID_H264) {
             av_freep(&enc_info);
+            av_freep(&enc_buf);
+        }
 #endif
         av_freep(&sync);
         return (ret == MFX_ERR_MORE_DATA) ?
@@ -1240,8 +1244,10 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
         av_packet_unref(&new_pkt);
         av_freep(&bs);
 #if QSV_VERSION_ATLEAST(1, 26)
-        if (avctx->codec_id == AV_CODEC_ID_H264)
+        if (avctx->codec_id == AV_CODEC_ID_H264) {
             av_freep(&enc_info);
+            av_freep(&enc_buf);
+        }
 #endif
     }
 
@@ -1264,7 +1270,9 @@  int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q,
         mfxSyncPoint *sync;
 #if QSV_VERSION_ATLEAST(1, 26)
         mfxExtAVCEncodedFrameInfo *enc_info;
+        mfxExtBuffer **enc_buf;
 #endif
+        enum AVPictureType pict_type;
 
         av_fifo_generic_read(q->async_fifo, &new_pkt, sizeof(new_pkt), NULL);
         av_fifo_generic_read(q->async_fifo, &sync,    sizeof(sync),    NULL);
@@ -1282,23 +1290,27 @@  int ff_qsv_encode(AVCodecContext *avctx, QSVEncContext *q,
             bs->FrameType & MFX_FRAMETYPE_xIDR)
             new_pkt.flags |= AV_PKT_FLAG_KEY;
 
-#if FF_API_CODED_FRAME
-FF_DISABLE_DEPRECATION_WARNINGS
         if (bs->FrameType & MFX_FRAMETYPE_I || bs->FrameType & MFX_FRAMETYPE_xI)
-            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_I;
+            pict_type = AV_PICTURE_TYPE_I;
         else if (bs->FrameType & MFX_FRAMETYPE_P || bs->FrameType & MFX_FRAMETYPE_xP)
-            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_P;
+            pict_type = AV_PICTURE_TYPE_P;
         else if (bs->FrameType & MFX_FRAMETYPE_B || bs->FrameType & MFX_FRAMETYPE_xB)
-            avctx->coded_frame->pict_type = AV_PICTURE_TYPE_B;
+            pict_type = AV_PICTURE_TYPE_B;
+
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+        avctx->coded_frame->pict_type = pict_type;
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif
 
 #if QSV_VERSION_ATLEAST(1, 26)
         if (avctx->codec_id == AV_CODEC_ID_H264) {
+            enc_buf = bs->ExtParam;
             enc_info = (mfxExtAVCEncodedFrameInfo *)(*bs->ExtParam);
-            av_log(avctx, AV_LOG_DEBUG, "QP is %d\n", enc_info->QP);
-            q->sum_frame_qp += enc_info->QP;
+            ff_side_data_set_encoder_stats(&new_pkt,
+                enc_info->QP * FF_QP2LAMBDA, NULL, 0, pict_type);
             av_freep(&enc_info);
+            av_freep(&enc_buf);
         }
 #endif
         av_freep(&bs);
diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h
index 3784a82..b2d6355 100644
--- a/libavcodec/qsvenc.h
+++ b/libavcodec/qsvenc.h
@@ -102,8 +102,6 @@  typedef struct QSVEncContext {
     int width_align;
     int height_align;
 
-    int sum_frame_qp;
-
     mfxVideoParam param;
     mfxFrameAllocRequest req;
 
diff --git a/libavcodec/qsvenc_h264.c b/libavcodec/qsvenc_h264.c
index b87bef6..5c262e5 100644
--- a/libavcodec/qsvenc_h264.c
+++ b/libavcodec/qsvenc_h264.c
@@ -95,11 +95,6 @@  static av_cold int qsv_enc_close(AVCodecContext *avctx)
 {
     QSVH264EncContext *q = avctx->priv_data;
 
-#if QSV_VERSION_ATLEAST(1, 26)
-    av_log(avctx, AV_LOG_VERBOSE, "encoded %d frames, avarge qp is %.2f\n",
-        avctx->frame_number,(double)q->qsv.sum_frame_qp / avctx->frame_number);
-#endif
-
     return ff_qsv_enc_close(avctx, &q->qsv);
 }