[FFmpeg-devel,v4,06/21] 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          | 172 ++++++++++++++++++++++++++++++++++++++
 libavcodec/cbs.h          |  29 +++++++
 libavcodec/cbs_internal.h |   1 +
 3 files changed, 202 insertions(+)

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

You may use my new email here.

> changed to use the unit type information if possible rather than
> requiring a codec-specific function.
> ---
>  libavcodec/cbs.c          | 172 ++++++++++++++++++++++++++++++++++++++
>  libavcodec/cbs.h          |  29 +++++++
>  libavcodec/cbs_internal.h |   1 +
>  3 files changed, 202 insertions(+)
> 
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 6cc559e545..91788f6dfb 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -881,3 +881,175 @@ 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;
> +    uint8_t **src_ptr, **copy_ptr;
> +    AVBufferRef **src_buf, **copy_buf;
> +    int err, i;
> +
> +    av_assert0(unit->content);
> +    src = unit->content;
> +
> +    copy = av_malloc(desc->content_size);
> +    if (!copy)
> +        return AVERROR(ENOMEM);
> +
> +    memcpy(copy, src, desc->content_size);

One can use av_memdup() for malloc+memcpy.

> +
> +    for (i = 0; i < desc->nb_ref_offsets; i++) {
> +        src_ptr  = (uint8_t**)(src + desc->ref_offsets[i]);
> +        src_buf  = (AVBufferRef**)(src_ptr + 1);
> +        copy_ptr = (uint8_t**)(copy + desc->ref_offsets[i]);
> +        copy_buf = (AVBufferRef**)(copy_ptr + 1);

In patch #2 you intend to make the AVBufferRef * directly follow the
pointer so that the above works. This has two drawbacks: It probably
works on all systems we care about, but it is not spec-compliant as a
compiler may add padding between these two elements. And furthermore
it forces you to make these ugly casts above.

How about the following approach: The *data pointer and the *data_ref
pointer will always be put into a dedicated structure (maybe even with
the size field?) ChildBuf (better name welcome) or whatever and the
descriptor contains the offset of these ChildBufs. Here is a scetch:

struct ChildBuf {
    uint8_t     *data;
    AVBufferRef *data_ref;
} ChildBuf;

  const ChildBuf *src_child = (const ChildBuf *)(src +
desc->child_offsets[i]);
  ChildBuf *copy_child = (ChildBuf *)(copy + desc->ref_offsets[i]);

  if (!src_child->data) {
    av_assert0(!src_child->data_ref);
    continue;
  }
...
  copy_child->data_ref = av_buffer_ref(src_child->data_ref);

I admit it would probably involve more writing (unless you can come up
with a really short name).

> +
> +        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(*src_ptr >= (*src_buf)->data &&
> +                   *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);

Making the child buf writable is neither necessary for fixing the
original problem, because h264_redundant_pps does not modifiy the
child buffer; nor is there any benefit to it: If unit->content_ref is
already writable, the child buffers will not be made writable, so that
a caller can not rely on the child buffers being writable after
ff_cbs_make_unit_writable() so that if he wants to modify them, a
further av_buffer_make_writable() for them is necessary.
Furthermore, if one only wants to modify e.g. a slice header that is
currently not writable (for whatever reason), one would also make the
internal ref (i.e. the unit's data) writable, which is expensive, but
one has no real choice because ff_cbs_make_unit_writable() does it
automatically.

> +        if (err < 0) {
> +            av_buffer_unref(copy_buf);
> +            goto fail;
> +        }
> +
> +        *copy_ptr = (*copy_buf)->data + (*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) {
> +        err = AVERROR(ENOMEM);
> +        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;
> +
> +    av_assert0(unit->content);
> +    if (unit->content_ref) {
> +        // Already refcounted, nothing to do.
> +        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;
> +
> +    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:
> +        err = av_buffer_make_writable(&unit->content_ref);
> +        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;
> +
> +    if (desc->content_type != CBS_CONTENT_TYPE_POD) {
> +        av_buffer_unref(&unit->content_ref);
> +        unit->content_ref = ref;
> +    }
> +    unit->content = unit->content_ref->data;
> +    return 0;
> +}
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index 2a5959a2b0..eb7f4b878b 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -407,4 +407,33 @@ void 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.
> + *
> + * It is not valid to call this function on a unit which does not have
> + * decomposed content.
> + */
> +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.
> + *
> + * It is not valid to call this function on a unit which does not have
> + * decomposed content.
> + */
> +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 2922878ed0..6fee4e90a5 100644
> --- a/libavcodec/cbs_internal.h
> +++ b/libavcodec/cbs_internal.h
> @@ -79,6 +79,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 {
>

On 24/02/2020 01:55, Andreas Rheinhardt wrote:
> 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
> 
> You may use my new email here.

Done.

>> changed to use the unit type information if possible rather than
>> requiring a codec-specific function.
>> ---
>>  libavcodec/cbs.c          | 172 ++++++++++++++++++++++++++++++++++++++
>>  libavcodec/cbs.h          |  29 +++++++
>>  libavcodec/cbs_internal.h |   1 +
>>  3 files changed, 202 insertions(+)
>>
>> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
>> index 6cc559e545..91788f6dfb 100644
>> --- a/libavcodec/cbs.c
>> +++ b/libavcodec/cbs.c
>> @@ -881,3 +881,175 @@ 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;
>> +    uint8_t **src_ptr, **copy_ptr;
>> +    AVBufferRef **src_buf, **copy_buf;
>> +    int err, i;
>> +
>> +    av_assert0(unit->content);
>> +    src = unit->content;
>> +
>> +    copy = av_malloc(desc->content_size);
>> +    if (!copy)
>> +        return AVERROR(ENOMEM);
>> +
>> +    memcpy(copy, src, desc->content_size);
> 
> One can use av_memdup() for malloc+memcpy.

Yep, changed.

>> +
>> +    for (i = 0; i < desc->nb_ref_offsets; i++) {
>> +        src_ptr  = (uint8_t**)(src + desc->ref_offsets[i]);
>> +        src_buf  = (AVBufferRef**)(src_ptr + 1);
>> +        copy_ptr = (uint8_t**)(copy + desc->ref_offsets[i]);
>> +        copy_buf = (AVBufferRef**)(copy_ptr + 1);
> 
> In patch #2 you intend to make the AVBufferRef * directly follow the
> pointer so that the above works. This has two drawbacks: It probably
> works on all systems we care about, but it is not spec-compliant as a
> compiler may add padding between these two elements. And furthermore
> it forces you to make these ugly casts above.

You would have to have a very weird ABI to cause problems here, so I'm not too worried - we already disallow differently-sized pointer types (see av_freep()), so it would have to be some extremely bizarre struct packing rule.

> How about the following approach: The *data pointer and the *data_ref
> pointer will always be put into a dedicated structure (maybe even with
> the size field?) ChildBuf (better name welcome) or whatever and the
> descriptor contains the offset of these ChildBufs. Here is a scetch:
> 
> struct ChildBuf {
>     uint8_t     *data;
>     AVBufferRef *data_ref;
> } ChildBuf;
> 
>   const ChildBuf *src_child = (const ChildBuf *)(src +
> desc->child_offsets[i]);
>   ChildBuf *copy_child = (ChildBuf *)(copy + desc->ref_offsets[i]);
> 
>   if (!src_child->data) {
>     av_assert0(!src_child->data_ref);
>     continue;
>   }
> ...
>   copy_child->data_ref = av_buffer_ref(src_child->data_ref);
> 
> I admit it would probably involve more writing (unless you can come up
> with a really short name).

Adding the extra layer in the name is annoyingly inelegant for both the declaration (extra structure) and the use (extra layer of indirection in the name).  Given that, I'm inclined to stay with the current approach without a stronger reason to change, because it isolates the ugliness to this one function.

I was thinking of adding a unit test which would go through all of the descriptor tables and structures to make sure that the offsets in the entries actually match.  Would that reassure you that the result is ok?

>> +
>> +        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(*src_ptr >= (*src_buf)->data &&
>> +                   *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);
> 
> Making the child buf writable is neither necessary for fixing the
> original problem, because h264_redundant_pps does not modifiy the
> child buffer; nor is there any benefit to it: If unit->content_ref is
> already writable, the child buffers will not be made writable, so that
> a caller can not rely on the child buffers being writable after
> ff_cbs_make_unit_writable() so that if he wants to modify them, a
> further av_buffer_make_writable() for them is necessary.
> Furthermore, if one only wants to modify e.g. a slice header that is
> currently not writable (for whatever reason), one would also make the
> internal ref (i.e. the unit's data) writable, which is expensive, but
> one has no real choice because ff_cbs_make_unit_writable() does it
> automatically.

You are right; I clearly didn't think about this carefully enough, thank you for your care in spotting that.  Removed.

>> +        if (err < 0) {
>> +            av_buffer_unref(copy_buf);
>> +            goto fail;
>> +        }
>> +
>> +        *copy_ptr = (*copy_buf)->data + (*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) {
>> +        err = AVERROR(ENOMEM);
>> +        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;
>> +}
>> ...
Thanks,

- Mark