Re: [FFmpeg-devel] [PATCH 001/279] Add a new channel layout API

Re: [FFmpeg-devel] [PATCH 001/279] Add a new channel layout API

[Marton Balint]

[-- Attachment #1: Type: text/plain, Size: 4234 bytes --]



On Wed, 15 Dec 2021, James Almer wrote:

>
>
> On 12/15/2021 7:24 AM, Marton Balint wrote:
>>
>>
>>  On Tue, 14 Dec 2021, James Almer wrote:
>>
>>>  On 12/14/2021 3:54 PM, Nicolas George wrote:
>>>>   James Almer (12021-12-14):
>>>>>   We add a const uint8_t* field to AVChannelCustom. If the user wants to
>>>>>   allocate the strings instead, and not worry about their lifetime, they
>>>>>   can
>>>>>   provide the layout with a custom free() function that will take care
>>>>>  of
>>>>>   cleaning anything they came up with on uninit().
>>>>
>>>>   I understood what you suggested, and it amounts to letting API users
>>>>   fend for themselves. We can do that, but I would prefer if we only did
>>>>   on last resort, if we do not find a more convenient solution.
>>>
>>>  There's "char name[16]". Bigger than a pointer (Could be 8 bytes instead,
>>>  but then it's kinda small). The user will not have to worry about the
>>>  lifetime of anything then.
>>>
>>>  I'm attaching a diff that goes on top of the last patch of this set
>>>  implementing this. It also adds support for these custom names to
>>>  av_channel_layout_describe(), av_channel_layout_index_from_string() and
>>>  av_channel_layout_channel_from_string(), including tests.
>>
>>  I'd rather not mix custom labels with our fixed names for channels. E.g.
>>  what if a label conflicts with our existing channel names? If the user
>>  wants to specify a channel based on its label, that should be a separate
>>  syntax IMHO.
>>
>>  Overall, having a char name[16] is still kind of limiting, e.g. a label
>>  read from a muxed file will be truncated, I'd rather not have anything.
>
> Container metadata is typically be exported as stream metadata or side data.

That is always a possibility, but if you want some data per-channel, and 
not per-stream, (e.g. variable size labels) then side data becomes 
difficult to work with.

>
>>
>>  Here is one more idea, kind of a mix of what I read so far: Let's refcount
>>  only the dynamically allocated part of AVChannelLayout. Something like:
>>
>>  typedef struct AVChannelLayout {
>>       enum AVChannelOrder order;
>>       int nb_channels;
>>       uint64_t mask;
>>       AVBufferRef *custom;
>>  } AVChannelLayout;
>>
>>  And the reference counted data could point to an array of AVChannelCustom
>>  entries. And AVChannelCustom can have AVDictionary *metadata, because it
>>  is refcounted.
>
> AVBufferRef is meant to hold flat arrays, though.

Since sizeof(AVChannelCustom) is fixed, I don't really see the difference.

> the custom channel names? As a fixed array like in the above, or in the 
> dictionary? The latter sounds like a nightmare for both the helpers and as an 
> API user.

Personally I think it can be put in the metadata dictionary, because if 
we work out some syntax to select channel based on their labels, then it 
also make sense to select channels based on other metadata, and syntax 
might be extended with it. E.g. FL.label.Oboe. or FL.language.eng.

But if for some reason that is frowned upon, we can extend AVChannelCustom 
with a char *label, and free it as well on free().


> Also, since AVChannelCustom would have a dictionary, the AVBufferRef holding 
> it needs a custom free() function. This makes creating layouts manually a 
> pain.

You are already initalizing the dynamically allocated part of 
AVChannelLayout:

layout.u.map = av_mallocz_array(nb_channels, sizeof());

If AVBufferRef is used for that, then a helper function can be added:

layout.custom = av_channel_layout_custom_new(nb_channels);

No difference really.


> And what about the buffer being writable or not? You need to consider 
> both copy and ref scenarios.

av_channel_layout_copy() can simply ref. If a deep copy is needed because 
the user wants to change anything in AVChannelCustom, then helper function 
can be added.

>
> It's a lot of added complexity for what should be a simple "this stream with 
> six channels has the channels arranged like this in your room: This is front 
> left, this is front right, this is 'under the carpet', etc".

See the attached proof-of-concept patch, to be used on top of your 
channel_layout branch. Is it that bad?

Thanks,
Marton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch; name=channellayout-bufferref-poc.patch, Size: 12123 bytes --]

