[FFmpeg-devel] [PATCH 1/5] avutil/channel_layout: change AV_CHAN_SILENCE to AV_CHAN_UNUSED in the docs

[FFmpeg-devel] [PATCH 1/5] avutil/channel_layout: change AV_CHAN_SILENCE to AV_CHAN_UNUSED in the docs

[Marton Balint]

It got renamed during the API design phase.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavutil/channel_layout.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
index 7ee5333ea8..b8bff6f365 100644
--- a/libavutil/channel_layout.h
+++ b/libavutil/channel_layout.h
@@ -119,7 +119,7 @@ enum AVChannelOrder {
     /**
      * The channel order does not correspond to any other predefined order and
      * is stored as an explicit map. For example, this could be used to support
-     * layouts with 64 or more channels, or with empty/skipped (AV_CHAN_SILENCE)
+     * layouts with 64 or more channels, or with empty/skipped (AV_CHAN_UNUSED)
      * channels at arbitrary positions.
      */
     AV_CHANNEL_ORDER_CUSTOM,
-- 
2.35.3

_______________________________________________
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] [PATCH 2/5] avutil/channel_layout: add AV_CHANNEL_ORDER_NB

[Marton Balint]

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavutil/channel_layout.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
index b8bff6f365..db0c005e87 100644
--- a/libavutil/channel_layout.h
+++ b/libavutil/channel_layout.h
@@ -146,6 +146,10 @@ enum AVChannelOrder {
      * as defined in AmbiX format $ 2.1.
      */
     AV_CHANNEL_ORDER_AMBISONIC,
+    /**
+     * Number of channel orders, not part of ABI/API
+     */
+    AV_CHANNEL_ORDER_NB
 };
 
 
-- 
2.35.3

_______________________________________________
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] [PATCH 3/5] avutil/tests/channel_layout: add tests for av_channel_order_retype

[Marton Balint]

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavutil/tests/channel_layout.c | 63 ++++++++++++++++++++++++++++++++
 tests/ref/fate/channel_layout    | 27 ++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/libavutil/tests/channel_layout.c b/libavutil/tests/channel_layout.c
index c537e7e710..d83839700c 100644
--- a/libavutil/tests/channel_layout.c
+++ b/libavutil/tests/channel_layout.c
@@ -24,6 +24,7 @@
 
 #include "libavutil/bprint.h"
 #include "libavutil/channel_layout.h"
+#include "libavutil/error.h"
 #include "libavutil/internal.h"
 #include "libavutil/mem.h"
 
@@ -112,6 +113,53 @@ static void channel_layout_from_string(AVChannelLayout *layout,
         av_bprintf(bp, "fail");
 }
 
+static const char* channel_order_names[AV_CHANNEL_ORDER_NB]  = {"UNSPEC", "NATIVE", "CUSTOM", "AMBI"};
+
+static void describe_type(AVBPrint *bp, AVChannelLayout *layout)
+{
+    if (layout->order >= 0 && layout->order < AV_CHANNEL_ORDER_NB) {
+        av_bprintf(bp, "%-6s (", channel_order_names[layout->order]);
+        av_channel_layout_describe_bprint(layout, bp);
+        av_bprintf(bp, ")");
+    } else {
+        av_bprintf(bp, "???");
+    }
+}
+
+static const char *channel_layout_retype(AVChannelLayout *layout, AVBPrint *bp, const char *channel_layout)
+{
+    av_channel_layout_uninit(layout);
+    av_bprint_clear(bp);
+    if (!av_channel_layout_from_string(layout, channel_layout) &&
+        av_channel_layout_check(layout)) {
+        describe_type(bp, layout);
+        for (int i = 0; i < AV_CHANNEL_ORDER_NB; i++) {
+            int ret;
+            AVChannelLayout copy = {0};
+            av_bprintf(bp, "\n ");
+            if (av_channel_layout_copy(&copy, layout) < 0)
+                return "nomem";
+            ret = av_channel_layout_retype(&copy, i, 0);
+            if (ret < 0 && (copy.order != layout->order || av_channel_layout_compare(&copy, layout)))
+                av_bprintf(bp, "failed to keep existing layout on failure ");
+            if (ret >= 0 && copy.order != i)
+                av_bprintf(bp, "returned success but did not change order ");
+            if (ret == AVERROR(ENOSYS)) {
+                av_bprintf(bp, " != %s", channel_order_names[i]);
+            } else if (ret < 0) {
+                av_bprintf(bp, "FAIL");
+            } else {
+                av_bprintf(bp, " %s ", ret ? "~~" : "==");
+                describe_type(bp, &copy);
+            }
+            av_channel_layout_uninit(&copy);
+        }
+    } else {
+        av_bprintf(bp, "fail");
+    }
+    return bp->str;
+}
+
 #define CHANNEL_NAME(x)                                                    \
     channel_name(&bp, (x));
 
