[FFmpeg-devel] [PATCH 001/279] Add a new channel layout API
James Almer
jamrial at gmail.com
Tue Dec 14 21:30:04 EET 2021
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.
>
>> Having something like this called on every copy sounds inefficient. And what
>
> What??? Ref-counting makes operations more efficient by avoiding new
> dynamic allocations.
>
> I grant you, the code currently tries to avoid dynamic allocations, but
> I do not think it is actually manageable without seriously limiting the
> features of the API. And you know me, I try to avoid dynamic allocations
> as much as possible.
>
>> will you refcount? The layout? Each channel's string? Can you be more
>> specific about how do you imagine implementing this?
>
> The whole layout structure, with all the possible custom layouts,
> dictionaries and strings.
>
> I do not think that sharing strings between layouts that are similar but
> not equal is worth the added complexity.
>
> (We may consider a ref-counted string type at some point, maybe, but
> that is another story.)
>
> Regards,
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
-------------- next part --------------
diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
index a51af95fcf..09f8fbe4b5 100644
--- a/libavutil/channel_layout.c
+++ b/libavutil/channel_layout.c
@@ -548,8 +548,8 @@ static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_l
if (!extra.u.map)
return AVERROR(ENOMEM);
- for (i = 0; i < extra.nb_channels; i++)
- extra.u.map[i].id = map[highest_ambi + 1 + i].id;
+ memcpy(extra.u.map, &map[highest_ambi + 1],
+ sizeof(*extra.u.map) * extra.nb_channels);
av_channel_layout_describe(&extra, buf, sizeof(buf));
av_channel_layout_uninit(&extra);
@@ -587,8 +587,15 @@ int av_channel_layout_describe(const AVChannelLayout *channel_layout,
}
for (i = 0; i < channel_layout->nb_channels; i++) {
- enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i);
- const char *ch_name = get_channel_name(ch);
+ const char *ch_name = NULL;
+
+ if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM &&
+ channel_layout->u.map[i].name[0])
+ ch_name = channel_layout->u.map[i].name;
+ if (channel_layout->order == AV_CHANNEL_ORDER_NATIVE || !ch_name) {
+ enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i);
+ ch_name = get_channel_name(ch);
+ }
if (i)
av_bprintf(&bp, "|");
@@ -641,6 +648,11 @@ av_channel_layout_channel_from_string(const AVChannelLayout *channel_layout,
switch (channel_layout->order) {
case AV_CHANNEL_ORDER_CUSTOM:
+ for (int i = 0; i < channel_layout->nb_channels; i++) {
+ if (channel_layout->u.map[i].name[0] && !strcmp(name, channel_layout->u.map[i].name))
+ return channel_layout->u.map[i].id;
+ }
+ // fall-through
case AV_CHANNEL_ORDER_NATIVE:
channel = av_channel_from_string(name);
if (channel < 0)
@@ -687,13 +699,22 @@ int av_channel_layout_index_from_string(const AVChannelLayout *channel_layout,
{
int ret;
- if (channel_layout->order == AV_CHANNEL_ORDER_UNSPEC)
- return AVERROR(EINVAL);
+ switch (channel_layout->order) {
+ case AV_CHANNEL_ORDER_CUSTOM:
+ for (int i = 0; i < channel_layout->nb_channels; i++) {
+ if (channel_layout->u.map[i].name[0] && !strcmp(name, channel_layout->u.map[i].name))
+ return i;
+ }
+ // fall-through
+ case AV_CHANNEL_ORDER_NATIVE:
+ case AV_CHANNEL_ORDER_AMBISONIC:
+ ret = av_channel_from_string(name);
+ if (ret < 0)
+ return ret;
+ return av_channel_layout_index_from_channel(channel_layout, ret);
+ }
- ret = av_channel_from_string(name);
- if (ret < 0)
- return ret;
- return av_channel_layout_index_from_channel(channel_layout, ret);
+ return AVERROR(EINVAL);
}
int av_channel_layout_check(const AVChannelLayout *channel_layout)
diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
index 7b77a74b61..996c389f5d 100644
--- a/libavutil/channel_layout.h
+++ b/libavutil/channel_layout.h
@@ -253,6 +253,7 @@ enum AVMatrixEncoding {
*/
typedef struct AVChannelCustom {
enum AVChannel id;
+ char name[16];
} AVChannelCustom;
/**
@@ -330,6 +331,10 @@ typedef struct AVChannelLayout {
* 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.
+ *
+ * map[i].name may be filled with a 0-terminated string, in which case
+ * it will be used for the purpose of identifying the channel with the
+ * convenience functions below. Otherise it must be zeroed.
*/
AVChannelCustom *map;
} u;
diff --git a/libavutil/tests/channel_layout.c b/libavutil/tests/channel_layout.c
index e4b42b1574..3c5b3c3320 100644
--- a/libavutil/tests/channel_layout.c
+++ b/libavutil/tests/channel_layout.c
@@ -183,6 +183,7 @@ int main(void)
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_strlcpy(custom.u.map[2].name, "Ch63", sizeof(custom.u.map[2].name));
buf[0] = 0;
printf("\nTesting av_channel_layout_describe\n");
av_channel_layout_describe(&custom, buf, sizeof(buf));
@@ -193,6 +194,8 @@ int main(void)
printf("On \"FR|FL|Ch63\" layout with \"FR\": %18d\n", ret);
CHANNEL_LAYOUT_INDEX_FROM_STRING(custom, "FL");
printf("On \"FR|FL|Ch63\" layout with \"FL\": %18d\n", ret);
+ CHANNEL_LAYOUT_INDEX_FROM_STRING(custom, "Ch63");
+ printf("On \"FR|FL|Ch63\" layout with \"Ch63\": %16d\n", ret);
CHANNEL_LAYOUT_INDEX_FROM_STRING(custom, "BC");
printf("On \"FR|FL|Ch63\" layout with \"BC\": %18d\n", ret);
@@ -201,6 +204,8 @@ int main(void)
printf("On \"FR|FL|Ch63\" layout with \"FR\": %18d\n", ret);
CHANNEL_LAYOUT_CHANNEL_FROM_STRING(custom, "FL");
printf("On \"FR|FL|Ch63\" layout with \"FL\": %18d\n", ret);
+ CHANNEL_LAYOUT_CHANNEL_FROM_STRING(custom, "Ch63");
+ printf("On \"FR|FL|Ch63\" layout with \"Ch63\": %16d\n", ret);
CHANNEL_LAYOUT_CHANNEL_FROM_STRING(custom, "BC");
printf("On \"FR|FL|Ch63\" layout with \"BC\": %18d\n", ret);
diff --git a/tests/ref/fate/channel_layout b/tests/ref/fate/channel_layout
index 1a74216125..a1cb9b7e04 100644
--- a/tests/ref/fate/channel_layout
+++ b/tests/ref/fate/channel_layout
@@ -69,16 +69,18 @@ On 5.1(side) layout with "BC": -1
==Custom layouts==
Testing av_channel_layout_describe
-On "FR|FL|Ch63" layout: FR|FL|?
+On "FR|FL|Ch63" layout: FR|FL|Ch63
Testing av_channel_layout_index_from_string
On "FR|FL|Ch63" layout with "FR": 0
On "FR|FL|Ch63" layout with "FL": 1
+On "FR|FL|Ch63" layout with "Ch63": 2
On "FR|FL|Ch63" layout with "BC": -1
Testing av_channel_layout_channel_from_string
On "FR|FL|Ch63" layout with "FR": 1
On "FR|FL|Ch63" layout with "FL": 0
+On "FR|FL|Ch63" layout with "Ch63": 63
On "FR|FL|Ch63" layout with "BC": -1
Testing av_channel_layout_index_from_channel
More information about the ffmpeg-devel
mailing list