diff --git a/libavcodec/libfdk-aacdec.c b/libavcodec/libfdk-aacdec.c
index 71ee727ce1..1a38aefff9 100644
--- a/libavcodec/libfdk-aacdec.c
+++ b/libavcodec/libfdk-aacdec.c
@@ -52,7 +52,7 @@ typedef struct FDKAACDecContext {
     uint8_t *anc_buffer;
     int conceal_method;
     int drc_level;
-    iMaMaMao[MaMaMant drc_boost;
+    int drc_boost;
     int drc_heavy;
     int drc_effect;
     int drc_cut;
diff --git a/libavcodec/libfdk-aacenc.c b/libavcodec/libfdk-aacenc.c
index 95acdd83a9..d43f45d9ee 100644
--- a/libavcodec/libfdk-aacenc.c
+++ b/libavcodec/libfdk-aacenc.c
@@ -493,9 +493,7 @@ const AVCodec ff_libfdk_aac_encoder = {
     .supported_samplerates = aac_sample_rates,
     .wrapper_name          = "libfdk",
 #if FF_API_OLD_CHANNEL_LAYOUT
-FF_DISABLE_DEPRECATION_WARNINGS
     .channel_layouts       = aac_channel_layout,
-FF_ENABLE_DEPRECATION_WARNINGS
 #endif
     .ch_layouts            = aac_ch_layouts,
 };
diff --git a/libavcodec/opus.c b/libavcodec/opus.c
index 4d89068025..4be562b8fb 100644
--- a/libavcodec/opus.c
+++ b/libavcodec/opus.c
@@ -403,16 +403,18 @@ av_cold int ff_opus_parse_extradata(AVCodecContext *avctx,
             if (channels == (ambisonic_order + 1) * (ambisonic_order + 1)) {
                 layout.order = AV_CHANNEL_ORDER_AMBISONIC;
             } else {
+                AVChannelCustom *custom;
                 layout.order = AV_CHANNEL_ORDER_CUSTOM;
-                layout.u.map = av_mallocz_array(channels, sizeof(*layout.u.map));
-                if (!layout.u.map) {
+                layout.custom = av_channel_layout_custom_new(channels);
+                if (!layout.custom) {
                     ret = AVERROR(ENOMEM);
                     goto fail;
                 }
+                custom = av_channel_layout_get_custom(&layout);
                 for (i = 0; i < channels - 2; i++)
-                    layout.u.map[i].id = AV_CHAN_AMBISONIC_BASE + i;
-                layout.u.map[channels - 2].id = AV_CHAN_FRONT_LEFT;
-                layout.u.map[channels - 1].id = AV_CHAN_FRONT_RIGHT;
+                    custom[i].id = AV_CHAN_AMBISONIC_BASE + i;
+                custom[channels - 2].id = AV_CHAN_FRONT_LEFT;
+                custom[channels - 1].id = AV_CHAN_FRONT_RIGHT;
             }
         } else {
             layout.order       = AV_CHANNEL_ORDER_UNSPEC;
diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
index a51af95fcf..f66f8d3620 100644
--- a/libavutil/channel_layout.c
+++ b/libavutil/channel_layout.c
@@ -445,6 +445,7 @@ int av_channel_layout_from_string(AVChannelLayout *channel_layout,
         channel_layout->nb_channels = (order + 1) * (order + 1);
 
         if (*endptr) {
+            AVChannelCustom *custom;
             int ret = av_channel_layout_from_string(&extra, endptr + 1);
             if (ret < 0)
                 return ret;
@@ -455,19 +456,18 @@ int av_channel_layout_from_string(AVChannelLayout *channel_layout,
             }
 
             channel_layout->order = AV_CHANNEL_ORDER_CUSTOM;
-            channel_layout->u.map =
-                av_mallocz_array(channel_layout->nb_channels + extra.nb_channels,
-                                sizeof(*channel_layout->u.map));
-            if (!channel_layout->u.map) {
+            channel_layout->custom = av_channel_layout_custom_new(channel_layout->nb_channels + extra.nb_channels);
+            if (!channel_layout->custom) {
                 av_channel_layout_uninit(&extra);
                 return AVERROR(ENOMEM);
             }
 
+            custom = av_channel_layout_get_custom(channel_layout);
             for (i = 0; i < channel_layout->nb_channels; i++)
-                channel_layout->u.map[i].id = AV_CHAN_AMBISONIC_BASE + i;
+                custom[i].id = AV_CHAN_AMBISONIC_BASE + i;
             for (i = 0; i < extra.nb_channels; i++) {
                 enum AVChannel ch = av_channel_layout_channel_from_index(&extra, i);
-                channel_layout->u.map[channel_layout->nb_channels + i].id = ch;
+                custom[channel_layout->nb_channels + i].id = ch;
             }
             channel_layout->nb_channels += extra.nb_channels;
             av_channel_layout_uninit(&extra);
@@ -479,22 +479,43 @@ int av_channel_layout_from_string(AVChannelLayout *channel_layout,
     return AVERROR_INVALIDDATA;
 }
 
+static void channel_layout_custom_free(void *opaque, uint8_t *data)
+{
+    AVChannelCustom *data_end = opaque;
+    for (AVChannelCustom *custom = (void *)data; custom < data_end; custom++)
+        av_dict_free(&custom->metadata);
+    av_free(data);
+}
+
 void av_channel_layout_uninit(AVChannelLayout *channel_layout)
 {
-    if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM)
-        av_freep(&channel_layout->u.map);
+    if (channel_layout->custom)
+        av_buffer_unref(&channel_layout->custom);
     memset(channel_layout, 0, sizeof(*channel_layout));
 }
 
+AVBufferRef *av_channel_layout_custom_new(int nb_channels)
+{
+    AVBufferRef *ref;
+    uint8_t *data = av_calloc(nb_channels, sizeof(AVChannelCustom));
+    if (!data)
+        return NULL;
+    ref = av_buffer_create(data, sizeof(AVChannelCustom) * nb_channels, channel_layout_custom_free, data + nb_channels * sizeof(AVChannelCustom), 0);
+    if (!ref) {
+        av_free(data);
+        return NULL;
+    }
+    return ref;
+}
+
 int av_channel_layout_copy(AVChannelLayout *dst, const AVChannelLayout *src)
 {
     av_channel_layout_uninit(dst);
     *dst = *src;
-    if (src->order == AV_CHANNEL_ORDER_CUSTOM) {
-        dst->u.map = av_malloc(src->nb_channels * sizeof(*dst->u.map));
-        if (!dst->u.map)
+    if (src->custom) {
+        dst->custom = av_buffer_ref(src->custom);
+        if (!dst->custom)
             return AVERROR(ENOMEM);
-        memcpy(dst->u.map, src->u.map, src->nb_channels * sizeof(*src->u.map));
     }
     return 0;
 }
@@ -508,7 +529,7 @@ int av_channel_layout_copy(AVChannelLayout *dst, const AVChannelLayout *src)
  */
 static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_layout)
 {
-    const AVChannelCustom *map = channel_layout->u.map;
+    const AVChannelCustom *map = av_channel_layout_get_custom(channel_layout);
     int i, highest_ambi, order;
 
     highest_ambi = -1;
@@ -540,16 +561,18 @@ static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_l
     /* extra channels present */
     if (highest_ambi < channel_layout->nb_channels - 1) {
         AVChannelLayout extra;
+        AVChannelCustom *custom;
         char buf[128];
 
         extra.order       = AV_CHANNEL_ORDER_CUSTOM;
         extra.nb_channels = channel_layout->nb_channels - highest_ambi - 1;
-        extra.u.map       = av_mallocz_array(extra.nb_channels, sizeof(*extra.u.map));
-        if (!extra.u.map)
+        extra.custom      = av_channel_layout_custom_new(extra.nb_channels);
+        if (!extra.custom)
             return AVERROR(ENOMEM);
 
+        custom = av_channel_layout_get_custom(&extra);
         for (i = 0; i < extra.nb_channels; i++)
-            extra.u.map[i].id = map[highest_ambi + 1 + i].id;
+            custom[i].id = map[highest_ambi + 1 + i].id;
 
         av_channel_layout_describe(&extra, buf, sizeof(buf));
         av_channel_layout_uninit(&extra);
@@ -619,7 +642,7 @@ av_channel_layout_channel_from_index(const AVChannelLayout *channel_layout,
 
     switch (channel_layout->order) {
     case AV_CHANNEL_ORDER_CUSTOM:
-        return channel_layout->u.map[idx].id;
+        return av_channel_layout_get_custom(channel_layout)[idx].id;
     case AV_CHANNEL_ORDER_AMBISONIC:
         return AV_CHAN_AMBISONIC_BASE + idx;
     case AV_CHANNEL_ORDER_NATIVE:
@@ -658,11 +681,13 @@ int av_channel_layout_index_from_channel(const AVChannelLayout *channel_layout,
                                          enum AVChannel channel)
 {
     int i;
+    AVChannelCustom *custom;
 
     switch (channel_layout->order) {
     case AV_CHANNEL_ORDER_CUSTOM:
+        custom = av_channel_layout_get_custom(channel_layout);
         for (i = 0; i < channel_layout->nb_channels; i++)
-            if (channel_layout->u.map[i].id == channel)
+            if (custom[i].id == channel)
                 return i;
         return AVERROR(EINVAL);
     case AV_CHANNEL_ORDER_AMBISONIC:
@@ -705,7 +730,7 @@ int av_channel_layout_check(const AVChannelLayout *channel_layout)
     case AV_CHANNEL_ORDER_NATIVE:
         return av_popcount64(channel_layout->u.mask) == channel_layout->nb_channels;
     case AV_CHANNEL_ORDER_CUSTOM:
-        return !!channel_layout->u.map;
+        return !!channel_layout->custom;
     case AV_CHANNEL_ORDER_UNSPEC:
         return 1;
     default:
diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
index 7b77a74b61..ed7688a197 100644
--- a/libavutil/channel_layout.h
+++ b/libavutil/channel_layout.h
@@ -27,6 +27,8 @@
 
 #include "version.h"
 #include "attributes.h"
+#include "buffer.h"
+#include "dict.h"
 
 /**
  * @file
@@ -253,6 +255,7 @@ enum AVMatrixEncoding {
  */
 typedef struct AVChannelCustom {
     enum AVChannel id;
+    AVDictionary *metadata;
 } AVChannelCustom;
 
 /**
@@ -317,22 +320,14 @@ typedef struct AVChannelLayout {
          * is equal to nb_channels.
          */
         uint64_t mask;
-        /**
-         * This member must be used when the channel order is
-         * AV_CHANNEL_ORDER_CUSTOM. It is a nb_channels-sized array, with each
-         * element signalling the presence of the AVChannel with the
-         * corresponding value in map[i].id.
-         *
-         * I.e. when map[i].id is equal to AV_CHAN_FOO, then AV_CH_FOO is the
-         * i-th channel in the audio data.
-         *
-         * When map[i].id is in the range between AV_CHAN_AMBISONIC_BASE and
-         * AV_CHAN_AMBISONIC_END (inclusive), the channel contains an ambisonic
-         * component with ACN index (as defined above)
-         * n = map[i].id - AV_CHAN_AMBISONIC_BASE.
-         */
-        AVChannelCustom *map;
     } u;
+    /**
+     * The AVBufferRef which refcounts an array of AVChannelCustom values.
+     * This member must be used an cannot be NULL when channel order is
+     * AV_CHANNEL_ORDER_CUSTOM. Otherwise it is optional, can be used
+     * to store additional channel metadata.
+     */
+    AVBufferRef *custom;
 } AVChannelLayout;
 
 #define AV_CHANNEL_LAYOUT_MONO \
@@ -396,6 +391,13 @@ typedef struct AVChannelLayout {
 #define AV_CHANNEL_LAYOUT_AMBISONIC_FIRST_ORDER \
     { .order = AV_CHANNEL_ORDER_AMBISONIC, .nb_channels = 4, .u = { .mask = 0 }}
 
+AVBufferRef *av_channel_layout_custom_new(int nb_channels);
+
+static inline AVChannelCustom *av_channel_layout_get_custom(const AVChannelLayout *layout)
+{
+    return (void *)layout->custom->data;
+}
+
 #if FF_API_OLD_CHANNEL_LAYOUT
 /**
  * Return a channel layout id that matches name, or 0 if no match is found.
diff --git a/libavutil/tests/channel_layout.c b/libavutil/tests/channel_layout.c
index e4b42b1574..a25b496060 100644
--- a/libavutil/tests/channel_layout.c
+++ b/libavutil/tests/channel_layout.c
@@ -177,12 +177,12 @@ int main(void)
 
     custom.order = AV_CHANNEL_ORDER_CUSTOM;
     custom.nb_channels = 3;
-    custom.u.map = av_mallocz_array(3, sizeof(*custom.u.map));
-    if (!custom.u.map)
+    custom.custom = av_channel_layout_custom_new(3);
+    if (!custom.custom)
         return 1;
-    custom.u.map[0].id = AV_CHAN_FRONT_RIGHT;
-    custom.u.map[1].id = AV_CHAN_FRONT_LEFT;
-    custom.u.map[2].id = 63;
+    av_channel_layout_get_custom(&custom)[0].id = AV_CHAN_FRONT_RIGHT;
+    av_channel_layout_get_custom(&custom)[1].id = AV_CHAN_FRONT_LEFT;
+    av_channel_layout_get_custom(&custom)[2].id = 63;
     buf[0] = 0;
     printf("\nTesting av_channel_layout_describe\n");
     av_channel_layout_describe(&custom, buf, sizeof(buf));

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

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

Re: [FFmpeg-devel] [PATCH 001/279] Add a new channel layout API

[James Almer]

On 12/15/2021 6:32 PM, Marton Balint wrote:
> 
> 
> On Wed, 15 Dec 2021, James Almer wrote:
> 
>>
>>
>> On 12/15/2021 7:24 AM, Marton Balint wrote:
>>>
>>>
>>>  On Tue, 14 Dec 2021, James Almer wrote:
>>>
>>>>  On 12/14/2021 3:54 PM, Nicolas George wrote:
>>>>>   James Almer (12021-12-14):
>>>>>>   We add a const uint8_t* field to AVChannelCustom. If the user 
>>>>>> wants to
>>>>>>   allocate the strings instead, and not worry about their 
>>>>>> lifetime, they
>>>>>>   can
>>>>>>   provide the layout with a custom free() function that will take 
>>>>>> care
>>>>>>  of
>>>>>>   cleaning anything they came up with on uninit().
>>>>>
>>>>>   I understood what you suggested, and it amounts to letting API users
>>>>>   fend for themselves. We can do that, but I would prefer if we 
>>>>> only did
>>>>>   on last resort, if we do not find a more convenient solution.
>>>>
>>>>  There's "char name[16]". Bigger than a pointer (Could be 8 bytes 
>>>> instead,
>>>>  but then it's kinda small). The user will not have to worry about the
>>>>  lifetime of anything then.
>>>>
>>>>  I'm attaching a diff that goes on top of the last patch of this set
>>>>  implementing this. It also adds support for these custom names to
>>>>  av_channel_layout_describe(), av_channel_layout_index_from_string() 
>>>> and
>>>>  av_channel_layout_channel_from_string(), including tests.
>>>
>>>  I'd rather not mix custom labels with our fixed names for channels. 
>>> E.g.
>>>  what if a label conflicts with our existing channel names? If the user
>>>  wants to specify a channel based on its label, that should be a 
>>> separate
>>>  syntax IMHO.
>>>
>>>  Overall, having a char name[16] is still kind of limiting, e.g. a label
>>>  read from a muxed file will be truncated, I'd rather not have anything.
>>
>> Container metadata is typically be exported as stream metadata or side 
>> data.
> 
> That is always a possibility, but if you want some data per-channel, and 
> not per-stream, (e.g. variable size labels) then side data becomes 
> difficult to work with.
> 
>>
>>>
>>>  Here is one more idea, kind of a mix of what I read so far: Let's 
>>> refcount
>>>  only the dynamically allocated part of AVChannelLayout. Something like:
>>>
>>>  typedef struct AVChannelLayout {
>>>       enum AVChannelOrder order;
>>>       int nb_channels;
>>>       uint64_t mask;
>>>       AVBufferRef *custom;
>>>  } AVChannelLayout;
>>>
>>>  And the reference counted data could point to an array of 
>>> AVChannelCustom
>>>  entries. And AVChannelCustom can have AVDictionary *metadata, 
>>> because it
>>>  is refcounted.
>>
>> AVBufferRef is meant to hold flat arrays, though.
> 
> Since sizeof(AVChannelCustom) is fixed, I don't really see the difference.

A flat array of bytes. If you do a copy of the contents of the buffer 
(av_buffer_make_writable), and there's a pointer in it, you're copying 
it but not what it points to. The copy is not a reference, so when the 
last actual reference is freed, the custom free() function is called, it 
frees the dictionary, and the copy now has a dangling pointer to a non 
existent dictionary.

It's the reason we used to have split side data to handle non flat array 
structures.

> 
>> the custom channel names? As a fixed array like in the above, or in 
>> the dictionary? The latter sounds like a nightmare for both the 
>> helpers and as an API user.
> 
> Personally I think it can be put in the metadata dictionary, because if 
> we work out some syntax to select channel based on their labels, then it 
> also make sense to select channels based on other metadata, and syntax 
> might be extended with it. E.g. FL.label.Oboe. or FL.language.eng.
> 
> But if for some reason that is frowned upon, we can extend 
> AVChannelCustom with a char *label, and free it as well on free().

Same situation as above. A fixed array would be ok, though.

> 
> 
>> Also, since AVChannelCustom would have a dictionary, the AVBufferRef 
>> holding it needs a custom free() function. This makes creating layouts 
>> manually a pain.
> 
> You are already initalizing the dynamically allocated part of 
> AVChannelLayout:
> 
> layout.u.map = av_mallocz_array(nb_channels, sizeof());
> 
> If AVBufferRef is used for that, then a helper function can be added:
> 
> layout.custom = av_channel_layout_custom_new(nb_channels);
> 
> No difference really.
> 
> 
>> And what about the buffer being writable or not? You need to consider 
>> both copy and ref scenarios.
> 
> av_channel_layout_copy() can simply ref. If a deep copy is needed 
> because the user wants to change anything in AVChannelCustom, then 
> helper function can be added.
> 
>>
>> It's a lot of added complexity for what should be a simple "this 
>> stream with six channels has the channels arranged like this in your 
>> room: This is front left, this is front right, this is 'under the 
>> carpet', etc".
> 
> See the attached proof-of-concept patch, to be used on top of your 
> channel_layout branch. Is it that bad?

It's fragile by misusing the AVBufferRef API. Also, why remove the 
pointer from the union?

I'll send a version with a flat name array plus an opaque pointer. Then 
an email listing all the proposed solutions, to be discussed in a call 
where we hopefully reach an agreement.

> 
> Thanks,
> Marton
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 001/279] Add a new channel layout API

[Marton Balint]

On Wed, 15 Dec 2021, James Almer wrote:

>
>
> On 12/15/2021 6:32 PM, Marton Balint wrote:
>>
>>
>>  On Wed, 15 Dec 2021, James Almer wrote:
>> 
>>> 
>>>
>>>  On 12/15/2021 7:24 AM, Marton Balint wrote:
>>>> 
>>>>
>>>>   On Tue, 14 Dec 2021, James Almer wrote:
>>>>
>>>>>   On 12/14/2021 3:54 PM, Nicolas George wrote:
>>>>>>    James Almer (12021-12-14):
>>>>>>>    We add a const uint8_t* field to AVChannelCustom. If the user wants
>>>>>>>  to
>>>>>>>    allocate the strings instead, and not worry about their lifetime,
>>>>>>>  they
>>>>>>>    can
>>>>>>>    provide the layout with a custom free() function that will take
>>>>>>>  care
>>>>>>>   of
>>>>>>>    cleaning anything they came up with on uninit().
>>>>>>
>>>>>>    I understood what you suggested, and it amounts to letting API users
>>>>>>    fend for themselves. We can do that, but I would prefer if we only
>>>>>>  did
>>>>>>    on last resort, if we do not find a more convenient solution.
>>>>>
>>>>>   There's "char name[16]". Bigger than a pointer (Could be 8 bytes
>>>>>  instead,
>>>>>   but then it's kinda small). The user will not have to worry about the
>>>>>   lifetime of anything then.
>>>>>
>>>>>   I'm attaching a diff that goes on top of the last patch of this set
>>>>>   implementing this. It also adds support for these custom names to
>>>>>   av_channel_layout_describe(), av_channel_layout_index_from_string()
>>>>>  and
>>>>>   av_channel_layout_channel_from_string(), including tests.
>>>>
>>>>   I'd rather not mix custom labels with our fixed names for channels.
>>>>  E.g.
>>>>   what if a label conflicts with our existing channel names? If the user
>>>>   wants to specify a channel based on its label, that should be a
>>>>  separate
>>>>   syntax IMHO.
>>>>
>>>>   Overall, having a char name[16] is still kind of limiting, e.g. a label
>>>>   read from a muxed file will be truncated, I'd rather not have anything.
>>>
>>>  Container metadata is typically be exported as stream metadata or side
>>>  data.
>>
>>  That is always a possibility, but if you want some data per-channel, and
>>  not per-stream, (e.g. variable size labels) then side data becomes
>>  difficult to work with.
>> 
>>> 
>>>>
>>>>   Here is one more idea, kind of a mix of what I read so far: Let's
>>>>  refcount
>>>>   only the dynamically allocated part of AVChannelLayout. Something like:
>>>>
>>>>   typedef struct AVChannelLayout {
>>>>        enum AVChannelOrder order;
>>>>        int nb_channels;
>>>>        uint64_t mask;
>>>>        AVBufferRef *custom;
>>>>   } AVChannelLayout;
>>>>
>>>>   And the reference counted data could point to an array of
>>>>  AVChannelCustom
>>>>   entries. And AVChannelCustom can have AVDictionary *metadata, because
>>>>  it
>>>>   is refcounted.
>>>
>>>  AVBufferRef is meant to hold flat arrays, though.
>>
>>  Since sizeof(AVChannelCustom) is fixed, I don't really see the difference.
>
> A flat array of bytes. If you do a copy of the contents of the buffer 
> (av_buffer_make_writable), and there's a pointer in it, you're copying it but 
> not what it points to. The copy is not a reference, so when the last actual 
> reference is freed, the custom free() function is called, it frees the 
> dictionary, and the copy now has a dangling pointer to a non existent 
> dictionary.

OK, I understand. Wrapped avframe misuses AVBufferRef similarly however, 
and that also should be fixed then sometime... Maybe with an optional 
copy() function for AVBuffer?

>>>  And what about the buffer being writable or not? You need to consider
>>>  both copy and ref scenarios.
>>
>>  av_channel_layout_copy() can simply ref. If a deep copy is needed because
>>  the user wants to change anything in AVChannelCustom, then helper function
>>  can be added.
>> 
>>>
>>>  It's a lot of added complexity for what should be a simple "this stream
>>>  with six channels has the channels arranged like this in your room: This
>>>  is front left, this is front right, this is 'under the carpet', etc".
>>
>>  See the attached proof-of-concept patch, to be used on top of your
>>  channel_layout branch. Is it that bad?
>
> It's fragile by misusing the AVBufferRef API. Also, why remove the pointer 
> from the union?

Because you may want additional metadata without having a custom 
channel layout.

>
> I'll send a version with a flat name array plus an opaque pointer. Then an 
> email listing all the proposed solutions, to be discussed in a call where we 
> hopefully reach an agreement.

OK.

Thanks,
Marton
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 001/279] Add a new channel layout API

[James Almer]

On 12/15/2021 7:55 PM, Marton Balint wrote:
> 
> 
> On Wed, 15 Dec 2021, James Almer wrote:
> 
>>
>>
>> On 12/15/2021 6:32 PM, Marton Balint wrote:
>>>
>>>
>>>  On Wed, 15 Dec 2021, James Almer wrote:
>>>
>>>>
>>>>
>>>>  On 12/15/2021 7:24 AM, Marton Balint wrote:
>>>>>
>>>>>
>>>>>   On Tue, 14 Dec 2021, James Almer wrote:
>>>>>
>>>>>>   On 12/14/2021 3:54 PM, Nicolas George wrote:
>>>>>>>    James Almer (12021-12-14):
>>>>>>>>    We add a const uint8_t* field to AVChannelCustom. If the user 
>>>>>>>> wants
>>>>>>>>  to
>>>>>>>>    allocate the strings instead, and not worry about their 
>>>>>>>> lifetime,
>>>>>>>>  they
>>>>>>>>    can
>>>>>>>>    provide the layout with a custom free() function that will take
>>>>>>>>  care
>>>>>>>>   of
>>>>>>>>    cleaning anything they came up with on uninit().
>>>>>>>
>>>>>>>    I understood what you suggested, and it amounts to letting API 
>>>>>>> users
>>>>>>>    fend for themselves. We can do that, but I would prefer if we 
>>>>>>> only
>>>>>>>  did
>>>>>>>    on last resort, if we do not find a more convenient solution.
>>>>>>
>>>>>>   There's "char name[16]". Bigger than a pointer (Could be 8 bytes
>>>>>>  instead,
>>>>>>   but then it's kinda small). The user will not have to worry 
>>>>>> about the
>>>>>>   lifetime of anything then.
>>>>>>
>>>>>>   I'm attaching a diff that goes on top of the last patch of this set
>>>>>>   implementing this. It also adds support for these custom names to
>>>>>>   av_channel_layout_describe(), av_channel_layout_index_from_string()
>>>>>>  and
>>>>>>   av_channel_layout_channel_from_string(), including tests.
>>>>>
>>>>>   I'd rather not mix custom labels with our fixed names for channels.
>>>>>  E.g.
>>>>>   what if a label conflicts with our existing channel names? If the 
>>>>> user
>>>>>   wants to specify a channel based on its label, that should be a
>>>>>  separate
>>>>>   syntax IMHO.
>>>>>
>>>>>   Overall, having a char name[16] is still kind of limiting, e.g. a 
>>>>> label
>>>>>   read from a muxed file will be truncated, I'd rather not have 
>>>>> anything.
>>>>
>>>>  Container metadata is typically be exported as stream metadata or side
>>>>  data.
>>>
>>>  That is always a possibility, but if you want some data per-channel, 
>>> and
>>>  not per-stream, (e.g. variable size labels) then side data becomes
>>>  difficult to work with.
>>>
>>>>
>>>>>
>>>>>   Here is one more idea, kind of a mix of what I read so far: Let's
>>>>>  refcount
>>>>>   only the dynamically allocated part of AVChannelLayout. Something 
>>>>> like:
>>>>>
>>>>>   typedef struct AVChannelLayout {
>>>>>        enum AVChannelOrder order;
>>>>>        int nb_channels;
>>>>>        uint64_t mask;
>>>>>        AVBufferRef *custom;
>>>>>   } AVChannelLayout;
>>>>>
>>>>>   And the reference counted data could point to an array of
>>>>>  AVChannelCustom
>>>>>   entries. And AVChannelCustom can have AVDictionary *metadata, 
>>>>> because
>>>>>  it
>>>>>   is refcounted.
>>>>
>>>>  AVBufferRef is meant to hold flat arrays, though.
>>>
>>>  Since sizeof(AVChannelCustom) is fixed, I don't really see the 
>>> difference.
>>
>> A flat array of bytes. If you do a copy of the contents of the buffer 
>> (av_buffer_make_writable), and there's a pointer in it, you're copying 
>> it but not what it points to. The copy is not a reference, so when the 
>> last actual reference is freed, the custom free() function is called, 
>> it frees the dictionary, and the copy now has a dangling pointer to a 
>> non existent dictionary.
> 
> OK, I understand. Wrapped avframe misuses AVBufferRef similarly however, 
> and that also should be fixed then sometime... Maybe with an optional 
> copy() function for AVBuffer?

Wrapped avframe is an abomination i tried to fix but my patch was rejected.

I also tried to add a copy() callback to AVBufferRef some time ago, and 
it was also rejected, as the API was meant to be for flat arrays and it 
simply became the defactor refcounter because there was nothing else.
So other uses should be done with a different, more adequate and 
extensible kind of refcounting API. Anton and Nicolas both suggested two 
different approaches for it.

> 
>>>>  And what about the buffer being writable or not? You need to consider
>>>>  both copy and ref scenarios.
>>>
>>>  av_channel_layout_copy() can simply ref. If a deep copy is needed 
>>> because
>>>  the user wants to change anything in AVChannelCustom, then helper 
>>> function
>>>  can be added.
>>>
>>>>
>>>>  It's a lot of added complexity for what should be a simple "this 
>>>> stream
>>>>  with six channels has the channels arranged like this in your room: 
>>>> This
>>>>  is front left, this is front right, this is 'under the carpet', etc".
>>>
>>>  See the attached proof-of-concept patch, to be used on top of your
>>>  channel_layout branch. Is it that bad?
>>
>> It's fragile by misusing the AVBufferRef API. Also, why remove the 
>> pointer from the union?
> 
> Because you may want additional metadata without having a custom channel 
> layout.

Then it's no longer a struct meant for the Custom order. If you allow 
using it for native, unspec or ambisonic, what will the id field 
represent? If you say "make it be undefined", then it becomes wasted space.

> 
>>
>> I'll send a version with a flat name array plus an opaque pointer. 
>> Then an email listing all the proposed solutions, to be discussed in a 
>> call where we hopefully reach an agreement.
> 
> OK.
> 
> Thanks,
> Marton
> _______________________________________________
> 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".
_______________________________________________
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".