| Message ID | CAJ0LFHXNg+TBpo9K_ZinQ+Z5tY0GC6YfuOBXLSVciUONX8xz6w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >>> >>
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 [...]
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.
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