diff mbox series

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

Message ID 863c268b-cb82-e0c0-9bb9-f83b04055e06@jkqxz.net
State Superseded
Headers show
Series [FFmpeg-devel] 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 Jan. 26, 2021, 11:06 p.m. UTC 
---
On 26/01/2021 13:40, James Almer wrote:
> On 1/26/2021 10:35 AM, Nuo Mi wrote:
>> On Tue, Jan 26, 2021 at 5:54 AM James Almer <jamrial@gmail.com> wrote:
>>
>>> On 1/25/2021 2:12 PM, James Almer wrote:
>>>> Did you add this just to set active_vps? I thought the
>>>> idea was to remove all that since it's not a concept defined in h266.
>>>
>>> The following should get rid of active_*, but the actual fields can't be
>>> removed from CodedBitstreamH266Context unless we duplicate the
>>> cbs_h2645_replace_ps() macro to remove the relevant lines it shares with
>>> the other two codecs.
>>>
>> One solution  is  define a cbs_h26n_replace_ps.
>> cbs_h2645 _replace_ps calls  cbs_h26n_replace_ps and set active_xps to NULL.
>> cbs_h266_replace_ps calls the cbs_h26n_replace_ps only.
>> I did not use this since I think active_xps will make the caller's life
>> easier.
>> They do not need to use the id again and again to get pps, sps, vps.
>> If you think remove it is better, I can remove it.
> 
> I'll leave that decision to Mark. I know he tries to keep CBS as close as the spec as possible, which is why i assumed removing active_*ps for h266 felt like the proper thing to do.

I think I have to agree with that.

On messing with all those replace-PS functions, how would you like something like this to put the information in a table rather than generating lots of functions?

Further thoughts:
* active_offset can be set to a special value for H.266 to mean "active not a thing here".
* The table definition might be better with a carefully chosen macro.
* The replace_ps calls are actually all the same and depend only on the NAL unit type, so can we move them out of the switch?

Thanks,

- Mark


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

Comments

Nuo Mi Jan. 27, 2021, 1:52 p.m. UTC | #1 
On Wed, Jan 27, 2021 at 7:06 AM Mark Thompson <sw@jkqxz.net> wrote:

>
> +
> +    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);
> +    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);
>
This happend after ff_cbs_make_unit_refcounted, do we need urnef
unit->content_ref
before return?

> +    ptr_array[id] = ref_array[id]->data;
> +
> +    return 0;
> +}
>
>
> 2.29.2
> _______________________________________________
> 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".
Nuo Mi Jan. 30, 2021, 11:01 a.m. UTC | #2 
Hi Mark,
Will this fix and merged?
thanks

On Wed, Jan 27, 2021 at 9:52 PM Nuo Mi <nuomi2021@gmail.com> wrote:

>
>
> On Wed, Jan 27, 2021 at 7:06 AM Mark Thompson <sw@jkqxz.net> wrote:
>
>>
>> +
>> +    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);
>> +    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);
>>
> This happend after ff_cbs_make_unit_refcounted, do we need urnef unit->content_ref
> before return?
>
>> +    ptr_array[id] = ref_array[id]->data;
>> +
>> +    return 0;
>> +}
>>
>>
>> 2.29.2
>> _______________________________________________
>> 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".
>
>
Mark Thompson Feb. 2, 2021, 9:07 p.m. UTC | #3 
On 30/01/2021 11:01, Nuo Mi wrote:
> On Wed, Jan 27, 2021 at 9:52 PM Nuo Mi <nuomi2021@gmail.com> wrote:
>> On Wed, Jan 27, 2021 at 7:06 AM Mark Thompson <sw@jkqxz.net> wrote:
>>>
>>> +
>>> +    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);
>>> +    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);
>>>
>> This happend after ff_cbs_make_unit_refcounted, do we need urnef unit->content_ref
>> before return?

I don't think so?  The content_ref will be freed by the next call to fragment_reset/fragment_free, and we don't want to unset it here because that would force us to also clear unit->content as well (which doesn't matter for reading, but is a very strange side-effect if you are writing).

>>> +    ptr_array[id] = ref_array[id]->data;
>>> +
>>> +    return 0;
>>> +}
>>>
>>> > Hi Mark,
> Will this fix and merged?

Do you prefer it to what is presently there?

My intent was to suggest it and ask the question given the trouble with many versions of this in H.266, not to prescribe a particular answer.

- Mark
Nuo Mi Feb. 6, 2021, 3:28 a.m. UTC | #4 
On Wed, Feb 3, 2021 at 5:07 AM Mark Thompson <sw@jkqxz.net> wrote:

