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@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
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
_______________________________________________
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".
