[opus] [Patch] Non-diegetic support for channel mapping 254

[Jean-Marc Valin]

Hi Drew,

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) ||
!validate_encoder_layout(&st->layout))
+   if (!validate_layout(&st->layout) ||
+       (mapping_type == MAPPING_TYPE_SURROUND &&
+       !validate_encoder_layout(&st->layout)))

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.

Cheers,

	Jean-Marc


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,
> 
> Cheers,
> Drew Allen (from the Chrome Media Audio team)
> IRC (#opus): llama_of_bits
> 
> 
> _______________________________________________
> opus mailing list
> opus at xiph.org
> http://lists.xiph.org/mailman/listinfo/opus
>