Re: [FFmpeg-devel] [PATCH] libopusenc: Add channel mapping family argument

[Michael Niedermayer]

On Sun, Jun 26, 2016 at 04:55:56PM -0700, Michael Graczyk wrote:
> Michael,
> Thanks for your comments.
> 
> On Sat, Jun 25, 2016 at 4:28 AM, Michael Niedermayer
>  wrote:
> > yes, rereading this a bit
> > can you explain what is the difference between the mapping familiy
> > and he channel_layout we have ?
> 
> The Opus channel mapping family and FFmpeg channel layout both provide
> a mapping between stream position and semantic meaning. That is, they
> both answer the question "What does the channel at position k in the
> stream contain?"
> 
> The two parameters are both defined by explicitly enumerating
> acceptable possibilities. There are some channel mappings which are
> defined by Opus but are not defined by FFmpeg. Likewise, there are
> channel mappings defined by FFmpeg which are not defined by Opus.
> Because of the differences, there is no unambiguous 1:1 correspondence
> between Opus channel mapping family and FFmpeg channel_layout. For
> example, FFmpeg has no way of specifying "channel k is an SN3D
> normalized ambisonic channel kth in ACN order". Opus does (or soon
> will) have a way of specifying that, namely mapping_family==2.
> 
> 
> > if the channel_layout is not set its undefined, if its set it should
> > be correct
> > why do you need this additional user parameter ?
> 
> The additional parameter is necessary to disambiguate between multiple
> cases which are all unknown to FFmpeg. For example, there is no way
> for FFmpeg to differentiate between inputs which consist of Ambisonics
> and inputs which have truly no semantic meaning. These two cases are
> differentiated by Opus, with mapping_family 2 and 255 respectively.
> 
> 
> Also the way this patch is written, mapping_family==-1 preserves
> existing FFmpeg behavior while mapping_family==1 does not. Although
> both values indicate the same semantic channel meanings, the latter
> configures libopus to use surround masking, which is a potentially
> quality-degrading change from current behavior. That is why I did not
> change default behavior with this patch.

patches applied

thanks

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libopusenc: Add channel mapping family argument

[Michael Graczyk]

Michael,
Thanks for your comments.

On Sat, Jun 25, 2016 at 4:28 AM, Michael Niedermayer
 wrote:
> yes, rereading this a bit
> can you explain what is the difference between the mapping familiy
> and he channel_layout we have ?

The Opus channel mapping family and FFmpeg channel layout both provide
a mapping between stream position and semantic meaning. That is, they
both answer the question "What does the channel at position k in the
stream contain?"

The two parameters are both defined by explicitly enumerating
acceptable possibilities. There are some channel mappings which are
defined by Opus but are not defined by FFmpeg. Likewise, there are
channel mappings defined by FFmpeg which are not defined by Opus.
Because of the differences, there is no unambiguous 1:1 correspondence
between Opus channel mapping family and FFmpeg channel_layout. For
example, FFmpeg has no way of specifying "channel k is an SN3D
normalized ambisonic channel kth in ACN order". Opus does (or soon
will) have a way of specifying that, namely mapping_family==2.


> if the channel_layout is not set its undefined, if its set it should
> be correct
> why do you need this additional user parameter ?

The additional parameter is necessary to disambiguate between multiple
cases which are all unknown to FFmpeg. For example, there is no way
for FFmpeg to differentiate between inputs which consist of Ambisonics
and inputs which have truly no semantic meaning. These two cases are
differentiated by Opus, with mapping_family 2 and 255 respectively.


Also the way this patch is written, mapping_family==-1 preserves
existing FFmpeg behavior while mapping_family==1 does not. Although
both values indicate the same semantic channel meanings, the latter
configures libopus to use surround masking, which is a potentially
quality-degrading change from current behavior. That is why I did not
change default behavior with this patch.

-- 

Thanks,
Michael Graczyk
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libopusenc: Add channel mapping family argument

[Michael Niedermayer]

