diff mbox series

[FFmpeg-devel,v2] cbs_h2645: Implement replace-PS with a table rather than many functions

Message ID ce9afa74-f16d-6a69-b1d7-6e2b7ba99fdb@jkqxz.net
State New
Headers show
Series [FFmpeg-devel,v2] cbs_h2645: Implement replace-PS with a table rather than many functions | expand

Checks

Context Check Description
andriy/configure warning Failed to apply patch

Commit Message

Mark Thompson Feb. 15, 2021, 11:12 p.m. UTC 
While this is mode source code, it generates less binary code and can be
more cleanly extended to H.266.
---
On 14/02/2021 19:45, Andreas Rheinhardt wrote:
> Nuo Mi:
>> From: Mark Thompson <sw@jkqxz.net>
>>
>> ---
>>   libavcodec/cbs_h2645.c | 171 +++++++++++++++++++++++++++++++----------
>>   1 file changed, 130 insertions(+), 41 deletions(-)
>>
>> ...
>> +static int cbs_h2645_replace_ps(CodedBitstreamContext *ctx,
>> +                                CodedBitstreamUnit *unit)
>> +{
>> +    typedef struct {
>> +        int nal_unit_type;
>> +        int max_count;
>> +        const char *name;
> 
> If this were a char name[4], these structs could be put in .rodata
> instead of .data (or .data.rel.ro) when using position-independent code.

True, that would be nicer.

New patch enclosing, which also uses some macros to simplify the arrays (and adds some explanatory comments).

This includes the no-active case, so extension to H.266 requires no additional logic:

#define H266_PS_TYPE(cname, uname, id_name) \
     H2645_PS_TYPE(H266, VVC_NUT_ ## cname, cname, uname, \
                   VVC_MAX_ ## cname ## _COUNT, \
                   uname ## _ ## id_name, common)

Then, "H266_PS_TYPE(APS, aps, adaptation)", etc.

Thanks,

- Mark

  libavcodec/cbs_h2645.c | 161 ++++++++++++++++++++++++++++++-----------
  1 file changed, 120 insertions(+), 41 deletions(-)

Comments

Nuo Mi Feb. 16, 2021, 3:16 p.m. UTC | #1 
On Tue, Feb 16, 2021 at 7:20 AM Mark Thompson <sw@jkqxz.net> wrote:

> While this is mode source code, it generates less binary code and can be
> more cleanly extended to H.266.
> ---
> On 14/02/2021 19:45, Andreas Rheinhardt wrote:
> > Nuo Mi:
> >> From: Mark Thompson <sw@jkqxz.net>
>
> Hi Andreas & Mark,
thanks for the suggestion.

Hi Mark,
vvc's xps does not need active_offset. vvc's picture header does not need
id_offset. see [1], [2] for details
How about we change the patch like this:

$git diff
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index 6c44e781fa..57c419aa05 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -690,20 +690,24 @@ static int cbs_h2645_replace_ps(CodedBitstreamContext
*ctx,
         size_t active_offset;
     } PSType;

-#define H2645_PS_TYPE(codec, nal, cname, uname, count, id_name,
active_field) { \
+#define H26456_PS_TYPE(codec, nal, cname, uname, count, id_off,
active_off) { \
         .codec_id         = AV_CODEC_ID_ ## codec, \
         .nal_unit_type    = nal, \
         .name             = #cname, \
         .id_count         = count, \
-        .id_offset        = offsetof(codec ## Raw ## cname, \
-                                     id_name ## _parameter_set_id), \
+        .id_offset        = id_off, \
         .ref_array_offset = offsetof(CodedBitstream ## codec ## Context, \
                                      uname ## _ref), \
         .ptr_array_offset = offsetof(CodedBitstream ## codec ## Context, \
                                      uname), \
-        .active_offset    = offsetof(CodedBitstream ## codec ## Context, \
-                                     active_field), \
+        .active_offset    = active_off, \
     }
+
+#define H2645_PS_TYPE(codec, nal, cname, uname, count, id_name,
active_field) \
+        H26456_PS_TYPE(codec, nal, cname, uname, count, \
+            offsetof(codec ## Raw ## cname, id_name ## _parameter_set_id),
\
+            offsetof(CodedBitstream ## codec ## Context, active_field))
+
 #define H264_PS_TYPE(cname, uname, id_name) \
     H2645_PS_TYPE(H264, H264_NAL_ ## cname, cname, uname, \
                   H264_MAX_ ## cname ## _COUNT, \
@@ -724,7 +728,7 @@ static int cbs_h2645_replace_ps(CodedBitstreamContext
*ctx,
     const PSType *ps_type;
     AVBufferRef **ref_array;
     void **ptr_array;
-    int err, id, i;
+    int err, id = 0, i;

     ps_type = NULL;
     for (i = 0; i < FF_ARRAY_ELEMS(ps_types); i++) {
@@ -736,12 +740,14 @@ static int cbs_h2645_replace_ps(CodedBitstreamContext
*ctx,
     }
     av_assert0(ps_type);

-    id = *((uint8_t*)unit->content + ps_type->id_offset);
+    if (ps_type->id_offset) {
+        id = *((uint8_t*)unit->content + ps_type->id_offset);

-    if (id >= ps_type->id_count) {
-        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid %s id: %d.\n",
-               ps_type->name, id);
-        return AVERROR_INVALIDDATA;
+        if (id >= ps_type->id_count) {
+            av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid %s id: %d.\n",
+                ps_type->name, id);
+            return AVERROR_INVALIDDATA;
+        }
     }

     err = ff_cbs_make_unit_refcounted(ctx, unit);


[1]:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210211143611.5663-5-nuomi2021@gmail.com/
[2] h266_ps_types in
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210211143611.5663-6-nuomi2021@gmail.com/
James Almer Feb. 16, 2021, 3:35 p.m. UTC | #2 
On 2/16/2021 12:16 PM, Nuo Mi wrote:
> On Tue, Feb 16, 2021 at 7:20 AM Mark Thompson <sw@jkqxz.net> wrote:
> 
>> While this is mode source code, it generates less binary code and can be
>> more cleanly extended to H.266.
>> ---
>> On 14/02/2021 19:45, Andreas Rheinhardt wrote:
>>> Nuo Mi:
>>>> From: Mark Thompson <sw@jkqxz.net>
>>
>> Hi Andreas & Mark,
> thanks for the suggestion.
> 
> Hi Mark,
> vvc's xps does not need active_offset.

The change you suggest here is not needed to handle this, as you only 
need to define a H266_PS_TYPE macro that does not set .active_offset 
(When using designated initializers, fields that are not set will be 
zeroed).
But at the end of the day, it's a cosmetic choice, so whatever you two 
prefer is fine.

 > vvc's picture header does not need id_offset. see [1], [2] for details

This one most likely needs the change you suggested below, though.

> How about we change the patch like this:
> 
> $git diff
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index 6c44e781fa..57c419aa05 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -690,20 +690,24 @@ static int cbs_h2645_replace_ps(CodedBitstreamContext
> *ctx,
>           size_t active_offset;
>       } PSType;
> 
> -#define H2645_PS_TYPE(codec, nal, cname, uname, count, id_name,
> active_field) { \
> +#define H26456_PS_TYPE(codec, nal, cname, uname, count, id_off,
> active_off) { \
>           .codec_id         = AV_CODEC_ID_ ## codec, \
>           .nal_unit_type    = nal, \
>           .name             = #cname, \
>           .id_count         = count, \
> -        .id_offset        = offsetof(codec ## Raw ## cname, \
> -                                     id_name ## _parameter_set_id), \
> +        .id_offset        = id_off, \
>           .ref_array_offset = offsetof(CodedBitstream ## codec ## Context, \
>                                        uname ## _ref), \
>           .ptr_array_offset = offsetof(CodedBitstream ## codec ## Context, \
>                                        uname), \
> -        .active_offset    = offsetof(CodedBitstream ## codec ## Context, \
> -                                     active_field), \
> +        .active_offset    = active_off, \
>       }
> +
> +#define H2645_PS_TYPE(codec, nal, cname, uname, count, id_name,
> active_field) \
> +        H26456_PS_TYPE(codec, nal, cname, uname, count, \
> +            offsetof(codec ## Raw ## cname, id_name ## _parameter_set_id),
> \
> +            offsetof(CodedBitstream ## codec ## Context, active_field))
> +
>   #define H264_PS_TYPE(cname, uname, id_name) \
>       H2645_PS_TYPE(H264, H264_NAL_ ## cname, cname, uname, \
>                     H264_MAX_ ## cname ## _COUNT, \
> @@ -724,7 +728,7 @@ static int cbs_h2645_replace_ps(CodedBitstreamContext
> *ctx,
>       const PSType *ps_type;
>       AVBufferRef **ref_array;
>       void **ptr_array;
> -    int err, id, i;
> +    int err, id = 0, i;
> 
>       ps_type = NULL;
>       for (i = 0; i < FF_ARRAY_ELEMS(ps_types); i++) {
> @@ -736,12 +740,14 @@ static int cbs_h2645_replace_ps(CodedBitstreamContext
> *ctx,
>       }
>       av_assert0(ps_type);
> 
> -    id = *((uint8_t*)unit->content + ps_type->id_offset);
> +    if (ps_type->id_offset) {
> +        id = *((uint8_t*)unit->content + ps_type->id_offset);
> 
> -    if (id >= ps_type->id_count) {
> -        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid %s id: %d.\n",
> -               ps_type->name, id);
> -        return AVERROR_INVALIDDATA;
> +        if (id >= ps_type->id_count) {
> +            av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid %s id: %d.\n",
> +                ps_type->name, id);
> +            return AVERROR_INVALIDDATA;
> +        }
>       }
> 
>       err = ff_cbs_make_unit_refcounted(ctx, unit);
> 
> 
> [1]:
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210211143611.5663-5-nuomi2021@gmail.com/
> [2] h266_ps_types in
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20210211143611.5663-6-nuomi2021@gmail.com/
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox series

Patch

diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index 6005d46e0d..6c44e781fa 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -661,38 +661,117 @@  static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
      return 0;
  }

-#define cbs_h2645_replace_ps(h26n, ps_name, ps_var, id_element) \
-static int cbs_h26 ## h26n ## _replace_ ## ps_var(CodedBitstreamContext *ctx, \
-                                                  CodedBitstreamUnit *unit)  \
-{ \
-    CodedBitstreamH26 ## h26n ## Context *priv = ctx->priv_data; \
-    H26 ## h26n ## Raw ## ps_name *ps_var = unit->content; \
-    unsigned int id = ps_var->id_element; \
-    int err; \
-    if (id >= FF_ARRAY_ELEMS(priv->ps_var)) { \
-        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid " #ps_name \
-               " id : %d.\n", id); \
-        return AVERROR_INVALIDDATA; \
-    } \
-    err = ff_cbs_make_unit_refcounted(ctx, unit); \
-    if (err < 0) \
-        return err; \
-    if (priv->ps_var[id] == priv->active_ ## ps_var) \
-        priv->active_ ## ps_var = NULL ; \
-    av_buffer_unref(&priv->ps_var ## _ref[id]); \
-    av_assert0(unit->content_ref); \
-    priv->ps_var ## _ref[id] = av_buffer_ref(unit->content_ref); \
-    if (!priv->ps_var ## _ref[id]) \
-        return AVERROR(ENOMEM); \
-    priv->ps_var[id] = (H26 ## h26n ## Raw ## ps_name *)priv->ps_var ## _ref[id]->data; \
-    return 0; \
-}
+static int cbs_h2645_replace_ps(CodedBitstreamContext *ctx,
+                                CodedBitstreamUnit *unit)
+{
+    typedef struct {
+        // Codec this parameter set exists in.
+        enum AVCodecID codec_id;
+        // The NAL unit type corresponding to this parameter set type.
+        int    nal_unit_type;
+        // Name of the parameter set.  This field is large enough to
+        // contain a string of the form "XPS".
+        char   name[4];
+        // The maximum number of this type of parameter set which might
+        // be stored.  The greatest valid id is also one less than this.
+        int    id_count;
+        // Offset of the ID field (uint8_t) in the decomposed raw
+        // parameter set structure.
+        size_t id_offset;
+        // Offset of the reference array (AVBufferRef*[]) in the codec
+        // private context.
+        size_t ref_array_offset;
+        // Offset of the pointer array (CodecRawXPS*[]) in the codec
+        // private context.
+        size_t ptr_array_offset;
+        // Offset of the active field (const CodecRawXPS*) in the codec
+        // private context, or zero if this codec does not have active
+        // parameter sets.
+        size_t active_offset;
+    } PSType;
+
+#define H2645_PS_TYPE(codec, nal, cname, uname, count, id_name, active_field) { \
+        .codec_id         = AV_CODEC_ID_ ## codec, \
+        .nal_unit_type    = nal, \
+        .name             = #cname, \
+        .id_count         = count, \
+        .id_offset        = offsetof(codec ## Raw ## cname, \
+                                     id_name ## _parameter_set_id), \
+        .ref_array_offset = offsetof(CodedBitstream ## codec ## Context, \
+                                     uname ## _ref), \
+        .ptr_array_offset = offsetof(CodedBitstream ## codec ## Context, \
+                                     uname), \
+        .active_offset    = offsetof(CodedBitstream ## codec ## Context, \
+                                     active_field), \
+    }
+#define H264_PS_TYPE(cname, uname, id_name) \
+    H2645_PS_TYPE(H264, H264_NAL_ ## cname, cname, uname, \
+                  H264_MAX_ ## cname ## _COUNT, \
+                  id_name, active_ ## uname)
+#define H265_PS_TYPE(cname, uname, id_name) \
+    H2645_PS_TYPE(H265, HEVC_NAL_ ## cname, cname, uname, \
+                  HEVC_MAX_ ## cname ## _COUNT, \
+                  uname ## _ ## id_name, active_ ## uname)
+
+    static const PSType ps_types[] = {
+        H264_PS_TYPE(SPS, sps, seq),
+        H264_PS_TYPE(PPS, pps, pic),
+        H265_PS_TYPE(VPS, vps, video),
+        H265_PS_TYPE(SPS, sps, seq),
+        H265_PS_TYPE(PPS, pps, pic),
+    };
+
+    const PSType *ps_type;
+    AVBufferRef **ref_array;
+    void **ptr_array;
+    int err, id, i;
+
+    ps_type = NULL;
+    for (i = 0; i < FF_ARRAY_ELEMS(ps_types); i++) {
+        if (ps_types[i].codec_id == ctx->codec->codec_id &&
+            ps_types[i].nal_unit_type == unit->type) {
+            ps_type = &ps_types[i];
+            break;
+        }
+    }
+    av_assert0(ps_type);
+
+    id = *((uint8_t*)unit->content + ps_type->id_offset);
+
+    if (id >= ps_type->id_count) {
+        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid %s id: %d.\n",
+               ps_type->name, id);
+        return AVERROR_INVALIDDATA;
+    }
+
+    err = ff_cbs_make_unit_refcounted(ctx, unit);
+    if (err < 0)
+        return err;
+
+    ref_array = (AVBufferRef**)((uint8_t*)ctx->priv_data +
+                                ps_type->ref_array_offset);
+    ptr_array = (void**)       ((uint8_t*)ctx->priv_data +
+                                ps_type->ptr_array_offset);

-cbs_h2645_replace_ps(4, SPS, sps, seq_parameter_set_id)
-cbs_h2645_replace_ps(4, PPS, pps, pic_parameter_set_id)
-cbs_h2645_replace_ps(5, VPS, vps, vps_video_parameter_set_id)
-cbs_h2645_replace_ps(5, SPS, sps, sps_seq_parameter_set_id)
-cbs_h2645_replace_ps(5, PPS, pps, pps_pic_parameter_set_id)
+    if (ps_type->active_offset) {
+        void **active = (void**)((uint8_t*)ctx->priv_data +
+                                 ps_type->active_offset);
+
+        if (ptr_array[id] == *active) {
+            // The old active parameter set is being overwritten, so it can't
+            // be active after this point.
+            *active = NULL;
+        }
+    }
+    av_buffer_unref(&ref_array[id]);
+
+    ref_array[id] = av_buffer_ref(unit->content_ref);
+    if (!ref_array[id])
+        return AVERROR(ENOMEM);
+    ptr_array[id] = ref_array[id]->data;
+
+    return 0;
+}

  static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
                                    CodedBitstreamUnit *unit)
@@ -717,7 +796,7 @@  static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h264_replace_sps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -739,7 +818,7 @@  static int cbs_h264_read_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h264_replace_pps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -836,7 +915,7 @@  static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h265_replace_vps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -849,7 +928,7 @@  static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h265_replace_sps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -863,7 +942,7 @@  static int cbs_h265_read_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h265_replace_pps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -1007,7 +1086,7 @@  static int cbs_h264_write_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h264_replace_sps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -1031,7 +1110,7 @@  static int cbs_h264_write_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h264_replace_pps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -1124,7 +1203,7 @@  static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h265_replace_vps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -1138,7 +1217,7 @@  static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h265_replace_sps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }
@@ -1152,7 +1231,7 @@  static int cbs_h265_write_nal_unit(CodedBitstreamContext *ctx,
              if (err < 0)
                  return err;

-            err = cbs_h265_replace_pps(ctx, unit);
+            err = cbs_h2645_replace_ps(ctx, unit);
              if (err < 0)
                  return err;
          }