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

[Andreas Rheinhardt]

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