[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
>>
>

Comments

Felicia Lim Nov. 27, 2018, 10:13 a.m.
Friendly ping. Please let me know if there are any changes I should make to
this patch?

Thanks!
Felicia



On Thu, Aug 9, 2018 at 6:41 AM Felicia Lim <flim@google.com> wrote:

> 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
>>>
>>
Michael Niedermayer Nov. 28, 2018, 8:35 p.m.
On Tue, Nov 27, 2018 at 06:13:54PM +0800, Felicia Lim wrote:
> Friendly ping. Please let me know if there are any changes I should make to
> this patch?

will apply, seems noone else cares about it ...

thanks

[...]
Rostislav Pehlivanov Nov. 29, 2018, 12:10 a.m.
On Wed, 28 Nov 2018 at 20:35, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Tue, Nov 27, 2018 at 06:13:54PM +0800, Felicia Lim wrote:
> > Friendly ping. Please let me know if there are any changes I should make
> to
> > this patch?
>
> will apply, seems noone else cares about it ...
>
> thanks
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Rewriting code that is poorly written but fully understood is good.
> Rewriting code that one doesnt understand is a sign that one is less smart
> then the original author, trying to rewrite it will not make it better.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Reverted.
I did describe what has to be done to properly support ambisonics as to
make this patch work properly. I know it works for Google's usecase but I'm
against "by-chance" ambisonics where you rely on a specific channel layout
and a specific encoder encoding them with a specific setting.
We still have bits leftover for channel layout and I'd agree to add an
extra bit to indicate the particular layout carries ambisonics, with the
number of channels indicating the order. This wouldn't need extensive
channel layout API replacements and would still be sufficient to implement
both encoding and decoding of such audio.

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