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

[Andreas Rheinhardt]

Mark Thompson:
> On 24/02/2020 17:19, Andreas Rheinhardt wrote:
>> 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 set of
>>> equivalent unit types, 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          | 69 +++++++++++++++++++++++++++++++++++++++
>>>  libavcodec/cbs.h          |  9 +++++
>>>  libavcodec/cbs_internal.h | 60 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 138 insertions(+)
>>>
>>> ...
>>> diff --git a/libavcodec/cbs_internal.h b/libavcodec/cbs_internal.h
>>> index 4c5a535ca6..615f514a85 100644
>>> --- a/libavcodec/cbs_internal.h
>>> +++ b/libavcodec/cbs_internal.h
>>> @@ -25,11 +25,71 @@
>>>  #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,
>>> +};
>>> +
>>> +enum {
>>> +      // Maximum number of unit types described by the same unit type
>>> +      // descriptor.
>>> +      CBS_MAX_UNIT_TYPES = 16,
>>
>> This is quite big and I wonder whether it would not be better to
>> simply split the HEVC-slices into two range descriptors in order to
>> reduce this to three.
> 
> As-written, the range case is only covering the one actual range case (MPEG-2 slices).  I think it's preferable to leave the HEVC slice headers as-is, because having the full list there explicitly is much clearer.
> 
> As an alternative, would you prefer the array here to be a pointer + compound literal array?  It would avoid this constant and save few bytes of space on average, at the cost of the definitions being slightly more complex.
> 
No.

>>> +      // Maximum number of reference buffer offsets in any one unit.
>>> +      CBS_MAX_REF_OFFSETS = 2,
>>> +      // Special value used in a unit type descriptor to indicate that it
>>> +      // applies to a large range of types rather than a set of discrete
>>> +      // values.
>>> +      CBS_UNIT_TYPE_RANGE = -1,
>>> +};
>>> +
>>> +typedef struct CodedBitstreamUnitTypeDescriptor {
>>> +    // Number of entries in the unit_types array, or the special value
>>> +    // CBS_UNIT_TYPE_RANGE to indicate that the range fields should be
>>> +    // used instead.
>>> +    int nb_unit_types;
>>> +
>>> +    // Array of unit types that this entry describes.
>>> +    const CodedBitstreamUnitType unit_types[CBS_MAX_UNIT_TYPES];
>>> +
>>> +    // Start and end of unit type range, used if nb_unit_types == 0.
>>
>> nb_unit_types == 0 is actually used for the sentinel in the
>> CodedBitstreamUnitTypeDescriptor-array. nb_unit_types ==
>> CBS_UNIT_TYPE_RANGE indicates that this descriptor uses ranges.
> 
> Fixed.
> 
>>> +    const CodedBitstreamUnitType unit_type_range_start;
>>> +    const CodedBitstreamUnitType unit_type_range_end;
>>
>> The ranges could be free (size-wise) if you used a union with unit_types.
> 
> Anonymous unions are still not allowed in FFmpeg, unfortunately (they are C11, though many compilers supported them before that).
> 
What about a non-anonymous union? It would only be used in
cbs_find_unit_type_desc().
>>> +
>>> +    // The type of content described, from CBS_CONTENT_TYPE_*.
>>> +    int    content_type;
>>
>> Maybe make a proper type out of the CBS_CONTENT_TYPE_*-enum and use it
>> here?
> 
> That's a good idea; done.
> 
>>> +    // The size of the structure which should be allocated to contain
>>> +    // the decomposed content of this type of unit.
>>> +    size_t content_size;
>>> +
>>> +    // Number of entries in the ref_offsets array.  Only used if the
>>> +    // content_type is CBS_CONTENT_TYPE_INTERNAL_REFS.
>>> +    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);
>>
>> Is there a usecase for a dedicated free-function different for a unit
>> of type CBS_CONTENT_TYPE_INTERNAL_REFS? If not, then one could use a
>> union for this and the ref_offset stuff.
> 
> Yes, but the same anonymous union problem.
> 
>>> +} CodedBitstreamUnitTypeDescriptor;
>>
>> I wonder whether you should add const to the typedef in order to omit
>> it everywhere else. After all, no CodedBitstreamUnitTypeDescriptor
>> will ever be assembled during runtime.
> 
> It definitely makes sense to add it to reduce errors.  Not so sure about the removing it from everywhere else - the fact that it looks wrong at the point of use probably causes more confusion.
> 
> So, I've done the first part but not the second (helpfully, redundant type qualifiers have no effect).

MSVC emits a warning (or just a note or so) for this.

- Andreas