On Fri, Jun 24, 2016 at 03:14:42PM -0700, Michael Graczyk wrote:
> I fixed a comment typo and slightly modified an error message in the second
> patch. I attached updated versions. Please let me know if you have any
> thoughts on these patches.

yes, rereading this a bit
can you explain what is the difference between the mapping familiy
and he channel_layout we have ?

if the channel_layout is not set its undefined, if its set it should
be correct
why do you need this additional user parameter ?

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libopusenc: Add channel mapping family argument

[Michael Graczyk]

I fixed a comment typo and slightly modified an error message in the second
patch. I attached updated versions. Please let me know if you have any
thoughts on these patches.

On Mon, Jun 20, 2016 at 3:48 PM, Michael Graczyk 
wrote:

> Hello,
>
> I have fixed the problems Mark pointed out and attached new patches.
> I'll address Mark's comments inline.
>
>
> > This implies that -mapping_family -1 (the default) is the same as some
> > other -mapping_family 0..255, determined by the input, but it is not.
>
> I updated the doc to explain these differences.
>
>
> > This removes the restrictions on input channel layouts, allowing any
> > channel layout to be accepted.
>
> I added explicit checks in the new function
> libopus_check_vorbis_layout for this.
>
>
> > When channels <= 2, the mapping family will still be 0 even if
> -mapping_family 255 was explicitly specified.
>
> Thanks, fixed.
>
>
>
> > If there are more than 8 channels it appears that it will read beyond
> the end of the array.  If the input does not have a family 1 layout
> and mapping family 255 was specified, it will still remap the channels
> as if they had a family 1 layout.
>
> That's right, this was incorrect but now fixed. I only reorder the
> channels when necessary and I explicitly check that the input channel
> layout is the expected one.
>



-- 

Thanks,
Michael Graczyk
From ae55f62df04dd9585681cbd9f770d556cf4047ec Mon Sep 17 00:00:00 2001
From: Michael Graczyk 
Date: Tue, 14 Jun 2016 18:30:36 -0700
Subject: [PATCH 1/2] libopusenc: Refactor to simplify forthcoming
 mapping_family parameter

