diff mbox series

[FFmpeg-devel,3/6] vaapi_encode_h265: Don't require uniform_spacing_flag

Message ID 20200728225025.1830283-3-sw@jkqxz.net
State Accepted
Headers show
Series [FFmpeg-devel,1/6] vaapi_encode_h265: Remove confusing and redundant tile options
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Mark Thompson July 28, 2020, 10:50 p.m. UTC
Though still use it if the tile arrangement matches.  Also try to keep the
fields in the same order as the standard.
---
 libavcodec/vaapi_encode_h265.c | 39 ++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 11 deletions(-)

Comments

Xiang, Haihao July 29, 2020, 3:13 a.m. UTC | #1
On Tue, 2020-07-28 at 23:50 +0100, Mark Thompson wrote:
> Though still use it if the tile arrangement matches.  Also try to keep the
> fields in the same order as the standard.
> ---
>  libavcodec/vaapi_encode_h265.c | 39 ++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c
> index 04bd2bef1d..a7230a676c 100644
> --- a/libavcodec/vaapi_encode_h265.c
> +++ b/libavcodec/vaapi_encode_h265.c
> @@ -555,23 +555,40 @@ static int
> vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
>      pps->cu_qp_delta_enabled_flag = (ctx->va_rc_mode != VA_RC_CQP);
>      pps->diff_cu_qp_delta_depth   = 0;
>  
> -    pps->pps_loop_filter_across_slices_enabled_flag = 1;
> -
>      if (ctx->tile_rows && ctx->tile_cols) {
> -        pps->tiles_enabled_flag         = 1;
> -        pps->uniform_spacing_flag       = 1;
> -
> -        pps->num_tile_rows_minus1       = ctx->tile_rows - 1;
> -        pps->num_tile_columns_minus1    = ctx->tile_cols - 1;
> -
> -        pps->loop_filter_across_tiles_enabled_flag = 1;
> +        int uniform_spacing;
> +
> +        pps->tiles_enabled_flag      = 1;
> +        pps->num_tile_columns_minus1 = ctx->tile_cols - 1;
> +        pps->num_tile_rows_minus1    = ctx->tile_rows - 1;
> +
> +        // Test whether the spacing provided matches the H.265 uniform
> +        // spacing, and set the flag if it does.
> +        uniform_spacing = 1;
> +        for (i = 0; i <= pps->num_tile_columns_minus1; i++) {

Better to use 'i <= pps->num_tile_columns_minus1 && uniform_spacing' to allow
early-exiting the loop if uniform_spacing has been 0


> +            if (ctx->col_width[i] !=
> +                (i + 1) * ctx->slice_block_cols / ctx->tile_cols -
> +                 i      * ctx->slice_block_cols / ctx->tile_cols)
> +                uniform_spacing = 0;
> +        }
> +        for (i = 0; i <= pps->num_tile_rows_minus1; i++) {

Same as above. 

> +            if (ctx->row_height[i] !=
> +                (i + 1) * ctx->slice_block_rows / ctx->tile_rows -
> +                 i      * ctx->slice_block_rows / ctx->tile_rows)
> +                uniform_spacing = 0;
> +        }
> +        pps->uniform_spacing_flag = uniform_spacing;
>  
> -        for (i = 0; i <= pps->num_tile_rows_minus1; i++)
> -            pps->row_height_minus1[i]   = ctx->row_height[i] - 1;
>          for (i = 0; i <= pps->num_tile_columns_minus1; i++)
>              pps->column_width_minus1[i] = ctx->col_width[i] - 1;
> +        for (i = 0; i <= pps->num_tile_rows_minus1; i++)
> +            pps->row_height_minus1[i]   = ctx->row_height[i] - 1;
> +
> +        pps->loop_filter_across_tiles_enabled_flag = 1;
>      }
>  
> +    pps->pps_loop_filter_across_slices_enabled_flag = 1;
> +
>      // Fill VAAPI parameter buffers.
>  
>      *vseq = (VAEncSequenceParameterBufferHEVC) {
Mark Thompson Aug. 1, 2020, 4:41 p.m. UTC | #2
On 29/07/2020 04:13, Xiang, Haihao wrote:
> On Tue, 2020-07-28 at 23:50 +0100, Mark Thompson wrote:
>> Though still use it if the tile arrangement matches.  Also try to keep the
>> fields in the same order as the standard.
>> ---
>>   libavcodec/vaapi_encode_h265.c | 39 ++++++++++++++++++++++++----------
>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c
>> index 04bd2bef1d..a7230a676c 100644
>> --- a/libavcodec/vaapi_encode_h265.c
>> +++ b/libavcodec/vaapi_encode_h265.c
>> @@ -555,23 +555,40 @@ static int
>> vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
>>       pps->cu_qp_delta_enabled_flag = (ctx->va_rc_mode != VA_RC_CQP);
>>       pps->diff_cu_qp_delta_depth   = 0;
>>   
>> -    pps->pps_loop_filter_across_slices_enabled_flag = 1;
>> -
>>       if (ctx->tile_rows && ctx->tile_cols) {
>> -        pps->tiles_enabled_flag         = 1;
>> -        pps->uniform_spacing_flag       = 1;
>> -
>> -        pps->num_tile_rows_minus1       = ctx->tile_rows - 1;
>> -        pps->num_tile_columns_minus1    = ctx->tile_cols - 1;
>> -
>> -        pps->loop_filter_across_tiles_enabled_flag = 1;
>> +        int uniform_spacing;
>> +
>> +        pps->tiles_enabled_flag      = 1;
>> +        pps->num_tile_columns_minus1 = ctx->tile_cols - 1;
>> +        pps->num_tile_rows_minus1    = ctx->tile_rows - 1;
>> +
>> +        // Test whether the spacing provided matches the H.265 uniform
>> +        // spacing, and set the flag if it does.
>> +        uniform_spacing = 1;
>> +        for (i = 0; i <= pps->num_tile_columns_minus1; i++) {
> 
> Better to use 'i <= pps->num_tile_columns_minus1 && uniform_spacing' to allow
> early-exiting the loop if uniform_spacing has been 0

I weakly think that a minor optimisation like this in a cold path doesn't seem like a particularly good tradeoff against having the code cleanly match the standard.

But whatever, changed as you suggest.

>> +            if (ctx->col_width[i] !=
>> +                (i + 1) * ctx->slice_block_cols / ctx->tile_cols -
>> +                 i      * ctx->slice_block_cols / ctx->tile_cols)
>> +                uniform_spacing = 0;
>> +        }
>> +        for (i = 0; i <= pps->num_tile_rows_minus1; i++) {
> 
> Same as above.
> 
>> +            if (ctx->row_height[i] !=
>> +                (i + 1) * ctx->slice_block_rows / ctx->tile_rows -
>> +                 i      * ctx->slice_block_rows / ctx->tile_rows)
>> +                uniform_spacing = 0;
>> +        }
>> +        pps->uniform_spacing_flag = uniform_spacing;
>>   
>> -        for (i = 0; i <= pps->num_tile_rows_minus1; i++)
>> -            pps->row_height_minus1[i]   = ctx->row_height[i] - 1;
>>           for (i = 0; i <= pps->num_tile_columns_minus1; i++)
>>               pps->column_width_minus1[i] = ctx->col_width[i] - 1;
>> +        for (i = 0; i <= pps->num_tile_rows_minus1; i++)
>> +            pps->row_height_minus1[i]   = ctx->row_height[i] - 1;
>> +
>> +        pps->loop_filter_across_tiles_enabled_flag = 1;
>>       }
>>   
>> +    pps->pps_loop_filter_across_slices_enabled_flag = 1;
>> +
>>       // Fill VAAPI parameter buffers.
>>   

Applied with that.

Thanks,

- Mark
diff mbox series

Patch

diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c
index 04bd2bef1d..a7230a676c 100644
--- a/libavcodec/vaapi_encode_h265.c
+++ b/libavcodec/vaapi_encode_h265.c
@@ -555,23 +555,40 @@  static int vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
     pps->cu_qp_delta_enabled_flag = (ctx->va_rc_mode != VA_RC_CQP);
     pps->diff_cu_qp_delta_depth   = 0;
 
-    pps->pps_loop_filter_across_slices_enabled_flag = 1;
-
     if (ctx->tile_rows && ctx->tile_cols) {
-        pps->tiles_enabled_flag         = 1;
-        pps->uniform_spacing_flag       = 1;
-
-        pps->num_tile_rows_minus1       = ctx->tile_rows - 1;
-        pps->num_tile_columns_minus1    = ctx->tile_cols - 1;
-
-        pps->loop_filter_across_tiles_enabled_flag = 1;
+        int uniform_spacing;
+
+        pps->tiles_enabled_flag      = 1;
+        pps->num_tile_columns_minus1 = ctx->tile_cols - 1;
+        pps->num_tile_rows_minus1    = ctx->tile_rows - 1;
+
+        // Test whether the spacing provided matches the H.265 uniform
+        // spacing, and set the flag if it does.
+        uniform_spacing = 1;
+        for (i = 0; i <= pps->num_tile_columns_minus1; i++) {
+            if (ctx->col_width[i] !=
+                (i + 1) * ctx->slice_block_cols / ctx->tile_cols -
+                 i      * ctx->slice_block_cols / ctx->tile_cols)
+                uniform_spacing = 0;
+        }
+        for (i = 0; i <= pps->num_tile_rows_minus1; i++) {
+            if (ctx->row_height[i] !=
+                (i + 1) * ctx->slice_block_rows / ctx->tile_rows -
+                 i      * ctx->slice_block_rows / ctx->tile_rows)
+                uniform_spacing = 0;
+        }
+        pps->uniform_spacing_flag = uniform_spacing;
 
-        for (i = 0; i <= pps->num_tile_rows_minus1; i++)
-            pps->row_height_minus1[i]   = ctx->row_height[i] - 1;
         for (i = 0; i <= pps->num_tile_columns_minus1; i++)
             pps->column_width_minus1[i] = ctx->col_width[i] - 1;
+        for (i = 0; i <= pps->num_tile_rows_minus1; i++)
+            pps->row_height_minus1[i]   = ctx->row_height[i] - 1;
+
+        pps->loop_filter_across_tiles_enabled_flag = 1;
     }
 
+    pps->pps_loop_filter_across_slices_enabled_flag = 1;
+
     // Fill VAAPI parameter buffers.
 
     *vseq = (VAEncSequenceParameterBufferHEVC) {