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

[Felicia Lim]

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 at 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 at google.com> wrote:
>
>> On Sat, Jul 28, 2018 at 10:46 AM Rostislav Pehlivanov <
>> atomnuker at gmail.com> wrote:
>>
>>> On 27 July 2018 at 20:44, Felicia Lim <flim-at-google.com at 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 at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>