> On 30/01/2021 11:01, Nuo Mi wrote:
> > On Wed, Jan 27, 2021 at 9:52 PM Nuo Mi <nuomi2021@gmail.com> wrote:
> >> On Wed, Jan 27, 2021 at 7:06 AM Mark Thompson <sw@jkqxz.net> wrote:
> >>>
> >>> +
> >>> +    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);
> >>> +    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);
> >>>
> >> This happend after ff_cbs_make_unit_refcounted, do we need urnef
> unit->content_ref
> >> before return?
>
> I don't think so?  The content_ref will be freed by the next call to
> fragment_reset/fragment_free, and we don't want to unset it here because
> that would force us to also clear unit->content as well (which doesn't
> matter for reading, but is a very strange side-effect if you are writing).
>
> >>> +    ptr_array[id] = ref_array[id]->data;
> >>> +
> >>> +    return 0;
> >>> +}
> >>>
> >>> > Hi Mark,
> > Will this fix and merged?
>
> Do you prefer it to what is presently there?
>
> My intent was to suggest it and ask the question given the trouble with
> many versions of this in H.266, not to prescribe a particular answer.

Thanks for the suggestion, I will include the patch and send the new
searials.

>




> - Mark
> _______________________________________________
> 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..36212d1da6 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -661,38 +661,127 @@  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 {
+        int nal_unit_type;
+        int max_count;
+        const char *name;
+        size_t id_offset;
+        size_t ref_array_offset;
+        size_t ptr_array_offset;
+        size_t active_offset;
+    } PSType;
+
+    static const PSType h264_ps_types[] = {
+        {
+            H264_NAL_SPS,
+            H264_MAX_SPS_COUNT,
+            "SPS",
+            offsetof(H264RawSPS, seq_parameter_set_id),
+            offsetof(CodedBitstreamH264Context, sps_ref),
+            offsetof(CodedBitstreamH264Context, sps),
+            offsetof(CodedBitstreamH264Context, active_sps),
+        },
+        {
+            H264_NAL_PPS,
+            H264_MAX_PPS_COUNT,
+            "PPS",
+            offsetof(H264RawPPS, pic_parameter_set_id),
+            offsetof(CodedBitstreamH264Context, pps_ref),
+            offsetof(CodedBitstreamH264Context, pps),
+            offsetof(CodedBitstreamH264Context, active_pps),
+        },
+    };
+
+    static const PSType h265_ps_types[] = {
+        {
+            HEVC_NAL_VPS,
+            HEVC_MAX_VPS_COUNT,
+            "VPS",
+            offsetof(H265RawVPS, vps_video_parameter_set_id),
+            offsetof(CodedBitstreamH265Context, vps_ref),
+            offsetof(CodedBitstreamH265Context, vps),
+            offsetof(CodedBitstreamH265Context, active_vps),
+        },
+        {
+            HEVC_NAL_SPS,
+            HEVC_MAX_SPS_COUNT,
+            "SPS",
+            offsetof(H265RawSPS, sps_seq_parameter_set_id),
+            offsetof(CodedBitstreamH265Context, sps_ref),
+            offsetof(CodedBitstreamH265Context, sps),
+            offsetof(CodedBitstreamH265Context, active_sps),
+        },
+        {
+            HEVC_NAL_PPS,
+            HEVC_MAX_PPS_COUNT,
+            "PPS",
+            offsetof(H265RawPPS, pps_pic_parameter_set_id),
+            offsetof(CodedBitstreamH265Context, pps_ref),
+            offsetof(CodedBitstreamH265Context, pps),
+            offsetof(CodedBitstreamH265Context, active_pps),
+        },
+    };

-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)
+    const PSType *ps_type;
+    AVBufferRef **ref_array;
+    void **ptr_array;
+    void **active;
+    int err, id, i, nb_ps_types;
+
+    switch (ctx->codec->codec_id) {
+    case AV_CODEC_ID_H264:
+        ps_type     = h264_ps_types;
+        nb_ps_types = FF_ARRAY_ELEMS(h264_ps_types);
+        break;
+    case AV_CODEC_ID_H265:
+        ps_type     = h265_ps_types;
+        nb_ps_types = FF_ARRAY_ELEMS(h265_ps_types);
+        break;
+    default:
+        av_assert0(0);
+    }
+
+    for (i = 0; i < nb_ps_types; i++) {
+        if (ps_type->nal_unit_type == unit->type)
+            break;
+        ++ps_type;
+    }
+    av_assert0(i < nb_ps_types);
+
+    id = *((uint8_t*)unit->content + ps_type->id_offset);
+
+    if (id >= ps_type->max_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);
+    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 +806,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 +828,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 +925,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 +938,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 +952,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 +1096,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 +1120,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 +1213,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 +1227,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 +1241,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;
          }