| Message ID | 20220424101409.95486-6-zane@zanevaniperen.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add UUID functionality to libavutil | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 24/04/2022 11:14, Zane van Iperen wrote: > From: Pierre-Anthony Lemieux <pal@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. > diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c > index 7a6b54ab6f..b3105d6ccc 100644 > --- a/libavcodec/vaapi_encode_h264.c > +++ b/libavcodec/vaapi_encode_h264.c > @@ -25,6 +25,7 @@ > #include "libavutil/common.h" > #include "libavutil/internal.h" > #include "libavutil/opt.h" > +#include "libavutil/uuid.h" > > #include "avcodec.h" > #include "cbs.h" > @@ -43,7 +44,7 @@ enum { > }; > > // Random (version 4) ISO 11578 UUID. > -static const uint8_t vaapi_encode_h264_sei_identifier_uuid[16] = { > +static const AVUUID vaapi_encode_h264_sei_identifier_uuid = { > 0x59, 0x94, 0x8b, 0x28, 0x11, 0xec, 0x45, 0xaf, > 0x96, 0x75, 0x19, 0xd4, 0x1f, 0xea, 0xa9, 0x4d, > }; > @@ -1089,9 +1090,8 @@ static av_cold int vaapi_encode_h264_configure(AVCodecContext *avctx) > const char *driver; > int len; > > - memcpy(priv->sei_identifier.uuid_iso_iec_11578, > - vaapi_encode_h264_sei_identifier_uuid, > - sizeof(priv->sei_identifier.uuid_iso_iec_11578)); > + av_uuid_copy(priv->sei_identifier.uuid_iso_iec_11578, > + vaapi_encode_h264_sei_identifier_uuid); > > driver = vaQueryVendorString(ctx->hwctx->display); > if (!driver) This is fair. - Mark
On Sat, Apr 30, 2022 at 10:31 AM Mark Thompson <sw@jkqxz.net> wrote: > > On 24/04/2022 11:14, Zane van Iperen wrote: > > From: Pierre-Anthony Lemieux <pal@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? > > > diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c > > index 7a6b54ab6f..b3105d6ccc 100644 > > --- a/libavcodec/vaapi_encode_h264.c > > +++ b/libavcodec/vaapi_encode_h264.c > > @@ -25,6 +25,7 @@ > > #include "libavutil/common.h" > > #include "libavutil/internal.h" > > #include "libavutil/opt.h" > > +#include "libavutil/uuid.h" > > > > #include "avcodec.h" > > #include "cbs.h" > > @@ -43,7 +44,7 @@ enum { > > }; > > > > // Random (version 4) ISO 11578 UUID. > > -static const uint8_t vaapi_encode_h264_sei_identifier_uuid[16] = { > > +static const AVUUID vaapi_encode_h264_sei_identifier_uuid = { > > 0x59, 0x94, 0x8b, 0x28, 0x11, 0xec, 0x45, 0xaf, > > 0x96, 0x75, 0x19, 0xd4, 0x1f, 0xea, 0xa9, 0x4d, > > }; > > @@ -1089,9 +1090,8 @@ static av_cold int vaapi_encode_h264_configure(AVCodecContext *avctx) > > const char *driver; > > int len; > > > > - memcpy(priv->sei_identifier.uuid_iso_iec_11578, > > - vaapi_encode_h264_sei_identifier_uuid, > > - sizeof(priv->sei_identifier.uuid_iso_iec_11578)); > > + av_uuid_copy(priv->sei_identifier.uuid_iso_iec_11578, > > + vaapi_encode_h264_sei_identifier_uuid); > > > > driver = vaQueryVendorString(ctx->hwctx->display); > > if (!driver) > > This is fair. > > - 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-request@ffmpeg.org with subject "unsubscribe".
On 30/04/2022 18:53, Pierre-Anthony Lemieux wrote: > On Sat, Apr 30, 2022 at 10:31 AM Mark Thompson <sw@jkqxz.net> wrote: >> >> On 24/04/2022 11:14, Zane van Iperen wrote: >>> From: Pierre-Anthony Lemieux <pal@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.) - Mark
On Sat, Apr 30, 2022 at 12:26 PM Mark Thompson <sw@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@jkqxz.net> wrote: > >> > >> On 24/04/2022 11:14, Zane van Iperen wrote: > >>> From: Pierre-Anthony Lemieux <pal@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); 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. > > - 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-request@ffmpeg.org with subject "unsubscribe".
On 30/04/2022 21:53, Pierre-Anthony Lemieux wrote: > On Sat, Apr 30, 2022 at 12:26 PM Mark Thompson <sw@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@jkqxz.net> wrote: >>>> >>>> On 24/04/2022 11:14, Zane van Iperen wrote: >>>>> From: Pierre-Anthony Lemieux <pal@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
On 2/5/22 07:06, Mark Thompson wrote: >> 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. > Okay, I'll drop the header changes from the patch set. Zane
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; diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c index 7a6b54ab6f..b3105d6ccc 100644 --- a/libavcodec/vaapi_encode_h264.c +++ b/libavcodec/vaapi_encode_h264.c @@ -25,6 +25,7 @@ #include "libavutil/common.h" #include "libavutil/internal.h" #include "libavutil/opt.h" +#include "libavutil/uuid.h" #include "avcodec.h" #include "cbs.h" @@ -43,7 +44,7 @@ enum { }; // Random (version 4) ISO 11578 UUID. -static const uint8_t vaapi_encode_h264_sei_identifier_uuid[16] = { +static const AVUUID vaapi_encode_h264_sei_identifier_uuid = { 0x59, 0x94, 0x8b, 0x28, 0x11, 0xec, 0x45, 0xaf, 0x96, 0x75, 0x19, 0xd4, 0x1f, 0xea, 0xa9, 0x4d, }; @@ -1089,9 +1090,8 @@ static av_cold int vaapi_encode_h264_configure(AVCodecContext *avctx) const char *driver; int len; - memcpy(priv->sei_identifier.uuid_iso_iec_11578, - vaapi_encode_h264_sei_identifier_uuid, - sizeof(priv->sei_identifier.uuid_iso_iec_11578)); + av_uuid_copy(priv->sei_identifier.uuid_iso_iec_11578, + vaapi_encode_h264_sei_identifier_uuid); driver = vaQueryVendorString(ctx->hwctx->display); if (!driver)
From: Pierre-Anthony Lemieux <pal@palemieux.com> --- libavcodec/cbs_sei.h | 3 ++- libavcodec/vaapi_encode_h264.c | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-)