[FFmpeg-devel] [PATCH v2 5/7] avcodec/cbs_sei: refactor to use avutil/uuid

[Mark Thompson]

On 30/04/2022 21:53, Pierre-Anthony Lemieux wrote:
> On Sat, Apr 30, 2022 at 12:26 PM Mark Thompson <sw at jkqxz.net> wrote:
>>
>> On 30/04/2022 18:53, Pierre-Anthony Lemieux wrote:
>>> On Sat, Apr 30, 2022 at 10:31 AM Mark Thompson <sw at jkqxz.net> wrote:
>>>>
>>>> On 24/04/2022 11:14, Zane van Iperen wrote:
>>>>> From: Pierre-Anthony Lemieux <pal at palemieux.com>
>>>>>
>>>>> ---
>>>>>     libavcodec/cbs_sei.h           | 3 ++-
>>>>>     libavcodec/vaapi_encode_h264.c | 8 ++++----
>>>>>     2 files changed, 6 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/cbs_sei.h b/libavcodec/cbs_sei.h
>>>>> index c7a7a95be0..67c6e6cbbd 100644
>>>>> --- a/libavcodec/cbs_sei.h
>>>>> +++ b/libavcodec/cbs_sei.h
>>>>> @@ -23,6 +23,7 @@
>>>>>     #include <stdint.h>
>>>>>
>>>>>     #include "libavutil/buffer.h"
>>>>> +#include "libavutil/uuid.h"
>>>>>
>>>>>     #include "cbs.h"
>>>>>     #include "sei.h"
>>>>> @@ -41,7 +42,7 @@ typedef struct SEIRawUserDataRegistered {
>>>>>     } SEIRawUserDataRegistered;
>>>>>
>>>>>     typedef struct SEIRawUserDataUnregistered {
>>>>> -    uint8_t      uuid_iso_iec_11578[16];
>>>>> +    AVUUID      uuid_iso_iec_11578;
>>>>>         uint8_t     *data;
>>>>>         AVBufferRef *data_ref;
>>>>>         size_t       data_length;
>>>>
>>>> This feels like a step backwards?  The syntax template files are explicitly relying on this being uint8_t[16], so giving it a different name is confusing.
>>>
>>> What/where are the syntax template files?
>>
>> <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/cbs_sei_syntax_template.c#l87>
>>
>> (It's included twice by cbs_h2645.c for read/write with different macro setups.)
> 
> Ok. Thanks. Are you concerned that the following line assumes that
> uuid_iso_iec_11578 is uint8_t[16] instead of being the opaque AVUUID?
> 
> us(8, uuid_iso_iec_11578[i], 0x00, 0xff, 1, i);

Yes.

> Did I get this right? If so, couple of options come to mind:
> 
> (a) revert the change
> (b) define a macro to access individual bytes of AVUUID, thereby
> keeping AVUUID opaque
> (c) not handle AVUUID as opaque, but instead always as uint8_t[16]
> 
> Maybe additional options exist.
> 
> I do not have a definitive opinion. Some folks expressed strong
> interest in having a consistent scheme for manipulating UUIDs.

I think for now the simplest option is just not to change the CBS header, which is completely fine with your current patch set.

Thanks,

- Mark