@@ -437,5 +485,20 @@ int main(void)
     av_channel_layout_uninit(&layout2);
     av_bprint_finalize(&bp, NULL);
 
+    printf("\nTesting av_channel_layout_retype\n");
+    {
+        const char* layouts[] = {
+            "FL@Boo",
+            "stereo",
+            "FR+FL",
+            "ambisonic 2+stereo",
+            "2C",
+            NULL
+        };
+        for (int i = 0; layouts[i]; i++) {
+            printf("With \"%s\": %s\n", layouts[i], channel_layout_retype(&layout, &bp, layouts[i]));
+        }
+    }
+
     return 0;
 }
diff --git a/tests/ref/fate/channel_layout b/tests/ref/fate/channel_layout
index ab9bee947b..1d1f1cb082 100644
--- a/tests/ref/fate/channel_layout
+++ b/tests/ref/fate/channel_layout
@@ -185,3 +185,30 @@ On "ambisonic 2+stereo" layout with 11:                    -1
 Testing av_channel_layout_subset
 On "ambisonic 2+stereo" layout with AV_CH_LAYOUT_STEREO:  0x3
 On "ambisonic 2+stereo" layout with AV_CH_LAYOUT_QUAD:    0x3
+
+Testing av_channel_layout_retype
+With "FL@Boo": CUSTOM (1 channels (FL@Boo))
+  ~~ UNSPEC (1 channels)
+  ~~ NATIVE (1 channels (FL))
+  == CUSTOM (1 channels (FL@Boo))
+  != AMBI
+With "stereo": NATIVE (stereo)
+  ~~ UNSPEC (2 channels)
+  == NATIVE (stereo)
+  == CUSTOM (2 channels (FL+FR))
+  != AMBI
+With "FR+FL": CUSTOM (2 channels (FR+FL))
+  ~~ UNSPEC (2 channels)
+  != NATIVE
+  == CUSTOM (2 channels (FR+FL))
+  != AMBI
+With "ambisonic 2+stereo": AMBI   (ambisonic 2+stereo)
+  ~~ UNSPEC (11 channels)
+  != NATIVE
+  == CUSTOM (ambisonic 2+2 channels (FL+FR))
+  == AMBI   (ambisonic 2+stereo)
+With "2C": UNSPEC (2 channels)
+  == UNSPEC (2 channels)
+  != NATIVE
+  == CUSTOM (2 channels (USR768+USR768))
+  != AMBI
-- 
2.35.3

_______________________________________________
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] [PATCH 4/5] avformat/mov: factorize reading the main part of the chnl atom to mov_chan

