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