[FFmpeg-devel] vaapi_encode: Refactor encode misc parameter buffer creation

Submitted by Mark Thompson on May 6, 2019, 3:21 p.m.

Details

Message ID 16133da8-ce0a-10da-c32c-dc43fdc4e10c@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson May 6, 2019, 3:21 p.m.
This removes the use of the nonstandard combined structures, which
generated some warnings with clang and will cause alignment problems
with some parameter buffer types.
---
On 27/03/2019 14:18, Carl Eugen Hoyos wrote:
> Attached patch fixes many warnings when compiling vaapi with clang.
> Also tested with clang-3.4.
> ...

How about this approach instead?  I think something like it is going to be needed anyway because of alignment problems with parameter structures which aren't yet used.

- Mark


 libavcodec/vaapi_encode.c      | 71 ++++++++++++++++++++++++----------
 libavcodec/vaapi_encode.h      | 23 +++--------
 libavcodec/vaapi_encode_h264.c |  8 ++--
 3 files changed, 60 insertions(+), 42 deletions(-)

Comments

Linjie Fu May 9, 2019, 2:46 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf

> Of Mark Thompson

> Sent: Monday, May 6, 2019 23:21

> To: FFmpeg development discussions and patches <ffmpeg-

> devel@ffmpeg.org>

> Subject: [FFmpeg-devel] [PATCH] vaapi_encode: Refactor encode misc

> parameter buffer creation

> 

> This removes the use of the nonstandard combined structures, which

> generated some warnings with clang and will cause alignment problems

> with some parameter buffer types.

> ---

> On 27/03/2019 14:18, Carl Eugen Hoyos wrote:

> > Attached patch fixes many warnings when compiling vaapi with clang.

> > Also tested with clang-3.4.

> > ...

> 

> How about this approach instead?  I think something like it is going to be

> needed anyway because of alignment problems with parameter structures

> which aren't yet used.

> 

> - Mark

> 

> 

>  libavcodec/vaapi_encode.c      | 71 ++++++++++++++++++++++++----------

>  libavcodec/vaapi_encode.h      | 23 +++--------

>  libavcodec/vaapi_encode_h264.c |  8 ++--

>  3 files changed, 60 insertions(+), 42 deletions(-)

> 

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

> index 2dda451882..95031187df 100644

> --- a/libavcodec/vaapi_encode.c

> +++ b/libavcodec/vaapi_encode.c

> @@ -103,6 +103,29 @@ static int

> vaapi_encode_make_param_buffer(AVCodecContext *avctx,

>      return 0;

>  }

> 

> +static int vaapi_encode_make_misc_param_buffer(AVCodecContext

> *avctx,

> +                                               VAAPIEncodePicture *pic,

> +                                               int type,

> +                                               const void *data, size_t len)

> +{

> +    // Construct the buffer on the stack - 1KB is much larger than any

> +    // current misc parameter buffer type (the largest is EncQuality at

> +    // 224 bytes).

> +    uint8_t buffer[1024];

> +    VAEncMiscParameterBuffer header = {

> +        .type = type,

> +    };

> +    size_t buffer_size = sizeof(header) + len;

> +    av_assert0(buffer_size <= sizeof(buffer));

> +

> +    memcpy(buffer, &header, sizeof(header));

> +    memcpy(buffer + sizeof(header), data, len);

> +

> +    return vaapi_encode_make_param_buffer(avctx, pic,

> +                                          VAEncMiscParameterBufferType,

> +                                          buffer, buffer_size);

> +}

> +

>  static int vaapi_encode_wait(AVCodecContext *avctx,

>                               VAAPIEncodePicture *pic)

