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

[Andreas Rheinhardt]

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).
> 
This is also the way it is in C99, but given that [1] says that it
leads to an error with MSVC in ANSI-C mode (which means C90), I looked
at C90 and found:
"The same type qualifier shall not appear more than once in the same
specifier list or qualifier list, either directly or via one or more
typedefs."

- Andreas

[1]:
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-1-c4114?view=vs-2019