[FFmpeg-devel] [PATCH v2 03/11] cbs: Describe allocate/free methods in tabular form

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Wed May 22 20:53:00 EEST 2019


Mark Thompson:
> Unit types are split into three categories, depending on how their
> content is managed:
> * POD structure - these require no special treatment.
> * Structure containing references to refcounted buffers - these can use
>   a common free function when the offsets of all the internal references
>   are known.
> * More complex structures - these still require ad-hoc treatment.
> 
> For each codec we can then maintain a table of descriptors for each unit
> type, defining the mechanism needed to allocate/free that unit content.
> This is not required to be used immediately - a new alloc function
> supports this, but does not replace the old one which works without
> referring to these tables.
> ---
>  libavcodec/cbs.c          | 62 +++++++++++++++++++++++++++++++++++++++
>  libavcodec/cbs.h          | 16 ++++++++++
>  libavcodec/cbs_internal.h | 36 +++++++++++++++++++++++
>  3 files changed, 114 insertions(+)
> 
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 0260ba6f67..1963d86133 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -754,3 +754,65 @@ int ff_cbs_delete_unit(CodedBitstreamContext *ctx,
>  
>      return 0;
>  }
> +
> +static void cbs_default_free_unit_content(void *opaque, uint8_t *data)
> +{
> +    const CodedBitstreamUnitTypeDescriptor *desc = opaque;
> +    if (desc->content_type == CBS_CONTENT_TYPE_INTERNAL_REFS) {
> +        int i;
> +        for (i = 0; i < desc->nb_ref_offsets; i++) {
> +            void **ptr = (void**)(data + desc->ref_offsets[i]);
> +            av_buffer_unref((AVBufferRef**)(ptr + 1));
> +            *ptr = NULL;

Why set *ptr to NULL? After all, the buffer that contains *ptr will be
freed anyway.

> +        }
> +    }
> +    av_free(data);
> +}
> +
> +static const CodedBitstreamUnitTypeDescriptor
> +    *cbs_find_unit_type_desc(CodedBitstreamContext *ctx,
> +                             CodedBitstreamUnit *unit)
> +{
> +    const CodedBitstreamUnitTypeDescriptor *desc;
> +    int i;
> +
> +    if (!ctx->codec->unit_types)
> +        return NULL;
> +
> +    for (i = 0;; i++) {
> +        desc = &ctx->codec->unit_types[i];
> +        if (desc->unit_type == CBS_INVALID_UNIT_TYPE)
> +            break;
> +        if (desc->unit_type == unit->type)
> +            return desc;
> +    }
> +    return NULL;
> +}
> +
> +int ff_cbs_alloc_unit_content2(CodedBitstreamContext *ctx,
> +                               CodedBitstreamUnit *unit)
> +{
> +    const CodedBitstreamUnitTypeDescriptor *desc;
> +
> +    av_assert0(!unit->content && !unit->content_ref);
> +
> +    desc = cbs_find_unit_type_desc(ctx, unit);
> +    if (!desc)
> +        return AVERROR(ENOSYS);
> +
> +    unit->content = av_mallocz(desc->content_size);
> +    if (!unit->content)
> +        return AVERROR(ENOMEM);
> +
> +    unit->content_ref =
> +        av_buffer_create(unit->content, desc->content_size,
> +                         desc->content_free ? desc->content_free
> +                                            : cbs_default_free_unit_content,
> +                         (void*)desc, 0);
> +    if (!unit->content_ref) {
> +        av_freep(&unit->content);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    return 0;
> +}
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index e8b2d41ecb..8f764905bd 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -54,6 +54,13 @@ struct CodedBitstreamType;
>   */
>  typedef uint32_t CodedBitstreamUnitType;
>  
> +/**
> + * Value which is never valid as a unit type.
> + *
> + * This can be used as a sentinel.
> + */
> +#define CBS_INVALID_UNIT_TYPE (UINT32_MAX)
> +
>  /**
>   * Coded bitstream unit structure.
>   *
> @@ -337,6 +344,15 @@ int ff_cbs_alloc_unit_content(CodedBitstreamContext *ctx,
>                                size_t size,
>                                void (*free)(void *unit, uint8_t *content));
>  
> +/**
> + * Allocate a new internal content buffer matching the type of the unit.
> + *
> + * The content will be zeroed.
> + */
> +int ff_cbs_alloc_unit_content2(CodedBitstreamContext *ctx,
> +                               CodedBitstreamUnit *unit);
> +
> +
>  /**
>   * Allocate a new internal data buffer of the given size in the unit.
>   *
> diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
> index dd4babf092..06a8f9b979 100644
> --- a/libavcodec/cbs_internal.h
> +++ b/libavcodec/cbs_internal.h
> @@ -25,11 +25,47 @@
>  #include "put_bits.h"
>  
>  
> +enum {
> +    // Unit content is a simple structure.
> +    CBS_CONTENT_TYPE_POD,
> +    // Unit content contains some references to other structures, but all
> +    // managed via buffer reference counting.  The descriptor defines the
> +    // structure offsets of every buffer reference.
> +    CBS_CONTENT_TYPE_INTERNAL_REFS,
> +    // Unit content is something more complex.  The descriptor defines
> +    // special functions to manage the content.
> +    CBS_CONTENT_TYPE_COMPLEX,
> +};
> +
> +// Maximum number of reference buffer offsets in any one unit.
> +#define CBS_MAX_REF_OFFSETS 1
> +
> +typedef struct CodedBitstreamUnitTypeDescriptor {
> +    CodedBitstreamUnitType unit_type;
> +
> +    int    content_type;
> +    size_t content_size;
> +
> +    int nb_ref_offsets;
> +    // The structure must contain two adjacent elements:
> +    //   type        *field;
> +    //   AVBufferRef *field_ref;
> +    // where field points to something in the buffer referred to by
> +    // field_ref.  This offset is then set to offsetof(struct, field).
> +    size_t ref_offsets[CBS_MAX_REF_OFFSETS];
> +
> +    void (*content_free)(void *opaque, uint8_t *data);
> +} CodedBitstreamUnitTypeDescriptor;
> +
>  typedef struct CodedBitstreamType {
>      enum AVCodecID codec_id;
>  
>      size_t priv_data_size;
>  
> +    // List of unit type descriptors for this codec.
> +    // Terminated by a descriptor with type CBS_INVALID_UNIT_TYPE.
> +    const CodedBitstreamUnitTypeDescriptor *unit_types;
> +
>      // Split frag->data into coded bitstream units, creating the
>      // frag->units array.  Fill data but not content on each unit.
>      // The header argument should be set if the fragment came from
> 

1. The approach with CBS_INVALID_UNIT_TYPE presupposes that there will
always be a number that is unused by all current and future codecs
supported by CBS. While this is reasonable, it would probably be
better to instead use a CodedBitstreamUnitTypeDescriptor with an
invalid content type CBS_CONTENT_TYPE_INVALID as a sentinel.
In any case, the type in "Terminated by a descriptor with type
CBS_INVALID_UNIT_TYPE." is potentially ambiguous as each
CodedBitstreamUnitTypeDescriptor has two types.

2. I see two ways to reduce the memory/size requirements of these tables:
a) Splitting nb_ref_offsets and ref_offsets into their own structure
and the function pointers into a separate structure and only include a
union of these two structures into CodedBitstreamUnitTypeDescriptor.
This would impair the ability to use non-standard free functions for
units of type CBS_CONTENT_TYPE_INTERNAL_REFS, but I don't think that
this is needed anyway.
b) Some codecs have multiple different unit types which are the same
for purposes of CBS. This is so for the slices in MPEG-2 and H.265.
And these unit types are often in a row. This could be exploited by
using the following system:
i) Add a new unit content type CBS_CONTENT_TYPE_ROW_END.
ii) Sort all the units by ascending unit_type.
iii) Use this loop in cbs_find_unit_type_desc:

for (desc = ctx->codec.unit_types;; desc++) {
    if (desc->unit_content == CBS_CONTENT_TYPE_INVALID)
        break;
    if (desc->unit_type >= unit->type) {
        if (desc->content_type == CBS_CONTENT_TYPE_ROW_END)
            return --desc;
        if (desc->unit_type == unit->type)
            return desc;
        break;
    }
}

(If you keep CBS_INVALID_UNIT_TYPE, then the first if clause is
unnecessary (as long as CBS_INVALID_UNIT_TYPE is greater than every
other unit type).)

The approach in b) has two downsides: ii) implies that units that
belong together logically might not be at adjacent places in the
relevant table (this applies to slices of auxiliary pictures in
H.264). And the returned CodedBitstreamUnitTypeDescriptor's unit_type
does no longer coincide with the unit's type. (In particular, the
assert in cbs_clone_unit_content for this would have to be removed.)

- Andreas


More information about the ffmpeg-devel mailing list