Re: [FFmpeg-devel] [PATCH] avcodec/libopusenc: allow encoding channel mapping 2
On Wed, 28 Nov 2018 at 20:35, Michael Niedermayer 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. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libopusenc: allow encoding channel mapping 2
On Fri, 27 Jul 2018 at 20:44, Felicia Lim
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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libopusenc: allow encoding channel mapping 2
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. signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libopusenc: allow encoding channel mapping 2
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 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 wrote:
>
>> On Sat, Jul 28, 2018 at 10:46 AM Rostislav Pehlivanov <
>> atomnu...@gmail.com> wrote:
>>
>>> On 27 July 2018 at 20:44, Felicia Lim
>>> 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
>>>
>>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libopusenc: allow encoding channel mapping 2
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 wrote:
> On Sat, Jul 28, 2018 at 10:46 AM Rostislav Pehlivanov
> wrote:
>
>> On 27 July 2018 at 20:44, Felicia Lim
>> 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
>>
>
From 75a2470f26ea0d4c5bf8482001ce7d32212ba06f Mon Sep 17 00:00:00 2001
From: Felicia Lim
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];
Re: [FFmpeg-devel] [PATCH] avcodec/libopusenc: allow encoding channel mapping 2
On Sat, Jul 28, 2018 at 10:46 AM Rostislav Pehlivanov
wrote:
> On 27 July 2018 at 20:44, Felicia Lim
> 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
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] avcodec/libopusenc: allow encoding channel mapping 2
On 27 July 2018 at 20:44, Felicia Lim 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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avcodec/libopusenc: allow encoding channel mapping 2
---
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
