[FFmpeg-devel,6/7] cbs: Add a function to make content of a unit writable

Submitted by Mark Thompson on Jan. 23, 2019, 11:01 p.m.

Details

Message ID 20190123230114.1086-7-sw@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson Jan. 23, 2019, 11:01 p.m.
Use the unit type table to determine what we need to do to clone the
internals of the unit content.  (Will still fail for units with complex
content if they do not have a defined clone function.)

Setup and naming from a patch by Andreas Rheinhardt
<andreas.rheinhardt@googlemail.com>, but with the implementation
changed to use the unit type information if possible rather than
requiring a codec-specific function.
---
 libavcodec/cbs.c          | 92 +++++++++++++++++++++++++++++++++++++++
 libavcodec/cbs.h          | 13 ++++++
 libavcodec/cbs_internal.h |  1 +
 3 files changed, 106 insertions(+)

Comments

Andreas Rheinhardt Jan. 24, 2019, 3:48 p.m.
From a macro point of view, I like your general approach using tables.
It really simplifies everything a lot.

(Rereading my old approach I don't know why I totally forgot that
there is a generic way to know the size of the internal buffers
(namely size of the AVBufferRefs). Not thinking of this, I opted for
the approach where the size is derived from other fields which made
everything very ungeneric and ugly. Sorry for wasting your time with
my earlier approach.)

Mark Thompson:
> +static int cbs_clone_unit_content(AVBufferRef **clone_ref,
> +                                  CodedBitstreamUnit *unit,
> +                                  const CodedBitstreamUnitTypeDescriptor *desc)
If this function were accessible from cbs_h2645.c, it could be used to
solve the dangling pointers problem in cbs_h26n_replace_xps by
performing a deep copy if the parameter sets are not refcounted (as I
did in "Implement copy-functions for parameter sets").
> +{
> +    uint8_t *src, *copy;
> +    AVBufferRef **src_buf, **copy_buf;
> +    int err, i = 0;
> +
> +    av_assert0(unit->type == desc->unit_type);
> +    av_assert0(unit->content);
> +    src = unit->content;
> +
> +    copy = av_malloc(desc->content_size);
> +    if (!copy)
> +        goto fail;
> +    memcpy(copy, src, desc->content_size);
> +
> +    for (i = 0; i < desc->nb_ref_offsets; i++) {
> +        src_buf  = (AVBufferRef**)(src  + desc->ref_offsets[i]);
> +        copy_buf = (AVBufferRef**)(copy + desc->ref_offsets[i]);
> +
> +        if (!*src_buf)
> +            continue;
> +
> +        *copy_buf = av_buffer_ref(*src_buf);
> +        if (!*copy_buf)
> +            goto fail;
> +
> +        err = av_buffer_make_writable(copy_buf);
> +        if (err < 0) {
> +            av_buffer_unref(copy_buf);
> +            goto fail;
> +        }
You make the internal reference buffers writable, but you forgot that
access to the data held in these buffers is not performed via
content->buf_ref->data, but via content->(pointer to data). These
pointers need to be updated, too; and the offsets of the pointers will
have to be added to the CodedBitstreamUnitTypeDescriptor.
> +    }
> +
> +    *clone_ref = av_buffer_create(copy, desc->content_size,
> +                                  desc->content_free ? desc->content_free :
> +                                  cbs_default_free_unit_content,
> +                                  (void*)desc, 0);
> +    if (!*clone_ref)
> +        goto fail;
> +
> +    return 0;
> +
> +fail:
> +    for (--i; i >= 0; i--)
> +        av_buffer_unref((AVBufferRef**)(copy + desc->ref_offsets[i]));
> +    av_freep(&copy);
> +    *clone_ref = NULL;
> +    return AVERROR(ENOMEM);
> +}
> +
> +int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
> +                              CodedBitstreamUnit *unit)
> +{
> +    const CodedBitstreamUnitTypeDescriptor *desc;
> +    AVBufferRef *ref;
> +    int err;
> +
> +    if (av_buffer_is_writable(unit->content_ref))
> +        return 0;
This check has the implicit assumption that the content is refcounted;
is this intended?
> +/**
> + * Make the content of a unit writable so that internal fields can be
> + * modified.
> + *
> + * If there are no other references to the content of the unit, does
> + * nothing and returned success.  Otherwise, it does a full clone of the
returns success.
> + * content (including any internal buffers) to make a new copy, and
> + * replaces the existing references inside the unit with that.
> + */
> +int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
> +                              CodedBitstreamUnit *unit);
> +

Patch hide | download patch | download mbox

diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
index deabf5dde5..cbf107528c 100644
--- a/libavcodec/cbs.c
+++ b/libavcodec/cbs.c
@@ -724,3 +724,95 @@  int ff_cbs_alloc_unit_content2(CodedBitstreamContext *ctx,
 
     return 0;
 }
+
+static int cbs_clone_unit_content(AVBufferRef **clone_ref,
+                                  CodedBitstreamUnit *unit,
+                                  const CodedBitstreamUnitTypeDescriptor *desc)
+{
+    uint8_t *src, *copy;
+    AVBufferRef **src_buf, **copy_buf;
+    int err, i = 0;
+
+    av_assert0(unit->type == desc->unit_type);
+    av_assert0(unit->content);
+    src = unit->content;
+
+    copy = av_malloc(desc->content_size);
+    if (!copy)
+        goto fail;
+    memcpy(copy, src, desc->content_size);
+
+    for (i = 0; i < desc->nb_ref_offsets; i++) {
+        src_buf  = (AVBufferRef**)(src  + desc->ref_offsets[i]);
+        copy_buf = (AVBufferRef**)(copy + desc->ref_offsets[i]);
+
+        if (!*src_buf)
+            continue;
+
+        *copy_buf = av_buffer_ref(*src_buf);
+        if (!*copy_buf)
+            goto fail;
+
+        err = av_buffer_make_writable(copy_buf);
+        if (err < 0) {
+            av_buffer_unref(copy_buf);
+            goto fail;
+        }
+    }
+
+    *clone_ref = av_buffer_create(copy, desc->content_size,
+                                  desc->content_free ? desc->content_free :
+                                  cbs_default_free_unit_content,
+                                  (void*)desc, 0);
+    if (!*clone_ref)
+        goto fail;
+
+    return 0;
+
+fail:
+    for (--i; i >= 0; i--)
+        av_buffer_unref((AVBufferRef**)(copy + desc->ref_offsets[i]));
+    av_freep(&copy);
+    *clone_ref = NULL;
+    return AVERROR(ENOMEM);
+}
+
+int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
+                              CodedBitstreamUnit *unit)
+{
+    const CodedBitstreamUnitTypeDescriptor *desc;
+    AVBufferRef *ref;
+    int err;
+
+    if (av_buffer_is_writable(unit->content_ref))
+        return 0;
+
+    desc = cbs_find_unit_type_desc(ctx, unit);
+    if (!desc)
+        return AVERROR(ENOSYS);
+
+    switch (desc->content_type) {
+    case CBS_CONTENT_TYPE_POD:
+        return av_buffer_make_writable(&unit->content_ref);
+
+    case CBS_CONTENT_TYPE_INTERNAL_REFS:
+        err = cbs_clone_unit_content(&ref, unit, desc);
+        break;
+
+    case CBS_CONTENT_TYPE_COMPLEX:
+        if (!desc->content_clone)
+            return AVERROR_PATCHWELCOME;
+        err = desc->content_clone(&ref, unit);
+        break;
+
+    default:
+        av_assert0(0 && "Invalid content type.");
+    }
+    if (err < 0)
+        return err;
+
+    av_buffer_unref(&unit->content_ref);
+    unit->content_ref = ref;
+    unit->content     = ref->data;
+    return 0;
+}
diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
index 1079f625bf..7e8f63139d 100644
--- a/libavcodec/cbs.h
+++ b/libavcodec/cbs.h
@@ -371,4 +371,17 @@  int ff_cbs_delete_unit(CodedBitstreamContext *ctx,
                        int position);
 
 
+/**
+ * Make the content of a unit writable so that internal fields can be
+ * modified.
+ *
+ * If there are no other references to the content of the unit, does
+ * nothing and returned success.  Otherwise, it does a full clone of the
+ * content (including any internal buffers) to make a new copy, and
+ * replaces the existing references inside the unit with that.
+ */
+int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
+                              CodedBitstreamUnit *unit);
+
+
 #endif /* AVCODEC_CBS_H */
diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
index aefe1a81e7..c7cf7f2192 100644
--- a/libavcodec/cbs_internal.h
+++ b/libavcodec/cbs_internal.h
@@ -50,6 +50,7 @@  typedef struct CodedBitstreamUnitTypeDescriptor {
     size_t ref_offsets[CBS_MAX_REF_OFFSETS];
 
     void (*content_free)(void *opaque, uint8_t *data);
+    int  (*content_clone)(AVBufferRef **ref, CodedBitstreamUnit *unit);
 } CodedBitstreamUnitTypeDescriptor;
 
 typedef struct CodedBitstreamType {