| Message ID | 20220315203013.14304-1-cus@passwd.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avutil/channel_layout: print channels using av_channel_name_bprint in av_channel_layout_describe_bprint | 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 3/15/2022 5:30 PM, Marton Balint wrote: > This reduces code duplication an allows printing AMBI%d channel names for > custom layouts for non-standard or partial ambisonic layouts. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/channel_layout.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c > index 8cc4efe4cf..c60ccf368f 100644 > --- a/libavutil/channel_layout.c > +++ b/libavutil/channel_layout.c > @@ -737,14 +737,10 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout, > av_bprintf(bp, "%d channels (", channel_layout->nb_channels); > for (i = 0; i < channel_layout->nb_channels; i++) { > enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i); > - const char *channel = get_channel_name(ch); > > if (i) > av_bprintf(bp, "+"); > - if (channel) > - av_bprintf(bp, "%s", channel); > - else > - av_bprintf(bp, "USR%d", ch); > + av_channel_name_bprint(bp, ch); > if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM && > channel_layout->u.map[i].name[0]) > av_bprintf(bp, "@%s", channel_layout->u.map[i].name); Should be ok.
On 3/15/2022 6:18 PM, Marton Balint wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/channel_layout.c | 42 +++++++++++++++++++++++++++----------- > libavutil/channel_layout.h | 1 + > 2 files changed, 31 insertions(+), 12 deletions(-) > > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c > index c60ccf368f..0069c6257b 100644 > --- a/libavutil/channel_layout.c > +++ b/libavutil/channel_layout.c > @@ -644,29 +644,31 @@ int av_channel_layout_copy(AVChannelLayout *dst, const AVChannelLayout *src) > } > > /** > - * If the custom layout is n-th order standard-order ambisonic, with optional > - * extra non-diegetic channels at the end, write its string description in bp. > - * Return a negative error code on error. > + * If the layout is n-th order standard-order ambisonic, with optional > + * extra non-diegetic channels at the end, return the order. > + * Return a negative error code otherwise. > */ > -static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_layout) > +static int ambisonic_order(const AVChannelLayout *channel_layout) > { > int i, highest_ambi, order; > > highest_ambi = -1; > - if (channel_layout->order == AV_CHANNEL_ORDER_AMBISONIC) > + if (channel_layout->order == AV_CHANNEL_ORDER_AMBISONIC) { > highest_ambi = channel_layout->nb_channels - av_popcount64(channel_layout->u.mask) - 1; > - else { > + } else { nit: Remove these brackets since it's still a single line after the if statement. They bloat the diff for no gain. > const AVChannelCustom *map = channel_layout->u.map; > + av_assert0(channel_layout->order == AV_CHANNEL_ORDER_CUSTOM); > + > for (i = 0; i < channel_layout->nb_channels; i++) { > int is_ambi = CHAN_IS_AMBI(map[i].id); > > /* ambisonic following non-ambisonic */ > if (i > 0 && is_ambi && !CHAN_IS_AMBI(map[i - 1].id)) > - return 0; > + return AVERROR(EINVAL); > > /* non-default ordering */ > if (is_ambi && map[i].id - AV_CHAN_AMBISONIC_BASE != i) > - return 0; > + return AVERROR(EINVAL); > > if (CHAN_IS_AMBI(map[i].id)) > highest_ambi = i; > @@ -674,17 +676,33 @@ static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_l > } > /* no ambisonic channels*/ > if (highest_ambi < 0) > - return 0; > + return AVERROR(EINVAL); > > order = floor(sqrt(highest_ambi)); > /* incomplete order - some harmonics are missing */ > if ((order + 1) * (order + 1) != highest_ambi + 1) > + return AVERROR(EINVAL); > + > + return order; > +} > + > +/** > + * If the custom layout is n-th order standard-order ambisonic, with optional > + * extra non-diegetic channels at the end, write its string description in bp. > + * Return a negative error code on error. > + */ > +static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_layout) > +{ > + int nb_ambi_channels; > + int order = ambisonic_order(channel_layout); > + if (order < 0) > return 0; > > av_bprintf(bp, "ambisonic %d", order); > > /* extra channels present */ > - if (highest_ambi < channel_layout->nb_channels - 1) { > + nb_ambi_channels = (order + 1) * (order + 1); > + if (nb_ambi_channels < channel_layout->nb_channels) { > AVChannelLayout extra = { 0 }; > char buf[128]; > > @@ -696,12 +714,12 @@ static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_l > const AVChannelCustom *map = channel_layout->u.map; > > extra.order = AV_CHANNEL_ORDER_CUSTOM; > - extra.nb_channels = channel_layout->nb_channels - highest_ambi - 1; > + extra.nb_channels = channel_layout->nb_channels - nb_ambi_channels; > extra.u.map = av_calloc(extra.nb_channels, sizeof(*extra.u.map)); > if (!extra.u.map) > return AVERROR(ENOMEM); > > - memcpy(extra.u.map, &map[highest_ambi + 1], > + memcpy(extra.u.map, &map[nb_ambi_channels], > sizeof(*extra.u.map) * extra.nb_channels); > } > > diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h > index 4dd6614de9..294e8b0773 100644 > --- a/libavutil/channel_layout.h > +++ b/libavutil/channel_layout.h > @@ -27,6 +27,7 @@ > > #include "version.h" > #include "attributes.h" > +#include "avassert.h" Nothing in the header uses it. It'll become an unnecessary dependency every user of this header will have to deal with, so add it to channel_layout.c instead. > > /** > * @file LGTM with the above two changes.
On 3/15/2022 6:18 PM, Marton Balint wrote: > Also use av_channel_layout_bprint directly for describing channel layout for > extra channels. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavutil/channel_layout.c | 17 ++++------------- > 1 file changed, 4 insertions(+), 13 deletions(-) > > diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c > index 0069c6257b..fb1f72737f 100644 > --- a/libavutil/channel_layout.c > +++ b/libavutil/channel_layout.c > @@ -704,29 +704,20 @@ static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_l > nb_ambi_channels = (order + 1) * (order + 1); > if (nb_ambi_channels < channel_layout->nb_channels) { > AVChannelLayout extra = { 0 }; > - char buf[128]; > > if (channel_layout->order == AV_CHANNEL_ORDER_AMBISONIC) { > extra.order = AV_CHANNEL_ORDER_NATIVE; > extra.nb_channels = av_popcount64(channel_layout->u.mask); > extra.u.mask = channel_layout->u.mask; > } else { > - const AVChannelCustom *map = channel_layout->u.map; > - > extra.order = AV_CHANNEL_ORDER_CUSTOM; > extra.nb_channels = channel_layout->nb_channels - nb_ambi_channels; > - extra.u.map = av_calloc(extra.nb_channels, sizeof(*extra.u.map)); > - if (!extra.u.map) > - return AVERROR(ENOMEM); > - > - memcpy(extra.u.map, &map[nb_ambi_channels], > - sizeof(*extra.u.map) * extra.nb_channels); > + extra.u.map = channel_layout->u.map + nb_ambi_channels; > } > > - av_channel_layout_describe(&extra, buf, sizeof(buf)); > - av_channel_layout_uninit(&extra); > - > - av_bprintf(bp, "+%s", buf); > + av_bprint_chars(bp, '+', 1); > + av_channel_layout_describe_bprint(&extra, bp); > + /* Not calling uninit here on extra because we don't own the u.map pointer */ > } > > return 0; LGTM.
On Tue, 15 Mar 2022, James Almer wrote: > > > On 3/15/2022 6:18 PM, Marton Balint wrote: >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavutil/channel_layout.c | 42 +++++++++++++++++++++++++++----------- >> libavutil/channel_layout.h | 1 + >> 2 files changed, 31 insertions(+), 12 deletions(-) >> >> diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c >> index c60ccf368f..0069c6257b 100644 >> --- a/libavutil/channel_layout.c >> +++ b/libavutil/channel_layout.c >> @@ -644,29 +644,31 @@ int av_channel_layout_copy(AVChannelLayout *dst, >> const AVChannelLayout *src) >> } >> >> /** >> - * If the custom layout is n-th order standard-order ambisonic, with >> optional >> - * extra non-diegetic channels at the end, write its string description >> in bp. >> - * Return a negative error code on error. >> + * If the layout is n-th order standard-order ambisonic, with optional >> + * extra non-diegetic channels at the end, return the order. >> + * Return a negative error code otherwise. >> */ >> -static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout >> *channel_layout) >> +static int ambisonic_order(const AVChannelLayout *channel_layout) >> { >> int i, highest_ambi, order; >> >> highest_ambi = -1; >> - if (channel_layout->order == AV_CHANNEL_ORDER_AMBISONIC) >> + if (channel_layout->order == AV_CHANNEL_ORDER_AMBISONIC) { >> highest_ambi = channel_layout->nb_channels - >> av_popcount64(channel_layout->u.mask) - 1; >> - else { >> + } else { > > nit: Remove these brackets since it's still a single line after the if > statement. They bloat the diff for no gain. Ok. > >> const AVChannelCustom *map = channel_layout->u.map; >> + av_assert0(channel_layout->order == AV_CHANNEL_ORDER_CUSTOM); >> + >> for (i = 0; i < channel_layout->nb_channels; i++) { >> int is_ambi = CHAN_IS_AMBI(map[i].id); >> >> /* ambisonic following non-ambisonic */ >> if (i > 0 && is_ambi && !CHAN_IS_AMBI(map[i - 1].id)) >> - return 0; >> + return AVERROR(EINVAL); >> >> /* non-default ordering */ >> if (is_ambi && map[i].id - AV_CHAN_AMBISONIC_BASE != i) >> - return 0; >> + return AVERROR(EINVAL); >> >> if (CHAN_IS_AMBI(map[i].id)) >> highest_ambi = i; >> @@ -674,17 +676,33 @@ static int try_describe_ambisonic(AVBPrint *bp, >> const AVChannelLayout *channel_l >> } >> /* no ambisonic channels*/ >> if (highest_ambi < 0) >> - return 0; >> + return AVERROR(EINVAL); >> >> order = floor(sqrt(highest_ambi)); >> /* incomplete order - some harmonics are missing */ >> if ((order + 1) * (order + 1) != highest_ambi + 1) >> + return AVERROR(EINVAL); >> + >> + return order; >> +} >> + >> +/** >> + * If the custom layout is n-th order standard-order ambisonic, with >> optional >> + * extra non-diegetic channels at the end, write its string description >> in bp. >> + * Return a negative error code on error. >> + */ >> +static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout >> *channel_layout) >> +{ >> + int nb_ambi_channels; >> + int order = ambisonic_order(channel_layout); >> + if (order < 0) >> return 0; >> >> av_bprintf(bp, "ambisonic %d", order); >> >> /* extra channels present */ >> - if (highest_ambi < channel_layout->nb_channels - 1) { >> + nb_ambi_channels = (order + 1) * (order + 1); >> + if (nb_ambi_channels < channel_layout->nb_channels) { >> AVChannelLayout extra = { 0 }; >> char buf[128]; >> @@ -696,12 +714,12 @@ static int try_describe_ambisonic(AVBPrint *bp, >> const AVChannelLayout *channel_l >> const AVChannelCustom *map = channel_layout->u.map; >> >> extra.order = AV_CHANNEL_ORDER_CUSTOM; >> - extra.nb_channels = channel_layout->nb_channels - >> highest_ambi - 1; >> + extra.nb_channels = channel_layout->nb_channels - >> nb_ambi_channels; >> extra.u.map = av_calloc(extra.nb_channels, >> sizeof(*extra.u.map)); >> if (!extra.u.map) >> return AVERROR(ENOMEM); >> - memcpy(extra.u.map, &map[highest_ambi + 1], >> + memcpy(extra.u.map, &map[nb_ambi_channels], >> sizeof(*extra.u.map) * extra.nb_channels); >> } >> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h >> index 4dd6614de9..294e8b0773 100644 >> --- a/libavutil/channel_layout.h >> +++ b/libavutil/channel_layout.h >> @@ -27,6 +27,7 @@ >> >> #include "version.h" >> #include "attributes.h" >> +#include "avassert.h" > > Nothing in the header uses it. It'll become an unnecessary dependency every > user of this header will have to deal with, so add it to channel_layout.c > instead. Ok, I have no idea why I added it to the header... :) Thanks, Marton
On Tue, 15 Mar 2022, James Almer wrote: > On 3/15/2022 6:18 PM, Marton Balint wrote: >> Also use av_channel_layout_bprint directly for describing channel layout >> for >> extra channels. >> >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> libavutil/channel_layout.c | 17 ++++------------- >> 1 file changed, 4 insertions(+), 13 deletions(-) >> >> diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c >> index 0069c6257b..fb1f72737f 100644 >> --- a/libavutil/channel_layout.c >> +++ b/libavutil/channel_layout.c >> @@ -704,29 +704,20 @@ static int try_describe_ambisonic(AVBPrint *bp, >> const AVChannelLayout *channel_l >> nb_ambi_channels = (order + 1) * (order + 1); >> if (nb_ambi_channels < channel_layout->nb_channels) { >> AVChannelLayout extra = { 0 }; >> - char buf[128]; >> >> if (channel_layout->order == AV_CHANNEL_ORDER_AMBISONIC) { >> extra.order = AV_CHANNEL_ORDER_NATIVE; >> extra.nb_channels = av_popcount64(channel_layout->u.mask); >> extra.u.mask = channel_layout->u.mask; >> } else { >> - const AVChannelCustom *map = channel_layout->u.map; >> - >> extra.order = AV_CHANNEL_ORDER_CUSTOM; >> extra.nb_channels = channel_layout->nb_channels - >> nb_ambi_channels; >> - extra.u.map = av_calloc(extra.nb_channels, >> sizeof(*extra.u.map)); >> - if (!extra.u.map) >> - return AVERROR(ENOMEM); >> - >> - memcpy(extra.u.map, &map[nb_ambi_channels], >> - sizeof(*extra.u.map) * extra.nb_channels); >> + extra.u.map = channel_layout->u.map + nb_ambi_channels; >> } >> - av_channel_layout_describe(&extra, buf, sizeof(buf)); >> - av_channel_layout_uninit(&extra); >> - >> - av_bprintf(bp, "+%s", buf); >> + av_bprint_chars(bp, '+', 1); >> + av_channel_layout_describe_bprint(&extra, bp); >> + /* Not calling uninit here on extra because we don't own the >> u.map pointer */ >> } >> >> return 0; > > LGTM. Thanks, applied the series (although in different order, because it turned out that this patch depends on the next). Thanks, Marton
diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c index 8cc4efe4cf..c60ccf368f 100644 --- a/libavutil/channel_layout.c +++ b/libavutil/channel_layout.c @@ -737,14 +737,10 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout, av_bprintf(bp, "%d channels (", channel_layout->nb_channels); for (i = 0; i < channel_layout->nb_channels; i++) { enum AVChannel ch = av_channel_layout_channel_from_index(channel_layout, i); - const char *channel = get_channel_name(ch); if (i) av_bprintf(bp, "+"); - if (channel) - av_bprintf(bp, "%s", channel); - else - av_bprintf(bp, "USR%d", ch); + av_channel_name_bprint(bp, ch); if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM && channel_layout->u.map[i].name[0]) av_bprintf(bp, "@%s", channel_layout->u.map[i].name);
This reduces code duplication an allows printing AMBI%d channel names for custom layouts for non-standard or partial ambisonic layouts. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavutil/channel_layout.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)