diff mbox

[FFmpeg-devel,v3] lavc/qsvenc: add forced_idr opiton

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

Commit Message

Zhong Li Nov. 13, 2018, 11:58 a.m. UTC 
This option can be used to repect original input I/IDR frame type.

Signed-off-by: Zhong Li <zhong.li@intel.com>
---
 libavcodec/qsvenc.c | 7 +++++++
 libavcodec/qsvenc.h | 2 ++
 2 files changed, 9 insertions(+)

Comments

Rogozhkin, Dmitry V Nov. 13, 2018, 6:44 p.m. UTC | #1 
On Tue, 2018-11-13 at 19:58 +0800, Zhong Li wrote:
> This option can be used to repect original input I/IDR frame type.

> 

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

> ---

>  libavcodec/qsvenc.c | 7 +++++++

>  libavcodec/qsvenc.h | 2 ++

>  2 files changed, 9 insertions(+)

> 

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

> index 948751d..63d0939 100644

> --- a/libavcodec/qsvenc.c

> +++ b/libavcodec/qsvenc.c

> @@ -1192,6 +1192,13 @@ static int encode_frame(AVCodecContext *avctx,

> QSVEncContext *q,

>      if (qsv_frame) {

>          surf = &qsv_frame->surface;

>          enc_ctrl = &qsv_frame->enc_ctrl;

> +        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));

> +

> +        if (frame->pict_type == AV_PICTURE_TYPE_I) {

> +            enc_ctrl->FrameType = MFX_FRAMETYPE_I |

> MFX_FRAMETYPE_REF;

> +            if (q->forced_idr || frame->key_frame)

> +                enc_ctrl->FrameType |= MFX_FRAMETYPE_IDR;

> +        }

>      }

>  

>      ret = av_new_packet(&new_pkt, q->packet_size);

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

> index 055b4a6..fbdc199 100644

> --- a/libavcodec/qsvenc.h

> +++ b/libavcodec/qsvenc.h

> @@ -87,6 +87,7 @@

>  { "adaptive_i",     "Adaptive I-frame

> placement",             OFFSET(qsv.adaptive_i),     AV_OPT_TYPE_INT,

> { .i64 = -1 }, -1,          1, VE },                         \

>  { "adaptive_b",     "Adaptive B-frame

> placement",             OFFSET(qsv.adaptive_b),     AV_OPT_TYPE_INT,

> { .i64 = -1 }, -1,          1, VE },                         \

>  { "b_strategy",     "Strategy to choose between I/P/B-frames",

> OFFSET(qsv.b_strategy),    AV_OPT_TYPE_INT, { .i64 = -1 },

> -1,          1, VE },                         \

> +{ "forced_idr",     "Forcing I frames as IDR

> frames",         OFFSET(qsv.forced_idr),     AV_OPT_TYPE_BOOL,{ .i64

> = 0  },  0,          1, VE },                         \

>  

>  typedef int SetEncodeCtrlCB (AVCodecContext *avctx,

>                               const AVFrame *frame, mfxEncodeCtrl*

> enc_ctrl);

> @@ -168,6 +169,7 @@ typedef struct QSVEncContext {

>  #endif

>      char *load_plugins;

>      SetEncodeCtrlCB *set_encode_ctrl_cb;

> +    int forced_idr;

>  } QSVEncContext;

>  

>  int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q);


s/opiton/option/ in commit first line. :)

Other than that, looks good to me.
Mark Thompson Nov. 13, 2018, 9:47 p.m. UTC | #2 
On 13/11/18 11:58, Zhong Li wrote:
> This option can be used to repect original input I/IDR frame type.
> 
> Signed-off-by: Zhong Li <zhong.li@intel.com>
> ---
>  libavcodec/qsvenc.c | 7 +++++++
>  libavcodec/qsvenc.h | 2 ++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
> index 948751d..63d0939 100644
> --- a/libavcodec/qsvenc.c
> +++ b/libavcodec/qsvenc.c
> @@ -1192,6 +1192,13 @@ static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
>      if (qsv_frame) {
>          surf = &qsv_frame->surface;
>          enc_ctrl = &qsv_frame->enc_ctrl;
> +        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));
> +
> +        if (frame->pict_type == AV_PICTURE_TYPE_I) {
> +            enc_ctrl->FrameType = MFX_FRAMETYPE_I | MFX_FRAMETYPE_REF;
> +            if (q->forced_idr || frame->key_frame)
> +                enc_ctrl->FrameType |= MFX_FRAMETYPE_IDR;
> +        }
>      }
>  
>      ret = av_new_packet(&new_pkt, q->packet_size);
> diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h
> index 055b4a6..fbdc199 100644
> --- a/libavcodec/qsvenc.h
> +++ b/libavcodec/qsvenc.h
> @@ -87,6 +87,7 @@
>  { "adaptive_i",     "Adaptive I-frame placement",             OFFSET(qsv.adaptive_i),     AV_OPT_TYPE_INT, { .i64 = -1 }, -1,          1, VE },                         \
>  { "adaptive_b",     "Adaptive B-frame placement",             OFFSET(qsv.adaptive_b),     AV_OPT_TYPE_INT, { .i64 = -1 }, -1,          1, VE },                         \
>  { "b_strategy",     "Strategy to choose between I/P/B-frames", OFFSET(qsv.b_strategy),    AV_OPT_TYPE_INT, { .i64 = -1 }, -1,          1, VE },                         \
> +{ "forced_idr",     "Forcing I frames as IDR frames",         OFFSET(qsv.forced_idr),     AV_OPT_TYPE_BOOL,{ .i64 = 0  },  0,          1, VE },                         \
>  
>  typedef int SetEncodeCtrlCB (AVCodecContext *avctx,
>                               const AVFrame *frame, mfxEncodeCtrl* enc_ctrl);
> @@ -168,6 +169,7 @@ typedef struct QSVEncContext {
>  #endif
>      char *load_plugins;
>      SetEncodeCtrlCB *set_encode_ctrl_cb;
> +    int forced_idr;
>  } QSVEncContext;
>  
>  int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q);
> 

