[FFmpeg-devel] lavc/vaapi_encode_h264: respect "slices" option in h264 vaapi encoder

Submitted by Jun Zhao on Aug. 2, 2017, 5:56 a.m.

Details

Message ID 868f0170-8721-273a-a9d8-c94c5c19323c@gmail.com
State Superseded
Headers show

Commit Message

Jun Zhao Aug. 2, 2017, 5:56 a.m.
From f9b42385faedd64dacf613785c393c7b025237c9 Mon Sep 17 00:00:00 2001
From: Jun Zhao <jun.zhao@intel.com>
Date: Tue, 1 Aug 2017 23:05:44 -0400
Subject: [PATCH V2 3/4] lavc/vaapi_encode_h264: respect "slices" option in
 h264 vaapi encoder

Enable multi-slice support in AVC/H.264 vaapi encoder.

Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
Signed-off-by: Jun Zhao <jun.zhao@intel.com>
---
 libavcodec/vaapi_encode_h264.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

Comments

Mark Thompson Aug. 9, 2017, 10:05 p.m.
On 02/08/17 06:56, Jun Zhao wrote:
> From f9b42385faedd64dacf613785c393c7b025237c9 Mon Sep 17 00:00:00 2001
> From: Jun Zhao <jun.zhao@intel.com>
> Date: Tue, 1 Aug 2017 23:05:44 -0400
> Subject: [PATCH V2 3/4] lavc/vaapi_encode_h264: respect "slices" option in
>  h264 vaapi encoder
> 
> Enable multi-slice support in AVC/H.264 vaapi encoder.
> 
> Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
> ---
>  libavcodec/vaapi_encode_h264.c | 38 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
> index f9fcd805a4..5dad6d10a5 100644
> --- a/libavcodec/vaapi_encode_h264.c
> +++ b/libavcodec/vaapi_encode_h264.c
> @@ -141,6 +141,10 @@ typedef struct VAAPIEncodeH264Context {
>      int mb_width;
>      int mb_height;
>  
> +    int slice_of_mbs;
> +    int slice_mod_mbs;
> +    int last_mb_index;
> +
>      int fixed_qp_idr;
>      int fixed_qp_p;
>      int fixed_qp_b;
> @@ -957,6 +961,7 @@ static int vaapi_encode_h264_init_picture_params(AVCodecContext *avctx,
>      VAEncPictureParameterBufferH264  *vpic = pic->codec_picture_params;
>      VAAPIEncodeH264Context           *priv = ctx->priv_data;
>      int i;
> +    int max_slices;
>  
>      if (pic->type == PICTURE_TYPE_IDR) {
>          av_assert0(pic->display_order == pic->encode_order);
> @@ -1002,7 +1007,22 @@ static int vaapi_encode_h264_init_picture_params(AVCodecContext *avctx,
>      vpic->pic_fields.bits.idr_pic_flag = (pic->type == PICTURE_TYPE_IDR);
>      vpic->pic_fields.bits.reference_pic_flag = (pic->type != PICTURE_TYPE_B);
>  
> -    pic->nb_slices = 1;
> +    max_slices = 1;
> +    if (ctx->max_slices) {
> +        max_slices = FFMIN(priv->mb_height, ctx->max_slices);

Where is this coming from?  You don't enforce anything about rows below.

> +        if (avctx->slices > max_slices) {
> +            av_log(avctx, AV_LOG_WARNING, "The max slices number per frame "
> +                   "cannot more than %d.\n", max_slices);

So if you pass a value which is too high, you get some lower number instead?  It should probably hard fail here - I'm not sure what your use case is for this option, but I imagine it being for conforming to the some specific stream requirements (like bluray or whatever), so just ignoring the option in that case is probably bad.

> +        } else {
> +            max_slices = avctx->slices;
> +        }
> +    }
> +
> +    pic->nb_slices = max_slices;
> +
> +    priv->slice_of_mbs  = (priv->mb_width * priv->mb_height) / pic->nb_slices;
> +    priv->slice_mod_mbs = (priv->mb_width * priv->mb_height) % pic->nb_slices;
> +    priv->last_mb_index = 0;
>  
>      return 0;
>  }
> @@ -1052,14 +1072,18 @@ static int vaapi_encode_h264_init_slice_params(AVCodecContext *avctx,
>          av_assert0(0 && "invalid picture type");
>      }
>  
> -    // Only one slice per frame.
> -    vslice->macroblock_address = 0;
> -    vslice->num_macroblocks = priv->mb_width * priv->mb_height;
> +    vslice->macroblock_address = priv->last_mb_index;
> +    vslice->num_macroblocks = priv->slice_of_mbs + (priv->slice_mod_mbs > 0 ? 1 : 0);
> +    if (priv->slice_mod_mbs > 0)
> +        priv->slice_mod_mbs--;
> +    priv->last_mb_index += vslice->num_macroblocks;

I'm still unconfortable about this method of distributing the macroblocks equally to slices with rounding error at the top - slice boundaries are not invisible at low bitrates.  I don't have anything better to suggest, though.

>  
>      vslice->macroblock_info = VA_INVALID_ID;
>  
>      vslice->pic_parameter_set_id = vpic->pic_parameter_set_id;
> -    vslice->idr_pic_id = priv->idr_pic_count++;
> +    vslice->idr_pic_id = priv->idr_pic_count;
> +    if (priv->last_mb_index == priv->mb_width * priv->mb_height)
> +        priv->idr_pic_count++;
>  
>      vslice->pic_order_cnt_lsb = (pic->display_order - priv->last_idr_frame) &
>          ((1 << (4 + vseq->seq_fields.bits.log2_max_pic_order_cnt_lsb_minus4)) - 1);
> @@ -1157,6 +1181,10 @@ static av_cold int vaapi_encode_h264_configure(AVCodecContext *avctx)
>  #endif
>      }
>  
> +    if (!ctx->max_slices && avctx->slices > 0)
> +        av_log(avctx, AV_LOG_WARNING, "The encode slice option is not "
> +               "supported with this VAAPI version.\n");

It's a driver constraint, not a VAAPI version one.

> +
>      return 0;
>  }
>  
> -- 
> 2.11.0
>
Jun Zhao Aug. 10, 2017, 8:02 a.m.
On 2017/8/10 6:05, Mark Thompson wrote:
> On 02/08/17 06:56, Jun Zhao wrote:
>> From f9b42385faedd64dacf613785c393c7b025237c9 Mon Sep 17 00:00:00 2001
>> From: Jun Zhao <jun.zhao@intel.com>
>> Date: Tue, 1 Aug 2017 23:05:44 -0400
>> Subject: [PATCH V2 3/4] lavc/vaapi_encode_h264: respect "slices" option in
>>  h264 vaapi encoder
>>
>> Enable multi-slice support in AVC/H.264 vaapi encoder.
>>
>> Signed-off-by: Wang, Yi A <yi.a.wang@intel.com>
>> Signed-off-by: Jun Zhao <jun.zhao@intel.com>
>> ---
>>  libavcodec/vaapi_encode_h264.c | 38 +++++++++++++++++++++++++++++++++-----
>>  1 file changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
>> index f9fcd805a4..5dad6d10a5 100644
>> --- a/libavcodec/vaapi_encode_h264.c
>> +++ b/libavcodec/vaapi_encode_h264.c
>> @@ -141,6 +141,10 @@ typedef struct VAAPIEncodeH264Context {
>>      int mb_width;
>>      int mb_height;
>>  
>> +    int slice_of_mbs;
>> +    int slice_mod_mbs;
>> +    int last_mb_index;
>> +
>>      int fixed_qp_idr;
>>      int fixed_qp_p;
>>      int fixed_qp_b;
>> @@ -957,6 +961,7 @@ static int vaapi_encode_h264_init_picture_params(AVCodecContext *avctx,
>>      VAEncPictureParameterBufferH264  *vpic = pic->codec_picture_params;
>>      VAAPIEncodeH264Context           *priv = ctx->priv_data;
>>      int i;
>> +    int max_slices;
>>  
>>      if (pic->type == PICTURE_TYPE_IDR) {
>>          av_assert0(pic->display_order == pic->encode_order);
>> @@ -1002,7 +1007,22 @@ static int vaapi_encode_h264_init_picture_params(AVCodecContext *avctx,
>>      vpic->pic_fields.bits.idr_pic_flag = (pic->type == PICTURE_TYPE_IDR);
>>      vpic->pic_fields.bits.reference_pic_flag = (pic->type != PICTURE_TYPE_B);
>>  
>> -    pic->nb_slices = 1;
>> +    max_slices = 1;
>> +    if (ctx->max_slices) {
>> +        max_slices = FFMIN(priv->mb_height, ctx->max_slices);
> 
> Where is this coming from?  You don't enforce anything about rows below.

I use mb_height as the upper limit about MAX(slices) , but maybe I am wrong,
in va.h, I find the flag VA_ENC_SLICE_STRUCTURE_ARBITRARY_MACROBLOCKS with comment
"Driver supports an arbitrary number of rows(I guess rows is wrong 
comment in this, I think the right word is microblocks) per slice".

So I will drop this check in V3 patch. Tks.

> 
>> +        if (avctx->slices > max_slices) {
>> +            av_log(avctx, AV_LOG_WARNING, "The max slices number per frame "
>> +                   "cannot more than %d.\n", max_slices);
> 
> So if you pass a value which is too high, you get some lower number instead?  It should probably hard fail here - I'm not sure what your use case is for this option, but I imagine it being for conforming to the some specific stream requirements (like bluray or whatever), so just ignoring the option in that case is probably bad.

Agree.

> 
>> +        } else {
>> +            max_slices = avctx->slices;
>> +        }
>> +    }
>> +
>> +    pic->nb_slices = max_slices;
>> +
>> +    priv->slice_of_mbs  = (priv->mb_width * priv->mb_height) / pic->nb_slices;
>> +    priv->slice_mod_mbs = (priv->mb_width * priv->mb_height) % pic->nb_slices;
>> +    priv->last_mb_index = 0;
>>  
>>      return 0;
>>  }
>> @@ -1052,14 +1072,18 @@ static int vaapi_encode_h264_init_slice_params(AVCodecContext *avctx,
>>          av_assert0(0 && "invalid picture type");
>>      }
>>  
>> -    // Only one slice per frame.
>> -    vslice->macroblock_address = 0;
>> -    vslice->num_macroblocks = priv->mb_width * priv->mb_height;
>> +    vslice->macroblock_address = priv->last_mb_index;
>> +    vslice->num_macroblocks = priv->slice_of_mbs + (priv->slice_mod_mbs > 0 ? 1 : 0);
>> +    if (priv->slice_mod_mbs > 0)
>> +        priv->slice_mod_mbs--;
>> +    priv->last_mb_index += vslice->num_macroblocks;
> 
> I'm still unconfortable about this method of distributing the macroblocks equally to slices with rounding error at the top - slice boundaries are not invisible at low bitrates.  I don't have anything better to suggest, though.

I don't have any idea to distribute MBs to slice, maybe we can follow x264 style to forces rectangular slices ? I don't know.

> 
>>  
>>      vslice->macroblock_info = VA_INVALID_ID;
>>  
>>      vslice->pic_parameter_set_id = vpic->pic_parameter_set_id;
>> -    vslice->idr_pic_id = priv->idr_pic_count++;
>> +    vslice->idr_pic_id = priv->idr_pic_count;
>> +    if (priv->last_mb_index == priv->mb_width * priv->mb_height)
>> +        priv->idr_pic_count++;
>>  
>>      vslice->pic_order_cnt_lsb = (pic->display_order - priv->last_idr_frame) &
>>          ((1 << (4 + vseq->seq_fields.bits.log2_max_pic_order_cnt_lsb_minus4)) - 1);
>> @@ -1157,6 +1181,10 @@ static av_cold int vaapi_encode_h264_configure(AVCodecContext *avctx)
>>  #endif
>>      }
>>  
>> +    if (!ctx->max_slices && avctx->slices > 0)
>> +        av_log(avctx, AV_LOG_WARNING, "The encode slice option is not "
>> +               "supported with this VAAPI version.\n");
> 
> It's a driver constraint, not a VAAPI version one.

Will change the warning message.
> 
>> +
>>      return 0;
>>  }
>>  
>> -- 
>> 2.11.0
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
index f9fcd805a4..5dad6d10a5 100644
--- a/libavcodec/vaapi_encode_h264.c
+++ b/libavcodec/vaapi_encode_h264.c
@@ -141,6 +141,10 @@  typedef struct VAAPIEncodeH264Context {
     int mb_width;
     int mb_height;
 
+    int slice_of_mbs;
+    int slice_mod_mbs;
+    int last_mb_index;
+
     int fixed_qp_idr;
     int fixed_qp_p;
     int fixed_qp_b;
@@ -957,6 +961,7 @@  static int vaapi_encode_h264_init_picture_params(AVCodecContext *avctx,
     VAEncPictureParameterBufferH264  *vpic = pic->codec_picture_params;
     VAAPIEncodeH264Context           *priv = ctx->priv_data;
     int i;
+    int max_slices;
 
     if (pic->type == PICTURE_TYPE_IDR) {
         av_assert0(pic->display_order == pic->encode_order);
@@ -1002,7 +1007,22 @@  static int vaapi_encode_h264_init_picture_params(AVCodecContext *avctx,
     vpic->pic_fields.bits.idr_pic_flag = (pic->type == PICTURE_TYPE_IDR);
     vpic->pic_fields.bits.reference_pic_flag = (pic->type != PICTURE_TYPE_B);
 
-    pic->nb_slices = 1;
+    max_slices = 1;
+    if (ctx->max_slices) {
+        max_slices = FFMIN(priv->mb_height, ctx->max_slices);
+        if (avctx->slices > max_slices) {
+            av_log(avctx, AV_LOG_WARNING, "The max slices number per frame "
+                   "cannot more than %d.\n", max_slices);
+        } else {
+            max_slices = avctx->slices;
+        }
+    }
+
+    pic->nb_slices = max_slices;
+
+    priv->slice_of_mbs  = (priv->mb_width * priv->mb_height) / pic->nb_slices;
+    priv->slice_mod_mbs = (priv->mb_width * priv->mb_height) % pic->nb_slices;
+    priv->last_mb_index = 0;
 
     return 0;
 }
@@ -1052,14 +1072,18 @@  static int vaapi_encode_h264_init_slice_params(AVCodecContext *avctx,
         av_assert0(0 && "invalid picture type");
     }
 
-    // Only one slice per frame.
-    vslice->macroblock_address = 0;
-    vslice->num_macroblocks = priv->mb_width * priv->mb_height;
+    vslice->macroblock_address = priv->last_mb_index;
+    vslice->num_macroblocks = priv->slice_of_mbs + (priv->slice_mod_mbs > 0 ? 1 : 0);
+    if (priv->slice_mod_mbs > 0)
+        priv->slice_mod_mbs--;
+    priv->last_mb_index += vslice->num_macroblocks;
 
     vslice->macroblock_info = VA_INVALID_ID;
 
     vslice->pic_parameter_set_id = vpic->pic_parameter_set_id;
-    vslice->idr_pic_id = priv->idr_pic_count++;
+    vslice->idr_pic_id = priv->idr_pic_count;
+    if (priv->last_mb_index == priv->mb_width * priv->mb_height)
+        priv->idr_pic_count++;
 
     vslice->pic_order_cnt_lsb = (pic->display_order - priv->last_idr_frame) &
         ((1 << (4 + vseq->seq_fields.bits.log2_max_pic_order_cnt_lsb_minus4)) - 1);
@@ -1157,6 +1181,10 @@  static av_cold int vaapi_encode_h264_configure(AVCodecContext *avctx)
 #endif
     }
 
+    if (!ctx->max_slices && avctx->slices > 0)
+        av_log(avctx, AV_LOG_WARNING, "The encode slice option is not "
+               "supported with this VAAPI version.\n");
+
     return 0;
 }