| Message ID | 20180727194431.191229-1-flim@google.com |
|---|---|
| State | Accepted |
| Commit | 4cd6f08d2005c20a03ccd53a0a6f8a115c5ebe2e |
| Headers | show |
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. 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?
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.
On Fri, 27 Jul 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); > + } > + break; > case 255: > ret = libopus_check_max_channels(avctx, 254); > break; > -- > 2.18.0.345.g5c9ce644c3-goog > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel I did not OK the patch at all and none of the requests I made were met. This has to be reverted.
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;