---
 libavcodec/libopusenc.c | 79 +++--
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/libavcodec/libopusenc.c b/libavcodec/libopusenc.c
index 3f3e80d..c9b96ce 100644
--- a/libavcodec/libopusenc.c
+++ b/libavcodec/libopusenc.c
@@ -79,6 +79,7 @@ static const uint8_t libavcodec_libopus_channel_map[8][8] = {
 
 static void libopus_write_header(AVCodecContext *avctx, int stream_count,
  int coupled_stream_count,
+ int mapping_family,
  const uint8_t *channel_mapping)
 {
 uint8_t *p   = avctx->extradata;
@@ -93,7 +94,7 @@ static void libopus_write_header(AVCodecContext *avctx, int stream_count,
 
 /* Channel mapping */
 if (channels > 2) {
-bytestream_put_byte(, channels <= 8 ? 1 : 255);
+bytestream_put_byte(, mapping_family);
 bytestream_put_byte(, stream_count);
 bytestream_put_byte(, coupled_stream_count);
 bytestream_put_buffer(, channel_mapping, channels);
@@ -159,37 +160,11 @@ static int libopus_configure_encoder(AVCodecContext *avctx, OpusMSEncoder *enc,
 static av_cold int libopus_encode_init(AVCodecContext *avctx)
 {
 LibopusEncContext *opus = avctx->priv_data;
-const uint8_t *channel_mapping;
 OpusMSEncoder *enc;
+uint8_t libopus_channel_mapping[255];
 int ret = OPUS_OK;
 int coupled_stream_count, header_size, frame_size;
 
-coupled_stream_count = opus_coupled_streams[avctx->channels - 1];
-opus->stream_count   = avctx->channels - coupled_stream_count;
-channel_mapping  = libavcodec_libopus_channel_map[avctx->channels - 1];
-
-/* FIXME: Opus can handle up to 255 channels. However, the mapping for
- * anything greater than 8 is undefined. */
-if (avctx->channels > 8) {
-av_log(avctx, AV_LOG_ERROR,
-   "Channel layout undefined for %d channels.\n", avctx->channels);
-return AVERROR_PATCHWELCOME;
-}
-if (!avctx->bit_rate) {
-/* Sane default copied from opusenc */
-avctx->bit_rate = 64000 * opus->stream_count +
-  32000 * coupled_stream_count;
-av_log(avctx, AV_LOG_WARNING,
-   "No bit rate set. Defaulting to %"PRId64" bps.\n", (int64_t)avctx->bit_rate);
-}
-
-if (avctx->bit_rate < 500 || avctx->bit_rate > 256000 * avctx->channels) {
-av_log(avctx, AV_LOG_ERROR, "The bit rate %"PRId64" bps is unsupported. "
-   "Please choose a value between 500 and %d.\n", (int64_t)avctx->bit_rate,
-   256000 * avctx->channels);
-return AVERROR(EINVAL);
-}
-
 frame_size = opus->opts.frame_duration * 48000 / 1000;
 switch (frame_size) {
 case 120:
@@ -251,17 +226,49 @@ static av_cold int libopus_encode_init(AVCodecContext *avctx)
 }
 }
 
-enc = opus_multistream_encoder_create(avctx->sample_rate, avctx->channels,
-  opus->stream_count,
-  coupled_stream_count,
-  channel_mapping,
-  opus->opts.application, );
+/* FIXME: Opus can handle up to 255 channels. However, the mapping for
+ * anything greater than 8 is undefined. */
+

Re: [FFmpeg-devel] [PATCH] libopusenc: Add channel mapping family argument

[Michael Graczyk]

Mark,

Thanks for the comments and for pointing out the problems with the
code. I am working on fixing these and will send an updated patch
shortly.

As for the difference in behavior between mapping_family == -1 and
mapping_family == 1, I wanted to preserve existing behavior for users
who do not specify the new mapping_family command line argument. The
code would be more simple if the default behavior were the same as the
behavior when mapping_family==1 is specified explicitly. Do you think
we should change the default behavior in this case for consistency and
simplicity?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libopusenc: Add channel mapping family argument

[Michael Graczyk]

Michael,

Thanks for the input. I have refactored the patch into two patches. The
first refactors to make the second patch simpler. I have attached both new
patches.

As for the approach, I can see why this may not be an ideal approach. I
based this patch on a discussion I had on #ffmpeg-devel a month or so ago.
It seemed that the consensus then was that I should add a mapping_family
command line parameter as I have done in this patch. However, it was not
absolutely clear that this would be the best approach.

Do you have an alternative in mind? What do you mean by "passed through API
like side data"?

Thanks,
Michael
From 52c16ba12344f1c461a7e0b8484361020039e419 Mon Sep 17 00:00:00 2001
From: Michael Graczyk 
Date: Tue, 14 Jun 2016 18:30:36 -0700
Subject: [PATCH 1/2] libopusenc: Refactor to simplify forthcoming
 mapping_family parameter

---
 libavcodec/libopusenc.c | 79 +++--
 1 file changed, 43 insertions(+), 36 deletions(-)

diff --git a/libavcodec/libopusenc.c b/libavcodec/libopusenc.c
index 3f3e80d..c9b96ce 100644
--- a/libavcodec/libopusenc.c
+++ b/libavcodec/libopusenc.c
@@ -79,6 +79,7 @@ static const uint8_t libavcodec_libopus_channel_map[8][8] = {
 
 static void libopus_write_header(AVCodecContext *avctx, int stream_count,
  int coupled_stream_count,
+ int mapping_family,
  const uint8_t *channel_mapping)
 {
 uint8_t *p   = avctx->extradata;
@@ -93,7 +94,7 @@ static void libopus_write_header(AVCodecContext *avctx, int 
stream_count,
 
 /* Channel mapping */
 if (channels > 2) {
-bytestream_put_byte(, channels <= 8 ? 1 : 255);
+bytestream_put_byte(, mapping_family);
 bytestream_put_byte(, stream_count);
 bytestream_put_byte(, coupled_stream_count);
 bytestream_put_buffer(, channel_mapping, channels);
@@ -159,37 +160,11 @@ static int libopus_configure_encoder(AVCodecContext 
*avctx, OpusMSEncoder *enc,
 static av_cold int libopus_encode_init(AVCodecContext *avctx)
 {
 LibopusEncContext *opus = avctx->priv_data;
-const uint8_t *channel_mapping;
 OpusMSEncoder *enc;
+uint8_t libopus_channel_mapping[255];
 int ret = OPUS_OK;
 int coupled_stream_count, header_size, frame_size;
 
-coupled_stream_count = opus_coupled_streams[avctx->channels - 1];
-opus->stream_count   = avctx->channels - coupled_stream_count;
-channel_mapping  = libavcodec_libopus_channel_map[avctx->channels - 1];
-
-/* FIXME: Opus can handle up to 255 channels. However, the mapping for
- * anything greater than 8 is undefined. */
-if (avctx->channels > 8) {
-av_log(avctx, AV_LOG_ERROR,
-   "Channel layout undefined for %d channels.\n", avctx->channels);
-return AVERROR_PATCHWELCOME;
-}
-if (!avctx->bit_rate) {
-/* Sane default copied from opusenc */
-avctx->bit_rate = 64000 * opus->stream_count +
-  32000 * coupled_stream_count;
-av_log(avctx, AV_LOG_WARNING,
-   "No bit rate set. Defaulting to %"PRId64" bps.\n", 
(int64_t)avctx->bit_rate);
-}
-
-if (avctx->bit_rate < 500 || avctx->bit_rate > 256000 * avctx->channels) {
-av_log(avctx, AV_LOG_ERROR, "The bit rate %"PRId64" bps is 
unsupported. "
-   "Please choose a value between 500 and %d.\n", 
(int64_t)avctx->bit_rate,
-   256000 * avctx->channels);
-return AVERROR(EINVAL);
-}
-
 frame_size = opus->opts.frame_duration * 48000 / 1000;
 switch (frame_size) {
 case 120:
@@ -251,17 +226,49 @@ static av_cold int libopus_encode_init(AVCodecContext 
*avctx)
 }
 }
 
-enc = opus_multistream_encoder_create(avctx->sample_rate, avctx->channels,
-  opus->stream_count,
-  coupled_stream_count,
-  channel_mapping,
-  opus->opts.application, );
+/* FIXME: Opus can handle up to 255 channels. However, the mapping for
+ * anything greater than 8 is undefined. */
+if (avctx->channels > 8) {
+av_log(avctx, AV_LOG_ERROR,
+   "Channel layout undefined for %d channels.\n", avctx->channels);
+return AVERROR_PATCHWELCOME;
+}
+
+coupled_stream_count = opus_coupled_streams[avctx->channels - 1];
+opus->stream_count   = avctx->channels - coupled_stream_count;
+
+memcpy(libopus_channel_mapping,
+   opus_vorbis_channel_map[avctx->channels - 1],
+   avctx->channels * sizeof(*libopus_channel_mapping));
+
+enc = opus_multistream_encoder_create(
+avctx->sample_rate, avctx->channels, opus->stream_count,
+coupled_stream_count,
+libavcodec_libopus_channel_map[avctx->channels - 1],
+opus->opts.application, );
+
 if (ret != 

Re: [FFmpeg-devel] [PATCH] libopusenc: Add channel mapping family argument

[Michael Niedermayer]

On Fri, Jun 10, 2016 at 10:39:23PM -0700, Michael Graczyk wrote:
> The libopus encoder has a parameter called "channel mapping family" which
> is used to specify inter-channel relationships for the purpose of encoding.
> I have added a new command line argument which makes it possible to forward
> the mapping family parameter along to libopus.
> 
> With this parameter, it is possible to choose between encoding multichannel
> audio streams as surround sound (with interchannel masking and channel
> coupling) versus independent channels (no interchannel masking nor channel
> coupling).
> 
> Example usage:
> 
> $ wget https://samples.ffmpeg.org/A-codecs/wavpcm/8_Channel_ID.wav -O in.wav
> 
> # Use the old behavior. Header contains layout, but no masking
> $ ./ffmpeg -y -i in.wav -c:a opus -mapping_family -1 out.ogg
> 
> # Use libopus surround mode. Masking + automatic channel coupling
> $ ./ffmpeg -y -i in.wav -c:a opus -mapping_family 1 out.ogg
> 
> # Use libopus with independent channels. No header info, no masking, no
> coupling
> $ ./ffmpeg -y -i in.wav -c:a opus -mapping_family 255 out.ogg
> 
> This patch also makes it possible to encode up to 254 channels with opus
> using channel mapping family 255.
> 
> $ head -c 1728000 /dev/urandom > noise.raw
> $ ./ffmpeg -y -f s16le -ar 48000 -ac 18 -i noise.raw -c:a opus
> -mapping_family 255 out.opus
> 
> 
> Questions:
> 
> 1. I had to remove .channel_layouts form ff_libopus_encoder to allow the
> encoder to accept streams with more than 8 channels. Is that the right way
> to extend the encoder? Based on a discussion on #ffmpeg it seemed like
> removing the .channel_layouts field would be the only way to allow more
> than 16 channels.

> 2. Am I using AVFrame.data correctly? I recall reading somewhere that
> channels after the eighth would be stored in extended_data, but from the
> documentation it seems like that is only the case for planar data.

extended_data is for more than AV_NUM_DATA_POINTERS planar channels,
yes

it seems this patch moves some code around, can that be split into
a seperate patch so its more clearly vissible what is changed.

Also iam not sure people will like the aprouch taken by this patch
or prefer the mapping_family to be passed through API like side data
and be preserved from input to output

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libopusenc: Add channel mapping family argument

[Michael Graczyk]

Has anybody had a chance to look at this patch? Any thoughts or ways I can
make it easier to look over?

On Fri, Jun 10, 2016 at 10:39 PM, Michael Graczyk 
wrote:

> The libopus encoder has a parameter called "channel mapping family" which
> is used to specify inter-channel relationships for the purpose of encoding.
> I have added a new command line argument which makes it possible to forward
> the mapping family parameter along to libopus.
>
> With this parameter, it is possible to choose between encoding
> multichannel audio streams as surround sound (with interchannel masking and
> channel coupling) versus independent channels (no interchannel masking nor
> channel coupling).
>
> Example usage:
>
> $ wget https://samples.ffmpeg.org/A-codecs/wavpcm/8_Channel_ID.wav -O
> in.wav
>
> # Use the old behavior. Header contains layout, but no masking
> $ ./ffmpeg -y -i in.wav -c:a opus -mapping_family -1 out.ogg
>
> # Use libopus surround mode. Masking + automatic channel coupling
> $ ./ffmpeg -y -i in.wav -c:a opus -mapping_family 1 out.ogg
>
> # Use libopus with independent channels. No header info, no masking, no
> coupling
> $ ./ffmpeg -y -i in.wav -c:a opus -mapping_family 255 out.ogg
>
> This patch also makes it possible to encode up to 254 channels with opus
> using channel mapping family 255.
>
> $ head -c 1728000 /dev/urandom > noise.raw
> $ ./ffmpeg -y -f s16le -ar 48000 -ac 18 -i noise.raw -c:a opus
> -mapping_family 255 out.opus
>
>
> Questions:
>
> 1. I had to remove .channel_layouts form ff_libopus_encoder to allow the
> encoder to accept streams with more than 8 channels. Is that the right way
> to extend the encoder? Based on a discussion on #ffmpeg it seemed like
> removing the .channel_layouts field would be the only way to allow more
> than 16 channels.
> 2. Am I using AVFrame.data correctly? I recall reading somewhere that
> channels after the eighth would be stored in extended_data, but from the
> documentation it seems like that is only the case for planar data.
>
>
> Thanks for reading,
> Michael Graczyk
>



-- 

Thanks,
Michael Graczyk
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel