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

[Mark Thompson]

On 24/02/2020 22:10, Andreas Rheinhardt wrote:
> 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().

Mildly against because it would be ugly to bunch together the unrelated fields, but I could be pushed into it.

>>>> +
>>>> +    // 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.
Urgh.  Is that definitely intended or is it a bug in the compiler?  The C standard is very clear that this is fine (C11 6.7.3).

- Mark