[opus] [Patch] Non-diegetic support for channel mapping 254
jmvalin at jmvalin.ca
Fri Apr 7 20:21:59 UTC 2017
Thakns for the patch. Here's some comments for now (not done reviewing):
1) You want to use isqrt32() rather than celt_sqrt(), since celt_sqrt()
changes behaviour for fixed-point and provides no guarantee about rounding.
2) About these two lines:
+ if (nondiegetic_channels && 2 != nondiegetic_channels)
+ return OPUS_UNIMPLEMENTED;
Why not just have the condition be:
+ if (nondiegetic_channels != 2)
As a minor detail, I prefer N != 2 than 2 != N.
Also, unless you think it may one day make sense to have
nondiegetic_channels != 2, I think you should just return OPUS_BAD_ARG.
OPUS_UNIMPLEMENTED is used for things that may one day be implemented.
Also, many sure you catch all non-sensical values. For example, what if
channels is set to 1000?
3) From the draft, it seems like you can only have two nondiegetic
channels, so instead of:
+ nb_streams = acn_channels + ((nondiegetic_channels / 2) % 2);
+ nb_coupled_streams = nondiegetic_channels / 2;
Why not just have:
+ nb_streams = acn_channels + nondiegetic_channels;
+ nb_coupled_streams = nondiegetic_channels != 0;
4) About this change:
- if (!validate_layout(&st->layout) ||
+ if (!validate_layout(&st->layout) ||
+ (mapping_type == MAPPING_TYPE_SURROUND &&
Is there nothing to validate for ambisonics?
5) For opus_multistream_surround_encoder_init(), the same comments as
for opus_multistream_surround_encoder_get_size() maybe some code can be
shared (if possible)?
6) ambisonics_rate_allocation() again duplicates some of the code from
above. That should be avoided if possible.
I haven't gone through the details of the rate allocation yet, but I
thought I'd give you these comments already.
On 07/04/17 11:26 AM, Drew Allen wrote:
> Hello all,
> Attached is a proposed patch for Opus that allows support for encoding
> non-diegetic stereo audio as a coupled stream for use with channel
> mapping 254. It also rejects channel counts that are not (n+1)^2 + 2j,
> where n is 0 to 14 and j is either 0 or 1 (See IETF public draft doc
> attached for clarification).
> Please let me know any suggestions / concerns / comments.
> Thank you for your time,
> Drew Allen (from the Chrome Media Audio team)
> IRC (#opus): llama_of_bits
> opus mailing list
> opus at xiph.org
More information about the opus