[Marton Balint]

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mov.c      | 58 +++--------------------------------------
 libavformat/mov_chan.c | 59 ++++++++++++++++++++++++++++++++++++++++++
 libavformat/mov_chan.h | 10 +++++++
 3 files changed, 73 insertions(+), 54 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 42b0135987..52436d71d6 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -953,9 +953,8 @@ static int mov_read_chan(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 static int mov_read_chnl(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
     int64_t end = av_sat_add64(avio_tell(pb), atom.size);
-    int stream_structure;
     int version, flags;
-    int ret = 0;
+    int ret;
     AVStream *st;
 
     if (c->fc->nb_streams < 1)
@@ -971,58 +970,9 @@ static int mov_read_chnl(MOVContext *c, AVIOContext *pb, MOVAtom atom)
         return AVERROR_INVALIDDATA;
     }
 
-    stream_structure = avio_r8(pb);
-
-    // stream carries channels
-    if (stream_structure & 1) {
-        int layout = avio_r8(pb);
-
-        av_log(c->fc, AV_LOG_TRACE, "'chnl' layout %d\n", layout);
-        if (!layout) {
-            uint8_t *positions = av_malloc(st->codecpar->ch_layout.nb_channels);
-
-            if (!positions)
-                return AVERROR(ENOMEM);
-            for (int i = 0; i < st->codecpar->ch_layout.nb_channels; i++) {
-                int speaker_pos = avio_r8(pb);
-
-                av_log(c->fc, AV_LOG_TRACE, "speaker_position %d\n", speaker_pos);
-                if (speaker_pos == 126) { // explicit position
-                    avpriv_request_sample(c->fc, "explicit position");
-                    av_freep(&positions);
-                    return AVERROR_PATCHWELCOME;
-                } else {
-                    positions[i] = speaker_pos;
-                }
-            }
-
-            ret = ff_mov_get_layout_from_channel_positions(positions,
-                    st->codecpar->ch_layout.nb_channels,
-                    &st->codecpar->ch_layout);
-            av_freep(&positions);
-            if (ret) {
-                av_log(c->fc, AV_LOG_ERROR,
-                        "get channel layout from speaker positions failed, %s\n",
-                        av_err2str(ret));
-                return ret;
-            }
-        } else {
-            uint64_t omitted_channel_map = avio_rb64(pb);
-
-            if (omitted_channel_map) {
-                avpriv_request_sample(c->fc, "omitted_channel_map 0x%" PRIx64 " != 0",
-                                      omitted_channel_map);
-                return AVERROR_PATCHWELCOME;
-            }
-            ff_mov_get_channel_layout_from_config(layout, &st->codecpar->ch_layout);
-        }
-    }
-
-    // stream carries objects
-    if (stream_structure & 2) {
-        int obj_count = avio_r8(pb);
-        av_log(c->fc, AV_LOG_TRACE, "'chnl' with object_count %d\n", obj_count);
-    }
+    ret = ff_mov_read_chnl(c->fc, pb, st);
+    if (ret < 0)
+        return ret;
 
     if (avio_tell(pb) != end) {
         av_log(c->fc, AV_LOG_WARNING, "skip %" PRId64 " bytes of unknown data inside chnl\n",
diff --git a/libavformat/mov_chan.c b/libavformat/mov_chan.c
index 79768bc210..cce9d7a697 100644
--- a/libavformat/mov_chan.c
+++ b/libavformat/mov_chan.c
@@ -863,3 +863,62 @@ error:
     av_channel_layout_uninit(layout);
     return ret;
 }
+
+int ff_mov_read_chnl(AVFormatContext *s, AVIOContext *pb, AVStream *st)
+{
+    int stream_structure = avio_r8(pb);
+    int ret;
+
+    // stream carries channels
+    if (stream_structure & 1) {
+        int layout = avio_r8(pb);
+
+        av_log(s, AV_LOG_TRACE, "'chnl' layout %d\n", layout);
+        if (!layout) {
+            uint8_t *positions = av_malloc(st->codecpar->ch_layout.nb_channels);
+
+            if (!positions)
+                return AVERROR(ENOMEM);
+            for (int i = 0; i < st->codecpar->ch_layout.nb_channels; i++) {
+                int speaker_pos = avio_r8(pb);
+
+                av_log(s, AV_LOG_TRACE, "speaker_position %d\n", speaker_pos);
+                if (speaker_pos == 126) { // explicit position
+                    avpriv_request_sample(s, "explicit position");
+                    av_freep(&positions);
+                    return AVERROR_PATCHWELCOME;
+                } else {
+                    positions[i] = speaker_pos;
+                }
+            }
+
+            ret = ff_mov_get_layout_from_channel_positions(positions,
+                    st->codecpar->ch_layout.nb_channels,
+                    &st->codecpar->ch_layout);
+            av_freep(&positions);
+            if (ret) {
+                av_log(s, AV_LOG_ERROR,
+                        "get channel layout from speaker positions failed, %s\n",
+                        av_err2str(ret));
+                return ret;
+            }
+        } else {
+            uint64_t omitted_channel_map = avio_rb64(pb);
+
+            if (omitted_channel_map) {
+                avpriv_request_sample(s, "omitted_channel_map 0x%" PRIx64 " != 0",
+                                      omitted_channel_map);
+                return AVERROR_PATCHWELCOME;
+            }
+            ff_mov_get_channel_layout_from_config(layout, &st->codecpar->ch_layout);
+        }
+    }
+
+    // stream carries objects
+    if (stream_structure & 2) {
+        int obj_count = avio_r8(pb);
+        av_log(s, AV_LOG_TRACE, "'chnl' with object_count %d\n", obj_count);
+    }
+
+    return 0;
+}
diff --git a/libavformat/mov_chan.h b/libavformat/mov_chan.h
index 8c807798ab..b7d435b99f 100644
--- a/libavformat/mov_chan.h
+++ b/libavformat/mov_chan.h
@@ -189,4 +189,14 @@ int ff_mov_get_channel_positions_from_layout(const AVChannelLayout *layout,
 int ff_mov_get_layout_from_channel_positions(const uint8_t *position, int position_num,
                                              AVChannelLayout *layout);
 
+/**
+ * Read 'chnl' tag from the input stream.
+ *
+ * @param s     AVFormatContext
+ * @param pb    AVIOContext
+ * @param st    The stream to set codec values for
+ * @return      0 if ok, or negative AVERROR code on failure
+ */
+int ff_mov_read_chnl(AVFormatContext *s, AVIOContext *pb, AVStream *st);
+
 #endif /* AVFORMAT_MOV_CHAN_H */
-- 
2.35.3

_______________________________________________
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] [PATCH 5/5] avformat/mov: rework ff_mov_read_chnl

[Marton Balint]

A lot of changes and fixes to channel layout parsing, notably
- get rid of dynamic allocation of channel positions
- signal unimplemented speaker positions as unknown instead of failure, but
  warn the user about it
- native order, and that a single channel only appears once was always assumed
  for less than 64 channels, obviously this was incorrect

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mov_chan.c | 106 ++++++++++-------------------------------
 libavformat/mov_chan.h |   6 ---
 2 files changed, 26 insertions(+), 86 deletions(-)

diff --git a/libavformat/mov_chan.c b/libavformat/mov_chan.c
index cce9d7a697..3e186b0837 100644
--- a/libavformat/mov_chan.c
+++ b/libavformat/mov_chan.c
@@ -804,66 +804,6 @@ int ff_mov_get_channel_positions_from_layout(const AVChannelLayout *layout,
     return 0;
 }
 
-int ff_mov_get_layout_from_channel_positions(const uint8_t *position, int position_num,
-                                             AVChannelLayout *layout)
-{
-    int ret;
-    enum AVChannel channel;
-
-    av_channel_layout_uninit(layout);
-
-    if (position_num <= 63) {
-        layout->order = AV_CHANNEL_ORDER_NATIVE;
-        layout->nb_channels = position_num;
-        for (int i = 0; i < position_num; i++) {
-            if (position[i] >= FF_ARRAY_ELEMS(iso_channel_position)) {
-                ret = AVERROR_PATCHWELCOME;
-                goto error;
-            }
-
-            channel = iso_channel_position[position[i]];
-            // unsupported layout
-            if (channel == AV_CHAN_NONE) {
-                ret = AVERROR_PATCHWELCOME;
-                goto error;
-            }
-
-            layout->u.mask |= 1ULL << channel;
-        }
-    } else {
-        layout->order = AV_CHANNEL_ORDER_CUSTOM;
-        layout->nb_channels = position_num;
-        layout->u.map = av_calloc(position_num, sizeof(*layout->u.map));
-        if (!layout->u.map) {
-            ret = AVERROR(ENOMEM);
-            goto error;
-        }
-
-        for (int i = 0; i < position_num; i++) {
-            if (position[i] >= FF_ARRAY_ELEMS(iso_channel_position)) {
-                ret = AVERROR_PATCHWELCOME;
-                goto error;
-            }
-
-            channel = iso_channel_position[position[i]];
-            // unsupported layout
-            if (channel == AV_CHAN_NONE) {
-                ret = AVERROR_PATCHWELCOME;
-                goto error;
-            }
-
-            layout->u.map[i].id = channel;
-        }
-    }
-
-
-    return 0;
-
-error:
-    av_channel_layout_uninit(layout);
-    return ret;
-}
-
 int ff_mov_read_chnl(AVFormatContext *s, AVIOContext *pb, AVStream *st)
 {
     int stream_structure = avio_r8(pb);
@@ -875,33 +815,39 @@ int ff_mov_read_chnl(AVFormatContext *s, AVIOContext *pb, AVStream *st)
 
         av_log(s, AV_LOG_TRACE, "'chnl' layout %d\n", layout);
         if (!layout) {
-            uint8_t *positions = av_malloc(st->codecpar->ch_layout.nb_channels);
+            AVChannelLayout *ch_layout = &st->codecpar->ch_layout;
+            int nb_channels = ch_layout->nb_channels;
+
+            av_channel_layout_uninit(ch_layout);
+            ret = av_channel_layout_custom_init(ch_layout, nb_channels);
+            if (ret < 0)
+                return ret;
 
-            if (!positions)
-                return AVERROR(ENOMEM);
-            for (int i = 0; i < st->codecpar->ch_layout.nb_channels; i++) {
+            for (int i = 0; i < nb_channels; i++) {
                 int speaker_pos = avio_r8(pb);
+                enum AVChannel channel;
+
+                if (speaker_pos == 126) // explicit position
+                    avio_skip(pb, 3);   // azimuth, elevation
 
-                av_log(s, AV_LOG_TRACE, "speaker_position %d\n", speaker_pos);
-                if (speaker_pos == 126) { // explicit position
-                    avpriv_request_sample(s, "explicit position");
-                    av_freep(&positions);
-                    return AVERROR_PATCHWELCOME;
-                } else {
-                    positions[i] = speaker_pos;
+                if (speaker_pos >= FF_ARRAY_ELEMS(iso_channel_position))
+                    channel = AV_CHAN_NONE;
+                else
+                    channel = iso_channel_position[speaker_pos];
+
+                if (channel == AV_CHAN_NONE) {
+                    av_log(s, AV_LOG_WARNING, "speaker position %d is not implemented\n", speaker_pos);
+                    channel = AV_CHAN_UNKNOWN;
                 }
+
+                ch_layout->u.map[i].id = channel;
             }
 
-            ret = ff_mov_get_layout_from_channel_positions(positions,
-                    st->codecpar->ch_layout.nb_channels,
-                    &st->codecpar->ch_layout);
-            av_freep(&positions);
-            if (ret) {
-                av_log(s, AV_LOG_ERROR,
-                        "get channel layout from speaker positions failed, %s\n",
-                        av_err2str(ret));
+            ret = av_channel_layout_retype(ch_layout, AV_CHANNEL_ORDER_NATIVE, AV_CHANNEL_LAYOUT_RETYPE_FLAG_LOSSLESS);
+            if (ret == AVERROR(ENOSYS))
+                ret = av_channel_layout_retype(ch_layout, AV_CHANNEL_ORDER_UNSPEC, AV_CHANNEL_LAYOUT_RETYPE_FLAG_LOSSLESS);
+            if (ret < 0 && ret != AVERROR(ENOSYS))
                 return ret;
-            }
         } else {
             uint64_t omitted_channel_map = avio_rb64(pb);
 
diff --git a/libavformat/mov_chan.h b/libavformat/mov_chan.h
index b7d435b99f..e480809c44 100644
--- a/libavformat/mov_chan.h
+++ b/libavformat/mov_chan.h
@@ -183,12 +183,6 @@ int ff_mov_get_channel_layout_from_config(int config, AVChannelLayout *layout);
 int ff_mov_get_channel_positions_from_layout(const AVChannelLayout *layout,
                                              uint8_t *position, int position_num);
 
-/**
- * Get AVChannelLayout from ISO/IEC 23001-8 OutputChannelPosition.
- */
-int ff_mov_get_layout_from_channel_positions(const uint8_t *position, int position_num,
-                                             AVChannelLayout *layout);
-
 /**
  * Read 'chnl' tag from the input stream.
  *
-- 
2.35.3

_______________________________________________
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 1/5] avutil/channel_layout: change AV_CHAN_SILENCE to AV_CHAN_UNUSED in the docs

[James Almer]

On 2/12/2024 6:15 PM, Marton Balint wrote:
> It got renamed during the API design phase.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>   libavutil/channel_layout.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
> index 7ee5333ea8..b8bff6f365 100644
> --- a/libavutil/channel_layout.h
> +++ b/libavutil/channel_layout.h
> @@ -119,7 +119,7 @@ enum AVChannelOrder {
>       /**
>        * The channel order does not correspond to any other predefined order and
>        * is stored as an explicit map. For example, this could be used to support
> -     * layouts with 64 or more channels, or with empty/skipped (AV_CHAN_SILENCE)
> +     * layouts with 64 or more channels, or with empty/skipped (AV_CHAN_UNUSED)
>        * channels at arbitrary positions.
>        */
>       AV_CHANNEL_ORDER_CUSTOM,

Good catch, LGTM. This should be backported too.
_______________________________________________
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 2/5] avutil/channel_layout: add AV_CHANNEL_ORDER_NB

[James Almer]

On 2/12/2024 6:15 PM, Marton Balint wrote:
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>   libavutil/channel_layout.h | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
> index b8bff6f365..db0c005e87 100644
> --- a/libavutil/channel_layout.h
> +++ b/libavutil/channel_layout.h
> @@ -146,6 +146,10 @@ enum AVChannelOrder {
>        * as defined in AmbiX format $ 2.1.
>        */
>       AV_CHANNEL_ORDER_AMBISONIC,
> +    /**
> +     * Number of channel orders, not part of ABI/API
> +     */
> +    AV_CHANNEL_ORDER_NB
>   };

Is it worth adding this to a public header just to limit a loop in a 
test? A loop that fwiw still depends on an array that needs to be 
updated with more names if you add new orders.

IMO, just do FF_ARRAY_ELEMS(channel_order_names) in the test.
_______________________________________________
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] [PATCH] avutil/channel_layout: print known layout names in custom layout

[James Almer]

If a custom layout is equivalent to a native one, check if it matches one of the
known layout names and print that instead.

Signed-off-by: James Almer <jamrial@gmail.com>
---
Should be applied after the patch this one is a reply to.

 libavutil/channel_layout.c    | 68 +++++++++++++++++++++--------------
 tests/ref/fate/channel_layout |  4 +--
 2 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/libavutil/channel_layout.c b/libavutil/channel_layout.c
index 7f51c076dc..8b3bf2f4af 100644
--- a/libavutil/channel_layout.c
+++ b/libavutil/channel_layout.c
@@ -679,6 +679,29 @@ int av_channel_layout_copy(AVChannelLayout *dst, const AVChannelLayout *src)
     return 0;
 }
 
+static int64_t masked_description(const AVChannelLayout *channel_layout, int start_channel)
+{
+    uint64_t mask = 0;
+    for (int i = start_channel; i < channel_layout->nb_channels; i++) {
+        enum AVChannel ch = channel_layout->u.map[i].id;
+        if (ch >= 0 && ch < 63 && mask < (1ULL << ch))
+            mask |= (1ULL << ch);
+        else
+            return AVERROR(EINVAL);
+    }
+    return mask;
+}
+
+static int has_channel_names(const AVChannelLayout *channel_layout)
+{
+    if (channel_layout->order != AV_CHANNEL_ORDER_CUSTOM)
+        return 0;
+    for (int i = 0; i < channel_layout->nb_channels; i++)
+        if (channel_layout->u.map[i].name[0])
+            return 1;
+    return 0;
+}
+
 /**
  * If the layout is n-th order standard-order ambisonic, with optional
  * extra non-diegetic channels at the end, return the order.
@@ -746,9 +769,17 @@ static int try_describe_ambisonic(AVBPrint *bp, const AVChannelLayout *channel_l
             extra.nb_channels = av_popcount64(channel_layout->u.mask);
             extra.u.mask      = channel_layout->u.mask;
         } else {
-            extra.order       = AV_CHANNEL_ORDER_CUSTOM;
-            extra.nb_channels = channel_layout->nb_channels - nb_ambi_channels;
-            extra.u.map       = channel_layout->u.map + nb_ambi_channels;
+            int64_t mask;
+            if (!has_channel_names(channel_layout) &&
+                (mask = masked_description(channel_layout, nb_ambi_channels)) > 0) {
+                extra.order       = AV_CHANNEL_ORDER_NATIVE;
+                extra.nb_channels = av_popcount64(mask);
+                extra.u.mask      = mask;
+            } else {
+                extra.order       = AV_CHANNEL_ORDER_CUSTOM;
+                extra.nb_channels = channel_layout->nb_channels - nb_ambi_channels;
+                extra.u.map       = channel_layout->u.map + nb_ambi_channels;
+            }
         }
 
         av_bprint_chars(bp, '+', 1);
@@ -774,9 +805,17 @@ int av_channel_layout_describe_bprint(const AVChannelLayout *channel_layout,
         // fall-through
     case AV_CHANNEL_ORDER_CUSTOM:
         if (channel_layout->order == AV_CHANNEL_ORDER_CUSTOM) {
+            int64_t mask;
             int res = try_describe_ambisonic(bp, channel_layout);
             if (res >= 0)
                 return 0;
+            if (!has_channel_names(channel_layout) &&
+                (mask = masked_description(channel_layout, 0)) > 0) {
+                AVChannelLayout native = { .order       = AV_CHANNEL_ORDER_NATIVE,
+                                           .nb_channels = av_popcount64(mask),
+                                           .u.mask      = mask };
+                return av_channel_layout_describe_bprint(&native, bp);
+            }
         }
         if (channel_layout->nb_channels)
             av_bprintf(bp, "%d channels (", channel_layout->nb_channels);
@@ -1037,29 +1076,6 @@ uint64_t av_channel_layout_subset(const AVChannelLayout *channel_layout,
     return ret;
 }
 
-static int64_t masked_description(AVChannelLayout *channel_layout, int start_channel)
-{
-    uint64_t mask = 0;
-    for (int i = start_channel; i < channel_layout->nb_channels; i++) {
-        enum AVChannel ch = channel_layout->u.map[i].id;
-        if (ch >= 0 && ch < 63 && mask < (1ULL << ch))
-            mask |= (1ULL << ch);
-        else
-            return AVERROR(EINVAL);
-    }
-    return mask;
-}
-
-static int has_channel_names(AVChannelLayout *channel_layout)
-{
-    if (channel_layout->order != AV_CHANNEL_ORDER_CUSTOM)
-        return 0;
-    for (int i = 0; i < channel_layout->nb_channels; i++)
-        if (channel_layout->u.map[i].name[0])
-            return 1;
-    return 0;
-}
-
 int av_channel_layout_retype(AVChannelLayout *channel_layout, enum AVChannelOrder order, int flags)
 {
     int allow_lossy = !(flags & AV_CHANNEL_LAYOUT_RETYPE_FLAG_LOSSLESS);
diff --git a/tests/ref/fate/channel_layout b/tests/ref/fate/channel_layout
index 1d1f1cb082..466fa78d9e 100644
--- a/tests/ref/fate/channel_layout
+++ b/tests/ref/fate/channel_layout
@@ -195,7 +195,7 @@ With "FL@Boo": CUSTOM (1 channels (FL@Boo))
 With "stereo": NATIVE (stereo)
   ~~ UNSPEC (2 channels)
   == NATIVE (stereo)
-  == CUSTOM (2 channels (FL+FR))
+  == CUSTOM (stereo)
   != AMBI
 With "FR+FL": CUSTOM (2 channels (FR+FL))
   ~~ UNSPEC (2 channels)
@@ -205,7 +205,7 @@ With "FR+FL": CUSTOM (2 channels (FR+FL))
 With "ambisonic 2+stereo": AMBI   (ambisonic 2+stereo)
   ~~ UNSPEC (11 channels)
   != NATIVE
-  == CUSTOM (ambisonic 2+2 channels (FL+FR))
+  == CUSTOM (ambisonic 2+stereo)
   == AMBI   (ambisonic 2+stereo)
 With "2C": UNSPEC (2 channels)
   == UNSPEC (2 channels)
-- 
2.43.1

_______________________________________________
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 2/5] avutil/channel_layout: add AV_CHANNEL_ORDER_NB

[Marton Balint]

On Tue, 13 Feb 2024, James Almer wrote:

> On 2/12/2024 6:15 PM, Marton Balint wrote:
>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>  ---
>>    libavutil/channel_layout.h | 4 ++++
>>    1 file changed, 4 insertions(+)
>>
>>  diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
>>  index b8bff6f365..db0c005e87 100644
>>  --- a/libavutil/channel_layout.h
>>  +++ b/libavutil/channel_layout.h
>>  @@ -146,6 +146,10 @@ enum AVChannelOrder {
>>         * as defined in AmbiX format $ 2.1.
>>         */
>>        AV_CHANNEL_ORDER_AMBISONIC,
>>  +    /**
>>  +     * Number of channel orders, not part of ABI/API
>>  +     */
>>  +    AV_CHANNEL_ORDER_NB
>>    };
>
> Is it worth adding this to a public header just to limit a loop in a test? A 
> loop that fwiw still depends on an array that needs to be updated with more 
> names if you add new orders.

Several other enums also have this. So API consistency can be considered 
a more important factor.

>
> IMO, just do FF_ARRAY_ELEMS(channel_order_names) in the test.

Then adding a new channel order would not show up as breakage in the 
test. I have no strong preference though, and can change it if you 
still want me to.

Regards,
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 2/5] avutil/channel_layout: add AV_CHANNEL_ORDER_NB

[Anton Khirnov]

Quoting Marton Balint (2024-02-13 21:27:34)
> 
> 
> On Tue, 13 Feb 2024, James Almer wrote:
> 
> > On 2/12/2024 6:15 PM, Marton Balint wrote:
> >>  Signed-off-by: Marton Balint <cus@passwd.hu>
> >>  ---
> >>    libavutil/channel_layout.h | 4 ++++
> >>    1 file changed, 4 insertions(+)
> >>
> >>  diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
> >>  index b8bff6f365..db0c005e87 100644
> >>  --- a/libavutil/channel_layout.h
> >>  +++ b/libavutil/channel_layout.h
> >>  @@ -146,6 +146,10 @@ enum AVChannelOrder {
> >>         * as defined in AmbiX format $ 2.1.
> >>         */
> >>        AV_CHANNEL_ORDER_AMBISONIC,
> >>  +    /**
> >>  +     * Number of channel orders, not part of ABI/API
> >>  +     */
> >>  +    AV_CHANNEL_ORDER_NB
> >>    };
> >
> > Is it worth adding this to a public header just to limit a loop in a test? A 
> > loop that fwiw still depends on an array that needs to be updated with more 
> > names if you add new orders.
> 
> Several other enums also have this. So API consistency can be considered 
> a more important factor.

I'd be concerned that many callers don't undertand the implications of
"not part of the ABI".

Maybe we should rename all of them to FF_ prefix to make it more clear
callers should not use these?

-- 
Anton Khirnov
_______________________________________________
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 5/5] avformat/mov: rework ff_mov_read_chnl

[Anton Khirnov]

Quoting Marton Balint (2024-02-12 22:15:37)
> - native order, and that a single channel only appears once was always assumed
>   for less than 64 channels, obviously this was incorrect

Could you add a test for the case where it's not true?

-- 
Anton Khirnov
_______________________________________________
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 5/5] avformat/mov: rework ff_mov_read_chnl

[Marton Balint]

On Thu, 15 Feb 2024, Anton Khirnov wrote:

> Quoting Marton Balint (2024-02-12 22:15:37)
>> - native order, and that a single channel only appears once was always assumed
>>   for less than 64 channels, obviously this was incorrect
>
> Could you add a test for the case where it's not true?

Actually fate-mov-mp4-pcm-float should cover this if I change the channel 
layout there to something which is not representible as a native layout.

Regards,
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 2/5] avutil/channel_layout: add AV_CHANNEL_ORDER_NB

[Marton Balint]

On Thu, 15 Feb 2024, Anton Khirnov wrote:

> Quoting Marton Balint (2024-02-13 21:27:34)
>>
>>
>> On Tue, 13 Feb 2024, James Almer wrote:
>>
>>> On 2/12/2024 6:15 PM, Marton Balint wrote:
>>>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>  ---
>>>>    libavutil/channel_layout.h | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>>  diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
>>>>  index b8bff6f365..db0c005e87 100644
>>>>  --- a/libavutil/channel_layout.h
>>>>  +++ b/libavutil/channel_layout.h
>>>>  @@ -146,6 +146,10 @@ enum AVChannelOrder {
>>>>         * as defined in AmbiX format $ 2.1.
>>>>         */
>>>>        AV_CHANNEL_ORDER_AMBISONIC,
>>>>  +    /**
>>>>  +     * Number of channel orders, not part of ABI/API
>>>>  +     */
>>>>  +    AV_CHANNEL_ORDER_NB
>>>>    };
>>>
>>> Is it worth adding this to a public header just to limit a loop in a test? A
>>> loop that fwiw still depends on an array that needs to be updated with more
>>> names if you add new orders.
>>
>> Several other enums also have this. So API consistency can be considered
>> a more important factor.
>
> I'd be concerned that many callers don't undertand the implications of
> "not part of the ABI".
>
> Maybe we should rename all of them to FF_ prefix to make it more clear
> callers should not use these?

