[FFmpeg-devel] avcodec/libopusenc: allow encoding channel mapping 2

Submitted by Felicia Lim on Aug. 8, 2018, 10:41 p.m.

Details

Message ID CAJ0LFHXNg+TBpo9K_ZinQ+Z5tY0GC6YfuOBXLSVciUONX8xz6w@mail.gmail.com
State New
Headers show

Commit Message

Felicia Lim Aug. 8, 2018, 10:41 p.m.
I've attached the patch with the updated description, please let me know if
I should make any other changes.

Thanks,
Felicia


On Mon, Jul 30, 2018 at 10:50 AM Felicia Lim <flim@google.com> wrote:

> On Sat, Jul 28, 2018 at 10:46 AM Rostislav Pehlivanov <atomnuker@gmail.com>
> wrote:
>
>> On 27 July 2018 at 20:44, Felicia Lim <flim-at-google.com@ffmpeg.org>
>> wrote:
>>
>> > ---
>> >  libavcodec/libopusenc.c | 22 ++++++++++++++++++++++
>> >  1 file changed, 22 insertions(+)
>> >
>> > diff --git a/libavcodec/libopusenc.c b/libavcodec/libopusenc.c
>> > index 4ae81b0bb2..6b450ec317 100644
>> > --- a/libavcodec/libopusenc.c
>> > +++ b/libavcodec/libopusenc.c
>> > @@ -27,6 +27,7 @@
>> >  #include "bytestream.h"
>> >  #include "internal.h"
>> >  #include "libopus.h"
>> > +#include "mathops.h"
>> >  #include "vorbis.h"
>> >  #include "audio_frame_queue.h"
>> >
>> > @@ -200,6 +201,21 @@ static int
>> libopus_check_vorbis_layout(AVCodecContext
>> > *avctx, int mapping_family
>> >      return 0;
>> >  }
>> >
>> > +static int libopus_check_ambisonics_channels(AVCodecContext *avctx) {
>> > +    int channels = avctx->channels;
>> > +    int ambisonic_order = ff_sqrt(channels) - 1;
>> > +    if (channels != ((ambisonic_order + 1) * (ambisonic_order + 1)) &&
>> > +        channels != ((ambisonic_order + 1) * (ambisonic_order + 1) +
>> 2)) {
>> > +        av_log(avctx, AV_LOG_ERROR,
>> > +               "Ambisonics coding is only specified for channel counts"
>> > +               " which can be written as (n + 1)^2 or (n + 1)^2 + 2"
>> > +               " for nonnegative integer n\n");
>> > +        return AVERROR_INVALIDDATA;
>> > +    }
>> > +
>> > +    return 0;
>> > +}
>> > +
>> >  static int libopus_validate_layout_and_get_channel_map(
>> >          AVCodecContext *avctx,
>> >          int mapping_family,
>> > @@ -231,6 +247,12 @@ static int libopus_validate_layout_and_
>> > get_channel_map(
>> >              channel_map =
>> ff_vorbis_channel_layout_offsets[avctx->channels
>> > - 1];
>> >          }
>> >          break;
>> > +    case 2:
>> > +        ret = libopus_check_max_channels(avctx, 227);
>> > +        if (ret == 0) {
>> > +            ret = libopus_check_ambisonics_channels(avctx);
>> > +        }
>> >
>>
>> No brackets needed, also if (ret) looks better imo.
>>
>
> Thanks for reviewing. Would this change would break with the style
> elsewhere in this function? I'm happy to change if you still think I should
> do it.
>
> I also just realized the patch description should be "Remove warning when
> trying to encode channel mapping 2": it already encodes even without this
> patch. Will send an update after the above comment is resolved.
>
>
>> Does libopus understand channel mapping 2 as ambisonics? What about the
>> libopus_generic_encoder_() API? Doesn't that need to be used to encode
>> ambisonics, like the patch by Drew did?
>>
>
> Yes, libopus also understands channel mapping 2 as ambisonics by default
> now.
>
> Ch map 2 uses the normal opus_multistream_encode(), so no further changes
> are needed. On the other hand, ch map 3 uses the new
> opus_projection_encode(), hence Drew's introduction of
> libopus_generic_encoder_() to handle the switch as needed.
>
>
> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>

Patch hide | download patch | download mbox

From 75a2470f26ea0d4c5bf8482001ce7d32212ba06f Mon Sep 17 00:00:00 2001
From: Felicia Lim <flim@google.com>
Date: Mon, 30 Jul 2018 12:59:44 -0700
Subject: [PATCH] avcodec/libopusenc: Fix warning when encoding ambisonics with
 channel mapping 2

Also adds checks on the number of channels.
---
 libavcodec/libopusenc.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/libavcodec/libopusenc.c b/libavcodec/libopusenc.c
index 4ae81b0bb2..6b450ec317 100644
--- a/libavcodec/libopusenc.c
+++ b/libavcodec/libopusenc.c
@@ -27,6 +27,7 @@ 
 #include "bytestream.h"
 #include "internal.h"
 #include "libopus.h"
+#include "mathops.h"
 #include "vorbis.h"
 #include "audio_frame_queue.h"
 
@@ -200,6 +201,21 @@  static int libopus_check_vorbis_layout(AVCodecContext *avctx, int mapping_family
     return 0;
 }
 
+static int libopus_check_ambisonics_channels(AVCodecContext *avctx) {
+    int channels = avctx->channels;
+    int ambisonic_order = ff_sqrt(channels) - 1;
+    if (channels != ((ambisonic_order + 1) * (ambisonic_order + 1)) &&
+        channels != ((ambisonic_order + 1) * (ambisonic_order + 1) + 2)) {
+        av_log(avctx, AV_LOG_ERROR,
+               "Ambisonics coding is only specified for channel counts"
+               " which can be written as (n + 1)^2 or (n + 1)^2 + 2"
+               " for nonnegative integer n\n");
+        return AVERROR_INVALIDDATA;
+    }
+
+    return 0;
+}
+
 static int libopus_validate_layout_and_get_channel_map(
         AVCodecContext *avctx,
         int mapping_family,
@@ -231,6 +247,12 @@  static int libopus_validate_layout_and_get_channel_map(
             channel_map = ff_vorbis_channel_layout_offsets[avctx->channels - 1];
         }
         break;
+    case 2:
+        ret = libopus_check_max_channels(avctx, 227);
+        if (ret == 0) {
+            ret = libopus_check_ambisonics_channels(avctx);
+        }
+        break;
     case 255:
         ret = libopus_check_max_channels(avctx, 254);
         break;
-- 
2.18.0.597.ga71716f1ad-goog