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
_______________________________________________
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".