>  {

> @@ -212,10 +235,10 @@ static int vaapi_encode_issue(AVCodecContext

> *avctx,

> 

>      if (pic->type == PICTURE_TYPE_IDR) {

>          for (i = 0; i < ctx->nb_global_params; i++) {

> -            err = vaapi_encode_make_param_buffer(avctx, pic,

> -                                                 VAEncMiscParameterBufferType,

> -                                                 (char*)ctx->global_params[i],

> -                                                 ctx->global_params_size[i]);

> +            err = vaapi_encode_make_misc_param_buffer(avctx, pic,

> +                                                      ctx->global_params_type[i],

> +                                                      ctx->global_params[i],

> +                                                      ctx->global_params_size[i]);

>              if (err < 0)

>                  goto fail;

>          }

> @@ -1034,14 +1057,14 @@ int ff_vaapi_encode2(AVCodecContext *avctx,

> AVPacket *pkt,

>      return AVERROR(ENOSYS);

>  }

> 

> -static av_cold void vaapi_encode_add_global_param(AVCodecContext

> *avctx,

> -                                                  VAEncMiscParameterBuffer *buffer,

> -                                                  size_t size)

> +static av_cold void vaapi_encode_add_global_param(AVCodecContext

> *avctx, int type,

> +                                                  void *buffer, size_t size)

>  {

>      VAAPIEncodeContext *ctx = avctx->priv_data;

> 

>      av_assert0(ctx->nb_global_params < MAX_GLOBAL_PARAMS);

> 

> +    ctx->global_params_type[ctx->nb_global_params] = type;

>      ctx->global_params     [ctx->nb_global_params] = buffer;

>      ctx->global_params_size[ctx->nb_global_params] = size;

> 

> @@ -1575,8 +1598,7 @@ rc_mode_found:

>                     rc_bits_per_second, rc_window_size);

>          }

> 

> -        ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;

> -        ctx->rc_params.rc = (VAEncMiscParameterRateControl) {

> +        ctx->rc_params = (VAEncMiscParameterRateControl) {

>              .bits_per_second    = rc_bits_per_second,

>              .target_percentage  = rc_target_percentage,

>              .window_size        = rc_window_size,

> @@ -1591,7 +1613,9 @@ rc_mode_found:

>              .quality_factor     = rc_quality,

>  #endif

>          };

> -        vaapi_encode_add_global_param(avctx, &ctx->rc_params.misc,

> +        vaapi_encode_add_global_param(avctx,

> +                                      VAEncMiscParameterTypeRateControl,

> +                                      &ctx->rc_params,

>                                        sizeof(ctx->rc_params));

>      }

> 

> @@ -1600,12 +1624,13 @@ rc_mode_found:

>                 "initial fullness %"PRId64" bits.\n",

>                 hrd_buffer_size, hrd_initial_buffer_fullness);

> 

> -        ctx->hrd_params.misc.type = VAEncMiscParameterTypeHRD;

> -        ctx->hrd_params.hrd = (VAEncMiscParameterHRD) {

> +        ctx->hrd_params = (VAEncMiscParameterHRD) {

>              .initial_buffer_fullness = hrd_initial_buffer_fullness,

>              .buffer_size             = hrd_buffer_size,

>          };

> -        vaapi_encode_add_global_param(avctx, &ctx->hrd_params.misc,

> +        vaapi_encode_add_global_param(avctx,

> +                                      VAEncMiscParameterTypeHRD,

> +                                      &ctx->hrd_params,

>                                        sizeof(ctx->hrd_params));

>      }

> 

> @@ -1619,11 +1644,13 @@ rc_mode_found:

>      av_log(avctx, AV_LOG_VERBOSE, "RC framerate: %d/%d (%.2f fps).\n",

>             fr_num, fr_den, (double)fr_num / fr_den);

> 

> -    ctx->fr_params.misc.type = VAEncMiscParameterTypeFrameRate;

> -    ctx->fr_params.fr.framerate = (unsigned int)fr_den << 16 | fr_num;

> -

> +    ctx->fr_params = (VAEncMiscParameterFrameRate) {

> +        .framerate = (unsigned int)fr_den << 16 | fr_num,

> +    };

>  #if VA_CHECK_VERSION(0, 40, 0)

> -    vaapi_encode_add_global_param(avctx, &ctx->fr_params.misc,

> +    vaapi_encode_add_global_param(avctx,

> +                                  VAEncMiscParameterTypeFrameRate,

> +                                  &ctx->fr_params,

>                                    sizeof(ctx->fr_params));

>  #endif

> 

> @@ -1885,10 +1912,12 @@ static av_cold int

> vaapi_encode_init_quality(AVCodecContext *avctx)

>              quality = attr.value;

>          }

> 

> -        ctx->quality_params.misc.type = VAEncMiscParameterTypeQualityLevel;

> -        ctx->quality_params.quality.quality_level = quality;

> -

> -        vaapi_encode_add_global_param(avctx, &ctx->quality_params.misc,

> +        ctx->quality_params = (VAEncMiscParameterBufferQualityLevel) {

> +            .quality_level = quality,

> +        };

> +        vaapi_encode_add_global_param(avctx,

> +                                      VAEncMiscParameterTypeQualityLevel,

> +                                      &ctx->quality_params,

>                                        sizeof(ctx->quality_params));

>      }

>  #else

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

> index 44a8db566e..80f6c680b4 100644

> --- a/libavcodec/vaapi_encode.h

> +++ b/libavcodec/vaapi_encode.h

> @@ -244,28 +244,17 @@ typedef struct VAAPIEncodeContext {

> 

>      // Global parameters which will be applied at the start of the

>      // sequence (includes rate control parameters below).

> -    VAEncMiscParameterBuffer *global_params[MAX_GLOBAL_PARAMS];

> +    int             global_params_type[MAX_GLOBAL_PARAMS];

> +    const void     *global_params     [MAX_GLOBAL_PARAMS];

>      size_t          global_params_size[MAX_GLOBAL_PARAMS];

>      int          nb_global_params;

> 

>      // Rate control parameters.

> -    struct {

> -        VAEncMiscParameterBuffer misc;

> -        VAEncMiscParameterRateControl rc;

> -    } rc_params;

> -    struct {

> -        VAEncMiscParameterBuffer misc;

> -        VAEncMiscParameterHRD hrd;

> -    } hrd_params;

> -    struct {

> -        VAEncMiscParameterBuffer misc;

> -        VAEncMiscParameterFrameRate fr;

> -    } fr_params;

> +    VAEncMiscParameterRateControl rc_params;

> +    VAEncMiscParameterHRD        hrd_params;

> +    VAEncMiscParameterFrameRate   fr_params;

>  #if VA_CHECK_VERSION(0, 36, 0)

> -    struct {

> -        VAEncMiscParameterBuffer misc;

> -        VAEncMiscParameterBufferQualityLevel quality;

> -    } quality_params;

> +    VAEncMiscParameterBufferQualityLevel quality_params;

>  #endif

> 

>      // Per-sequence parameter structure (VAEncSequenceParameterBuffer*).

> diff --git a/libavcodec/vaapi_encode_h264.c

> b/libavcodec/vaapi_encode_h264.c

> index 4cf99d7c78..d1427112ea 100644

> --- a/libavcodec/vaapi_encode_h264.c

> +++ b/libavcodec/vaapi_encode_h264.c

> @@ -471,9 +471,9 @@ static int

> vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)

>              (ctx->va_bit_rate >> hrd->bit_rate_scale + 6) - 1;

> 

>          hrd->cpb_size_scale =

> -            av_clip_uintp2(av_log2(ctx->hrd_params.hrd.buffer_size) - 15 - 4, 4);

> +            av_clip_uintp2(av_log2(ctx->hrd_params.buffer_size) - 15 - 4, 4);

>          hrd->cpb_size_value_minus1[0] =

> -            (ctx->hrd_params.hrd.buffer_size >> hrd->cpb_size_scale + 4) - 1;

> +            (ctx->hrd_params.buffer_size >> hrd->cpb_size_scale + 4) - 1;

> 

>          // CBR mode as defined for the HRD cannot be achieved without filler

>          // data, so this flag cannot be set even with VAAPI CBR modes.

> @@ -488,8 +488,8 @@ static int

> vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)

> 

>          // This calculation can easily overflow 32 bits.

>          bp->nal.initial_cpb_removal_delay[0] = 90000 *

> -            (uint64_t)ctx->hrd_params.hrd.initial_buffer_fullness /

> -            ctx->hrd_params.hrd.buffer_size;

> +            (uint64_t)ctx->hrd_params.initial_buffer_fullness /

> +            ctx->hrd_params.buffer_size;

>          bp->nal.initial_cpb_removal_delay_offset[0] = 0;

>      } else {

>          sps->vui.nal_hrd_parameters_present_flag = 0;

> --

> 2.20.1


LGTM, for separating Misc parameter structure(they are separated in MSDK too)
This is more robust for the features with alignment issues.(ROI, trellis, max frame size as far as I know)

Thanks.
Mark Thompson June 3, 2019, 8:19 p.m.
On 09/05/2019 03:46, Fu, Linjie wrote:
>> -----Original Message-----
>> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf
>> Of Mark Thompson
>> Sent: Monday, May 6, 2019 23:21
>> To: FFmpeg development discussions and patches <ffmpeg-
>> devel@ffmpeg.org>
>> Subject: [FFmpeg-devel] [PATCH] vaapi_encode: Refactor encode misc
>> parameter buffer creation
>>
>> This removes the use of the nonstandard combined structures, which
>> generated some warnings with clang and will cause alignment problems
>> with some parameter buffer types.
>> ---
>> On 27/03/2019 14:18, Carl Eugen Hoyos wrote:
>>> Attached patch fixes many warnings when compiling vaapi with clang.
>>> Also tested with clang-3.4.
>>> ...
>>
>> How about this approach instead?  I think something like it is going to be
>> needed anyway because of alignment problems with parameter structures
>> which aren't yet used.
>>
>> - Mark
>>
>>
>>  libavcodec/vaapi_encode.c      | 71 ++++++++++++++++++++++++----------
>>  libavcodec/vaapi_encode.h      | 23 +++--------
>>  libavcodec/vaapi_encode_h264.c |  8 ++--
>>  3 files changed, 60 insertions(+), 42 deletions(-)
> 
> LGTM, for separating Misc parameter structure(they are separated in MSDK too)
> This is more robust for the features with alignment issues.(ROI, trellis, max frame size as far as I know)

Applied.

Thanks!

- Mark

Patch hide | download patch | download mbox

diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index 2dda451882..95031187df 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -103,6 +103,29 @@  static int vaapi_encode_make_param_buffer(AVCodecContext *avctx,
     return 0;
 }
 
+static int vaapi_encode_make_misc_param_buffer(AVCodecContext *avctx,
+                                               VAAPIEncodePicture *pic,
+                                               int type,
+                                               const void *data, size_t len)
+{
+    // Construct the buffer on the stack - 1KB is much larger than any
+    // current misc parameter buffer type (the largest is EncQuality at
+    // 224 bytes).
+    uint8_t buffer[1024];
+    VAEncMiscParameterBuffer header = {
+        .type = type,
+    };
+    size_t buffer_size = sizeof(header) + len;
+    av_assert0(buffer_size <= sizeof(buffer));
+
+    memcpy(buffer, &header, sizeof(header));
+    memcpy(buffer + sizeof(header), data, len);
+
+    return vaapi_encode_make_param_buffer(avctx, pic,
+                                          VAEncMiscParameterBufferType,
+                                          buffer, buffer_size);
+}
+
 static int vaapi_encode_wait(AVCodecContext *avctx,
                              VAAPIEncodePicture *pic)
 {
@@ -212,10 +235,10 @@  static int vaapi_encode_issue(AVCodecContext *avctx,
 
     if (pic->type == PICTURE_TYPE_IDR) {
         for (i = 0; i < ctx->nb_global_params; i++) {
-            err = vaapi_encode_make_param_buffer(avctx, pic,
-                                                 VAEncMiscParameterBufferType,
-                                                 (char*)ctx->global_params[i],
-                                                 ctx->global_params_size[i]);
+            err = vaapi_encode_make_misc_param_buffer(avctx, pic,
+                                                      ctx->global_params_type[i],
+                                                      ctx->global_params[i],
+                                                      ctx->global_params_size[i]);
             if (err < 0)
                 goto fail;
         }
@@ -1034,14 +1057,14 @@  int ff_vaapi_encode2(AVCodecContext *avctx, AVPacket *pkt,
     return AVERROR(ENOSYS);
 }
 
-static av_cold void vaapi_encode_add_global_param(AVCodecContext *avctx,
-                                                  VAEncMiscParameterBuffer *buffer,
-                                                  size_t size)
+static av_cold void vaapi_encode_add_global_param(AVCodecContext *avctx, int type,
+                                                  void *buffer, size_t size)
 {
     VAAPIEncodeContext *ctx = avctx->priv_data;
 
     av_assert0(ctx->nb_global_params < MAX_GLOBAL_PARAMS);
 
+    ctx->global_params_type[ctx->nb_global_params] = type;
     ctx->global_params     [ctx->nb_global_params] = buffer;
     ctx->global_params_size[ctx->nb_global_params] = size;
 
@@ -1575,8 +1598,7 @@  rc_mode_found:
                    rc_bits_per_second, rc_window_size);
         }
 
-        ctx->rc_params.misc.type = VAEncMiscParameterTypeRateControl;
-        ctx->rc_params.rc = (VAEncMiscParameterRateControl) {
+        ctx->rc_params = (VAEncMiscParameterRateControl) {
             .bits_per_second    = rc_bits_per_second,
             .target_percentage  = rc_target_percentage,
             .window_size        = rc_window_size,
@@ -1591,7 +1613,9 @@  rc_mode_found:
             .quality_factor     = rc_quality,
 #endif
         };
-        vaapi_encode_add_global_param(avctx, &ctx->rc_params.misc,
+        vaapi_encode_add_global_param(avctx,
+                                      VAEncMiscParameterTypeRateControl,
+                                      &ctx->rc_params,
                                       sizeof(ctx->rc_params));
     }
 
@@ -1600,12 +1624,13 @@  rc_mode_found:
                "initial fullness %"PRId64" bits.\n",
                hrd_buffer_size, hrd_initial_buffer_fullness);
 
-        ctx->hrd_params.misc.type = VAEncMiscParameterTypeHRD;
-        ctx->hrd_params.hrd = (VAEncMiscParameterHRD) {
+        ctx->hrd_params = (VAEncMiscParameterHRD) {
             .initial_buffer_fullness = hrd_initial_buffer_fullness,
             .buffer_size             = hrd_buffer_size,
         };
-        vaapi_encode_add_global_param(avctx, &ctx->hrd_params.misc,
+        vaapi_encode_add_global_param(avctx,
+                                      VAEncMiscParameterTypeHRD,
+                                      &ctx->hrd_params,
                                       sizeof(ctx->hrd_params));
     }
 
@@ -1619,11 +1644,13 @@  rc_mode_found:
     av_log(avctx, AV_LOG_VERBOSE, "RC framerate: %d/%d (%.2f fps).\n",
            fr_num, fr_den, (double)fr_num / fr_den);
 
-    ctx->fr_params.misc.type = VAEncMiscParameterTypeFrameRate;
-    ctx->fr_params.fr.framerate = (unsigned int)fr_den << 16 | fr_num;
-
+    ctx->fr_params = (VAEncMiscParameterFrameRate) {
+        .framerate = (unsigned int)fr_den << 16 | fr_num,
+    };
 #if VA_CHECK_VERSION(0, 40, 0)
-    vaapi_encode_add_global_param(avctx, &ctx->fr_params.misc,
+    vaapi_encode_add_global_param(avctx,
+                                  VAEncMiscParameterTypeFrameRate,
+                                  &ctx->fr_params,
                                   sizeof(ctx->fr_params));
 #endif
 
@@ -1885,10 +1912,12 @@  static av_cold int vaapi_encode_init_quality(AVCodecContext *avctx)
             quality = attr.value;
         }
 
-        ctx->quality_params.misc.type = VAEncMiscParameterTypeQualityLevel;
-        ctx->quality_params.quality.quality_level = quality;
-
-        vaapi_encode_add_global_param(avctx, &ctx->quality_params.misc,
+        ctx->quality_params = (VAEncMiscParameterBufferQualityLevel) {
+            .quality_level = quality,
+        };
+        vaapi_encode_add_global_param(avctx,
+                                      VAEncMiscParameterTypeQualityLevel,
+                                      &ctx->quality_params,
                                       sizeof(ctx->quality_params));
     }
 #else
diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h
index 44a8db566e..80f6c680b4 100644
--- a/libavcodec/vaapi_encode.h
+++ b/libavcodec/vaapi_encode.h
@@ -244,28 +244,17 @@  typedef struct VAAPIEncodeContext {
 
     // Global parameters which will be applied at the start of the
     // sequence (includes rate control parameters below).
-    VAEncMiscParameterBuffer *global_params[MAX_GLOBAL_PARAMS];
+    int             global_params_type[MAX_GLOBAL_PARAMS];
+    const void     *global_params     [MAX_GLOBAL_PARAMS];
     size_t          global_params_size[MAX_GLOBAL_PARAMS];
     int          nb_global_params;
 
     // Rate control parameters.
-    struct {
-        VAEncMiscParameterBuffer misc;
-        VAEncMiscParameterRateControl rc;
-    } rc_params;
-    struct {
-        VAEncMiscParameterBuffer misc;
-        VAEncMiscParameterHRD hrd;
-    } hrd_params;
-    struct {
-        VAEncMiscParameterBuffer misc;
-        VAEncMiscParameterFrameRate fr;
-    } fr_params;
+    VAEncMiscParameterRateControl rc_params;
+    VAEncMiscParameterHRD        hrd_params;
+    VAEncMiscParameterFrameRate   fr_params;
 #if VA_CHECK_VERSION(0, 36, 0)
-    struct {
-        VAEncMiscParameterBuffer misc;
-        VAEncMiscParameterBufferQualityLevel quality;
-    } quality_params;
+    VAEncMiscParameterBufferQualityLevel quality_params;
 #endif
 
     // Per-sequence parameter structure (VAEncSequenceParameterBuffer*).
diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c
index 4cf99d7c78..d1427112ea 100644
--- a/libavcodec/vaapi_encode_h264.c
+++ b/libavcodec/vaapi_encode_h264.c
@@ -471,9 +471,9 @@  static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
             (ctx->va_bit_rate >> hrd->bit_rate_scale + 6) - 1;
 
         hrd->cpb_size_scale =
-            av_clip_uintp2(av_log2(ctx->hrd_params.hrd.buffer_size) - 15 - 4, 4);
+            av_clip_uintp2(av_log2(ctx->hrd_params.buffer_size) - 15 - 4, 4);
         hrd->cpb_size_value_minus1[0] =
-            (ctx->hrd_params.hrd.buffer_size >> hrd->cpb_size_scale + 4) - 1;
+            (ctx->hrd_params.buffer_size >> hrd->cpb_size_scale + 4) - 1;
 
         // CBR mode as defined for the HRD cannot be achieved without filler
         // data, so this flag cannot be set even with VAAPI CBR modes.
@@ -488,8 +488,8 @@  static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx)
 
         // This calculation can easily overflow 32 bits.
         bp->nal.initial_cpb_removal_delay[0] = 90000 *
-            (uint64_t)ctx->hrd_params.hrd.initial_buffer_fullness /
-            ctx->hrd_params.hrd.buffer_size;
+            (uint64_t)ctx->hrd_params.initial_buffer_fullness /
+            ctx->hrd_params.buffer_size;
         bp->nal.initial_cpb_removal_delay_offset[0] = 0;
     } else {
         sps->vui.nal_hrd_parameters_present_flag = 0;