I think this is a good idea. So is it OK to apply this if I change the 
prefix to FF?

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 2/5] avutil/channel_layout: add AV_CHANNEL_ORDER_NB

[James Almer]

On 2/16/2024 7:42 PM, Marton Balint wrote:
> 
> 
> On Thu, 15 Feb 2024, Anton Khirnov wrote:
> 
>> Quoting Marton Balint (2024-02-13 21:27:34)
>>>
>>>
>>> On Tue, 13 Feb 2024, James Almer wrote:
>>>
>>>> On 2/12/2024 6:15 PM, Marton Balint wrote:
>>>>>  Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>>  ---
>>>>>    libavutil/channel_layout.h | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>>  diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
>>>>>  index b8bff6f365..db0c005e87 100644
>>>>>  --- a/libavutil/channel_layout.h
>>>>>  +++ b/libavutil/channel_layout.h
>>>>>  @@ -146,6 +146,10 @@ enum AVChannelOrder {
>>>>>         * as defined in AmbiX format $ 2.1.
>>>>>         */
>>>>>        AV_CHANNEL_ORDER_AMBISONIC,
>>>>>  +    /**
>>>>>  +     * Number of channel orders, not part of ABI/API
>>>>>  +     */
>>>>>  +    AV_CHANNEL_ORDER_NB
>>>>>    };
>>>>
>>>> Is it worth adding this to a public header just to limit a loop in a 
>>>> test? A
>>>> loop that fwiw still depends on an array that needs to be updated 
>>>> with more
>>>> names if you add new orders.
>>>
>>> Several other enums also have this. So API consistency can be considered
>>> a more important factor.
>>
>> I'd be concerned that many callers don't undertand the implications of
>> "not part of the ABI".
>>
>> Maybe we should rename all of them to FF_ prefix to make it more clear
>> callers should not use these?
> 
> I think this is a good idea. So is it OK to apply this if I change the 
> prefix to FF?

