[FFmpeg-devel,v2,06/11] cbs: Add support functions for handling unit content references

Use the unit type table to determine what we need to do to clone the
internals of the unit content when making copies for refcounting or
writeability.  (This 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          | 168 ++++++++++++++++++++++++++++++++++++++
 libavcodec/cbs.h          |  23 ++++++
 libavcodec/cbs_internal.h |   1 +
 3 files changed, 192 insertions(+)

On 5/20/2019 8:02 PM, Mark Thompson wrote:
> Use the unit type table to determine what we need to do to clone the
> internals of the unit content when making copies for refcounting or
> writeability.  (This 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          | 168 ++++++++++++++++++++++++++++++++++++++
>  libavcodec/cbs.h          |  23 ++++++
>  libavcodec/cbs_internal.h |   1 +
>  3 files changed, 192 insertions(+)
> 
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 1963d86133..9fc8e1eb47 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -816,3 +816,171 @@ 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;
> +    void **src_ptr, **copy_ptr;

Why use void if you're going to cast these to uint8_t everywhere?

> +    AVBufferRef **src_buf, **copy_buf;
> +    int err, i;
> +
> +    av_assert0(unit->type == desc->unit_type);
> +    av_assert0(unit->content);
> +    src = unit->content;
> +
> +    copy = av_malloc(desc->content_size);
> +    if (!copy) {
> +        err = AVERROR(ENOMEM);
> +        goto fail;
> +    }
> +    memcpy(copy, src, desc->content_size);
> +
> +    for (i = 0; i < desc->nb_ref_offsets; i++) {
> +        src_ptr  = (void**)(src + desc->ref_offsets[i]);
> +        src_buf  = (AVBufferRef**)(src_ptr + 1);
> +        copy_ptr = (void**)(copy + desc->ref_offsets[i]);
> +        copy_buf = (AVBufferRef**)(copy_ptr + 1);
> +
> +        if (!*src_ptr) {
> +            av_assert0(!*src_buf);
> +            continue;
> +        }
> +        if (!*src_buf) {
> +            // We can't handle a non-refcounted pointer here - we don't
> +            // have enough information to handle whatever structure lies
> +            // at the other end of it.
> +            err = AVERROR(EINVAL);
> +            goto fail;
> +        }
> +
> +        // src_ptr is required to point somewhere inside src_buf.  If it
> +        // doesn't, there is a bug somewhere.
> +        av_assert0((uint8_t*)*src_ptr >= (*src_buf)->data &&
> +                   (uint8_t*)*src_ptr <  (*src_buf)->data + (*src_buf)->size);
> +
> +        *copy_buf = av_buffer_ref(*src_buf);
> +        if (!*copy_buf) {
> +            err = AVERROR(ENOMEM);
> +            goto fail;
> +        }
> +
> +        err = av_buffer_make_writable(copy_buf);
> +        if (err < 0) {
> +            av_buffer_unref(copy_buf);
> +            goto fail;
> +        }

Maybe instead of buffer_ref + make_writable, just do a buffer_alloc +
memcpy. It should be faster and only requires one failure check.

> +
> +        *copy_ptr = (*copy_buf)->data +
> +            ((uint8_t*)*src_ptr - (*src_buf)->data);
> +    }
> +
> +    *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 err;
> +}
> +
> +int ff_cbs_make_unit_refcounted(CodedBitstreamContext *ctx,
> +                                CodedBitstreamUnit *unit)
> +{
> +    const CodedBitstreamUnitTypeDescriptor *desc;
> +    AVBufferRef *ref;
> +    int err;
> +
> +    if (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:
> +        ref = av_buffer_alloc(desc->content_size);
> +        if (!ref)
> +            return AVERROR(ENOMEM);
> +        memcpy(ref->data, unit->content, desc->content_size);
> +        err = 0;
> +        break;
> +
> +    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;
> +}
> +
> +int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
> +                              CodedBitstreamUnit *unit)
> +{
> +    const CodedBitstreamUnitTypeDescriptor *desc;
> +    AVBufferRef *ref;
> +    int err;
> +
> +    // This can only be applied to refcounted units.
> +    err = ff_cbs_make_unit_refcounted(ctx, unit);
> +    if (err < 0)
> +        return err;
> +    av_assert0(unit->content && unit->content_ref);
> +
> +    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 8f764905bd..f5d2dcdb08 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -396,4 +396,27 @@ int ff_cbs_delete_unit(CodedBitstreamContext *ctx,
>                         int position);
>  
>  
> +/**
> + * Make the content of a unit refcounted.
> + *
> + * If the unit is not refcounted, this will do a deep copy of the unit
> + * content to new refcounted buffers.
> + */
> +int ff_cbs_make_unit_refcounted(CodedBitstreamContext *ctx,
> +                                CodedBitstreamUnit *unit);
> +
> +/**
> + * Make the content of a unit writable so that internal fields can be
> + * modified.
> + *
> + * If it is known that there are no other references to the content of
> + * the unit, does nothing and returns success.  Otherwise (including the
> + * case where the unit content is not refcounted), 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 439fa7934b..194adc42c1 100644
> --- a/libavcodec/cbs_internal.h
> +++ b/libavcodec/cbs_internal.h
> @@ -55,6 +55,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);

Wouldn't implementing this directly in AVBufferRef be a better idea
instead? Right now it only supports flat data arrays, and having a
callback function that gets called when av_buffer_make_writable() makes
a copy of the buffer would allow us to also have complex structures,
with pointers or even other AVBufferRefs as well.

The relevant CBS changes from this patch would then be much simpler, and
it would also solve things like the decoupled qp_table frame side data
types, or how a copy of the entire raw sps is embedded in struct sps in
h264_ps.h as a 4kb array.

>  } CodedBitstreamUnitTypeDescriptor;
>  
>  typedef struct CodedBitstreamType {
>

Mark Thompson:
> Use the unit type table to determine what we need to do to clone the
> internals of the unit content when making copies for refcounting or
> writeability.  (This 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          | 168 ++++++++++++++++++++++++++++++++++++++
>  libavcodec/cbs.h          |  23 ++++++
>  libavcodec/cbs_internal.h |   1 +
>  3 files changed, 192 insertions(+)
> 
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 1963d86133..9fc8e1eb47 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -816,3 +816,171 @@ 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;
> +    void **src_ptr, **copy_ptr;

+ 1 to James' remark about the types.

> +    AVBufferRef **src_buf, **copy_buf;
> +    int err, i;
> +
> +    av_assert0(unit->type == desc->unit_type);
> +    av_assert0(unit->content);
> +    src = unit->content;
> +
> +    copy = av_malloc(desc->content_size);
> +    if (!copy) {
> +        err = AVERROR(ENOMEM);
> +        goto fail;
You need to initialize i before going to fail; or even better, simply
return AVERROR(ENOMEM);
> +    }
> +    memcpy(copy, src, desc->content_size);
> +
> +    for (i = 0; i < desc->nb_ref_offsets; i++) {
> +        src_ptr  = (void**)(src + desc->ref_offsets[i]);
> +        src_buf  = (AVBufferRef**)(src_ptr + 1);
> +        copy_ptr = (void**)(copy + desc->ref_offsets[i]);
> +        copy_buf = (AVBufferRef**)(copy_ptr + 1);
> +
> +        if (!*src_ptr) {
> +            av_assert0(!*src_buf);
> +            continue;
> +        }
> +        if (!*src_buf) {
> +            // We can't handle a non-refcounted pointer here - we don't
> +            // have enough information to handle whatever structure lies
> +            // at the other end of it.
> +            err = AVERROR(EINVAL);
> +            goto fail;
> +        }
> +
> +        // src_ptr is required to point somewhere inside src_buf.  If it
> +        // doesn't, there is a bug somewhere.
> +        av_assert0((uint8_t*)*src_ptr >= (*src_buf)->data &&
> +                   (uint8_t*)*src_ptr <  (*src_buf)->data + (*src_buf)->size);
> +
Just for completeness: Pointer comparisons on pointers that do not
point inside the same object (or one member past the end of an array)
is undefined behaviour, so the above is not a 100% kosher way to check
for whether src_ptr points inside src_buf. But it will probably work
anyway, so I don't mind.

> +        *copy_buf = av_buffer_ref(*src_buf);
> +        if (!*copy_buf) {
> +            err = AVERROR(ENOMEM);
> +            goto fail;
> +        }
> +
> +        err = av_buffer_make_writable(copy_buf);
> +        if (err < 0) {
> +            av_buffer_unref(copy_buf);
> +            goto fail;
> +        }
> +
> +        *copy_ptr = (*copy_buf)->data +
> +            ((uint8_t*)*src_ptr - (*src_buf)->data);

> +    }
> +
> +    *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;

You forgot to set err to AVERROR(ENOMEM).
> +
> +    return 0;
> +
> +fail:
> +    for (--i; i >= 0; i--)
> +        av_buffer_unref((AVBufferRef**)(copy + desc->ref_offsets[i]));
> +    av_freep(&copy);

av_free(copy) is enough.

> +    *clone_ref = NULL;
> +    return err;
> +}
> +
> +int ff_cbs_make_unit_refcounted(CodedBitstreamContext *ctx,
> +                                CodedBitstreamUnit *unit)
> +{
> +    const CodedBitstreamUnitTypeDescriptor *desc;
> +    AVBufferRef *ref;
> +    int err;
> +
> +    if (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:
> +        ref = av_buffer_alloc(desc->content_size);
> +        if (!ref)
> +            return AVERROR(ENOMEM);
> +        memcpy(ref->data, unit->content, desc->content_size);
> +        err = 0;
> +        break;
> +
> +    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 must be NULL, because otherwise the function would
have already returned 0 at the beginning. So this is unnecessary.

> +    unit->content_ref = ref;
> +    unit->content     = ref->data;
> +    return 0;
> +}
> +
> +int ff_cbs_make_unit_writable(CodedBitstreamContext *ctx,
> +                              CodedBitstreamUnit *unit)
> +{
> +    const CodedBitstreamUnitTypeDescriptor *desc;
> +    AVBufferRef *ref;
> +    int err;
> +
> +    // This can only be applied to refcounted units.
> +    err = ff_cbs_make_unit_refcounted(ctx, unit);
> +    if (err < 0)
> +        return err;
> +    av_assert0(unit->content && unit->content_ref);
> +
> +    if (av_buffer_is_writable(unit->content_ref))
> +        return 0;

Just to be sure: In the hypothetical scenario that the unit's content
buffer is writable, but one of the internal buffers is not,
ff_cbs_make_unit_writable does nothing. This is consistent with its
documentation below, but it makes me wonder why the internal buffers
are made writable at all if unit->content isn't writable. After all,
the user may not rely on the internal buffers being writable after a
call to ff_cbs_make_unit_writable and if he nevertheless rely on it
(and the behaviour in case unit->content isn't writable entices him to
this), he will be surprised.
Notice that it is not necessary to make the internal buffers writable
to fix h264_redundant_pps or the dangling pointers problem that exists
when a parameter set that is not refcounted is read; just making sure
that distinct copies of the parameter sets contain pointers to
distinct AVBufferRefs is enough for the latter.
If one proceeded along these lines, then one would only need to modify
the _ref pointers to the AVBufferRefs, not the pointer to the actual
data. In particular, it should be the offset of the AVBufferRef* that
should be part of the CodedBitstreamUnitTypeDescriptor.

(I am of course aware of the fact that checking whether a unit's
content is truly totally writable actually requires a new function for
every CBS_CONTENT_TYPE_COMPLEX unit, so I get why you did it the way
you did.)

> +
> +    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);
> +
Immediately returning here would leave the unit in an inconsistent
state where unit->content_ref->data pointed to something else than
unit->content (which would still not be writable).

> +    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 8f764905bd..f5d2dcdb08 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -396,4 +396,27 @@ int ff_cbs_delete_unit(CodedBitstreamContext *ctx,
>                         int position);
>  
>  
> +/**
> + * Make the content of a unit refcounted.
> + *
> + * If the unit is not refcounted, this will do a deep copy of the unit
> + * content to new refcounted buffers.
> + */
> +int ff_cbs_make_unit_refcounted(CodedBitstreamContext *ctx,
> +                                CodedBitstreamUnit *unit);
> +
> +/**
> + * Make the content of a unit writable so that internal fields can be
> + * modified.
> + *
> + * If it is known that there are no other references to the content of
> + * the unit, does nothing and returns success.  Otherwise (including the
> + * case where the unit content is not refcounted), 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);
> +

You should add that these two functions are only made for units that
have content at all. (memcpy(dst, NULL, size) is always undefined
behaviour.)

> +
>  #endif /* AVCODEC_CBS_H */
> diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
> index 439fa7934b..194adc42c1 100644
> --- a/libavcodec/cbs_internal.h
> +++ b/libavcodec/cbs_internal.h
> @@ -55,6 +55,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 {
> 

- Andreas