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

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(+)

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);
> +