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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