I wont oppose to it.
_______________________________________________
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 2/5] avutil/channel_layout: add AV_CHANNEL_ORDER_NB

[Marton Balint]

On Fri, 16 Feb 2024, James Almer wrote:

> On 2/16/2024 7:42 PM, Marton Balint wrote:
>>
>>
>>  On Thu, 15 Feb 2024, Anton Khirnov wrote:
>>
>>>  Quoting Marton Balint (2024-02-13 21:27:34)
>>>> 
>>>>
>>>>  On Tue, 13 Feb 2024, James Almer wrote:
>>>>
>>>>>  On 2/12/2024 6:15 PM, Marton Balint wrote:
>>>>>>   Signed-off-by: Marton Balint <cus@passwd.hu>
>>>>>>   ---
>>>>>>     libavutil/channel_layout.h | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>>   diff --git a/libavutil/channel_layout.h b/libavutil/channel_layout.h
>>>>>>   index b8bff6f365..db0c005e87 100644
>>>>>>   --- a/libavutil/channel_layout.h
>>>>>>   +++ b/libavutil/channel_layout.h
>>>>>>   @@ -146,6 +146,10 @@ enum AVChannelOrder {
>>>>>>          * as defined in AmbiX format $ 2.1.
>>>>>>          */
>>>>>>         AV_CHANNEL_ORDER_AMBISONIC,
>>>>>>   +    /**
>>>>>>   +     * Number of channel orders, not part of ABI/API
>>>>>>   +     */
>>>>>>   +    AV_CHANNEL_ORDER_NB
>>>>>>     };
>>>>>
>>>>>  Is it worth adding this to a public header just to limit a loop in a
>>>>>  test? A
>>>>>  loop that fwiw still depends on an array that needs to be updated with
>>>>>  more
>>>>>  names if you add new orders.
>>>>
>>>>  Several other enums also have this. So API consistency can be considered
>>>>  a more important factor.
>>>
>>>  I'd be concerned that many callers don't undertand the implications of
>>>  "not part of the ABI".
>>>
>>>  Maybe we should rename all of them to FF_ prefix to make it more clear
>>>  callers should not use these?
>>
>>  I think this is a good idea. So is it OK to apply this if I change the
>>  prefix to FF?
>
> I wont oppose to it.

Ok, changed locally.

Will apply the series soon.

Regards,
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] avutil/channel_layout: print known layout names in custom layout

[James Almer]

On 2/13/2024 2:39 PM, James Almer wrote:
> If a custom layout is equivalent to a native one, check if it matches one of the
> known layout names and print that instead.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> Should be applied after the patch this one is a reply to.
> 
>   libavutil/channel_layout.c    | 68 +++++++++++++++++++++--------------
>   tests/ref/fate/channel_layout |  4 +--
>   2 files changed, 44 insertions(+), 28 deletions(-)

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