Testing this, I think the reason not to look at key_frame in an encoder is that it isn't usually written by the user.  Rather, it gets copied (including through copy_props) from whatever the original source of the frame was.

You can see that it results in surprising behaviour in ffmpeg (which doesn't touch that field) with your patch as-is:

$ ffprobe h264.mp4
...
    Stream #0:0(und): Video: h264 (High) (avc1 / 0x31637661), yuv420p, 1920x1080 [SAR 1:1 DAR 16:9], 11244 kb/s, 60 fps, 60 tbr, 15360 tbn, 120 tbc (default)
...
$ ffprobe jpeg.mp4
...
    Stream #0:0(und): Video: mjpeg (Baseline) (mp4v / 0x7634706D), yuvj420p(pc, bt470bg/unknown/unknown, progressive), 1920x1080, 92213 kb/s, SAR 1:1 DAR 16:9, 60 fps, 60 tbr, 15360 tbn, 15360 tbc (default)
...
$ LIBVA_DRIVER_NAME=iHD ./ffmpeg_g -y -i h264.mp4 -an -c:v h264_qsv -force_key_frames expr:1 -b:v 1M -frames:v 10 -bsf:v trace_headers -f null - 2>&1 | grep 'nal_unit_type.*\(1\|5\)$' 
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
[AVBSFContext @ 0x55dd71352700] 3           nal_unit_type                                           00001 = 1
$ LIBVA_DRIVER_NAME=iHD ./ffmpeg_g -y -i jpeg.mp4 -an -c:v h264_qsv -force_key_frames expr:1 -b:v 1M -frames:v 10 -bsf:v trace_headers -f null - 2>&1 | grep 'nal_unit_type.*\(1\|5\)$' 
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x55989b59b540] 3           nal_unit_type                                           00101 = 5
$ LIBVA_DRIVER_NAME=iHD ./ffmpeg_g -y -i h264.mp4 -an -c:v h264_qsv -force_key_frames expr:1 -b:v 1M -forced_idr 1 -frames:v 10 -bsf:v trace_headers -f null - 2>&1 | grep 'nal_unit_type.*\(1\|5\)$' 
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5
[AVBSFContext @ 0x560d28291180] 3           nal_unit_type                                           00101 = 5

So, LGTM with the reference to key_frame dropped and the title typo fixed.

Thanks,

- Mark
diff mbox

Patch

diff --git a/libavcodec/qsvenc.c b/libavcodec/qsvenc.c
index 948751d..63d0939 100644
--- a/libavcodec/qsvenc.c
+++ b/libavcodec/qsvenc.c
@@ -1192,6 +1192,13 @@  static int encode_frame(AVCodecContext *avctx, QSVEncContext *q,
     if (qsv_frame) {
         surf = &qsv_frame->surface;
         enc_ctrl = &qsv_frame->enc_ctrl;
+        memset(enc_ctrl, 0, sizeof(mfxEncodeCtrl));
+
+        if (frame->pict_type == AV_PICTURE_TYPE_I) {
+            enc_ctrl->FrameType = MFX_FRAMETYPE_I | MFX_FRAMETYPE_REF;
+            if (q->forced_idr || frame->key_frame)
+                enc_ctrl->FrameType |= MFX_FRAMETYPE_IDR;
+        }
     }
 
     ret = av_new_packet(&new_pkt, q->packet_size);
diff --git a/libavcodec/qsvenc.h b/libavcodec/qsvenc.h
index 055b4a6..fbdc199 100644
--- a/libavcodec/qsvenc.h
+++ b/libavcodec/qsvenc.h
@@ -87,6 +87,7 @@ 
 { "adaptive_i",     "Adaptive I-frame placement",             OFFSET(qsv.adaptive_i),     AV_OPT_TYPE_INT, { .i64 = -1 }, -1,          1, VE },                         \
 { "adaptive_b",     "Adaptive B-frame placement",             OFFSET(qsv.adaptive_b),     AV_OPT_TYPE_INT, { .i64 = -1 }, -1,          1, VE },                         \
 { "b_strategy",     "Strategy to choose between I/P/B-frames", OFFSET(qsv.b_strategy),    AV_OPT_TYPE_INT, { .i64 = -1 }, -1,          1, VE },                         \
+{ "forced_idr",     "Forcing I frames as IDR frames",         OFFSET(qsv.forced_idr),     AV_OPT_TYPE_BOOL,{ .i64 = 0  },  0,          1, VE },                         \
 
 typedef int SetEncodeCtrlCB (AVCodecContext *avctx,
                              const AVFrame *frame, mfxEncodeCtrl* enc_ctrl);
@@ -168,6 +169,7 @@  typedef struct QSVEncContext {
 #endif
     char *load_plugins;
     SetEncodeCtrlCB *set_encode_ctrl_cb;
+    int forced_idr;
 } QSVEncContext;
 
 int ff_qsv_enc_init(AVCodecContext *avctx, QSVEncContext *q);