<div dir="ltr"><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg">Hi Jean-Marc et all,<div class="gmail_msg"><br class="gmail_msg"></div><div class="gmail_msg">Thanks for the quick feedback, responses to your questions are below:<br class="gmail_msg"><br><div class="gmail_quote gmail_msg"></div></div><div class="gmail_msg">I've attached a revised patch. PTAL, thanks!</div><div class="gmail_msg"><br></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Fri, Apr 7, 2017 at 1:22 PM Jean-Marc Valin <<a href="mailto:jmvalin@jmvalin.ca" class="gmail_msg" target="_blank">jmvalin@jmvalin.ca</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Drew,<br class="gmail_msg">
<br class="gmail_msg">
Thakns for the patch. Here's some comments for now (not done reviewing):<br class="gmail_msg">
<br class="gmail_msg">
1) You want to use isqrt32() rather than celt_sqrt(), since celt_sqrt()<br class="gmail_msg">
changes behaviour for fixed-point and provides no guarantee about rounding.<br class="gmail_msg"></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> Thanks! I'll make that change.</div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
2) About these two lines:<br class="gmail_msg">
+      if (nondiegetic_channels && 2 != nondiegetic_channels)<br class="gmail_msg">
+         return OPUS_UNIMPLEMENTED;<br class="gmail_msg">
<br class="gmail_msg">
Why not just have the condition be:<br class="gmail_msg">
+      if (nondiegetic_channels != 2)<br class="gmail_msg"></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">We should allow either 0 or 2 nondiegetic channels.</div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
As a minor detail, I prefer N != 2 than 2 != N.<br class="gmail_msg"></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">Acknowledged.</div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, unless you think it may one day make sense to have<br class="gmail_msg">
nondiegetic_channels != 2, I think you should just return OPUS_BAD_ARG.<br class="gmail_msg">
OPUS_UNIMPLEMENTED is used for things that may one day be implemented.</blockquote></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> <br class="gmail_msg">Done.</div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, many sure you catch all non-sensical values. For example, what if<br class="gmail_msg">
channels is set to 1000?<br class="gmail_msg"></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">Done.</div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
3) From the draft, it seems like you can only have two nondiegetic<br class="gmail_msg">
channels, so instead of:<br class="gmail_msg">
+      nb_streams = acn_channels + ((nondiegetic_channels / 2) % 2);<br class="gmail_msg">
+      nb_coupled_streams = nondiegetic_channels / 2;<br class="gmail_msg">
<br class="gmail_msg">
Why not just have:<br class="gmail_msg">
+      nb_streams = acn_channels + nondiegetic_channels;<br class="gmail_msg">
+      nb_coupled_streams = nondiegetic_channels != 0;<br class="gmail_msg">
<br class="gmail_msg"></blockquote></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">Done<br class="gmail_msg"> </div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
4) About this change:<br class="gmail_msg">
-   if (!validate_layout(&st->layout) ||<br class="gmail_msg">
!validate_encoder_layout(&st->layout))<br class="gmail_msg">
+   if (!validate_layout(&st->layout) ||<br class="gmail_msg">
+       (mapping_type == MAPPING_TYPE_SURROUND &&<br class="gmail_msg">
+       !validate_encoder_layout(&st->layout)))<br class="gmail_msg">
<br class="gmail_msg">
Is there nothing to validate for ambisonics?<br class="gmail_msg">
<br class="gmail_msg"></blockquote></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">We do this when determine the (n+1)^2 + 2j listed above, but we can validate that here as well.</div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
5) For opus_multistream_surround_encoder_init(), the same comments as<br class="gmail_msg">
for opus_multistream_surround_encoder_get_size() maybe some code can be<br class="gmail_msg">
shared (if possible)?<br class="gmail_msg">
<br class="gmail_msg">
6) ambisonics_rate_allocation() again duplicates some of the code from<br class="gmail_msg">
above. That should be avoided if possible.<br class="gmail_msg">
<br class="gmail_msg"></blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">Agreed, this is smelly. I will make a new function and call it from all these places.</div></div></div></div></div><div dir="ltr" class="gmail_msg"><div dir="ltr" class="gmail_msg"><div class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I haven't gone through the details of the rate allocation yet, but I<br class="gmail_msg">
thought I'd give you these comments already.<br class="gmail_msg">
<br class="gmail_msg">
Cheers,<br class="gmail_msg">
<br class="gmail_msg">
        Jean-Marc<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
On 07/04/17 11:26 AM, Drew Allen wrote:<br class="gmail_msg">
> Hello all,<br class="gmail_msg">
><br class="gmail_msg">
> Attached is a proposed patch for Opus that allows support for encoding<br class="gmail_msg">
> non-diegetic stereo audio as a coupled stream for use with channel<br class="gmail_msg">
> mapping 254. It also rejects channel counts that are not (n+1)^2 + 2j,<br class="gmail_msg">
> where n is 0 to 14 and j is either 0 or 1 (See IETF public draft doc<br class="gmail_msg">
> attached for clarification).<br class="gmail_msg">
><br class="gmail_msg">
> Please let me know any suggestions / concerns / comments.<br class="gmail_msg">
><br class="gmail_msg">
> Thank you for your time,<br class="gmail_msg">
><br class="gmail_msg">
> Cheers,<br class="gmail_msg">
> Drew Allen (from the Chrome Media Audio team)<br class="gmail_msg">
> IRC (#opus): llama_of_bits<br class="gmail_msg">
><br class="gmail_msg">
><br class="gmail_msg">
> _______________________________________________<br class="gmail_msg">
> opus mailing list<br class="gmail_msg">
> <a href="mailto:opus@xiph.org" class="gmail_msg" target="_blank">opus@xiph.org</a><br class="gmail_msg">
> <a href="http://lists.xiph.org/mailman/listinfo/opus" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.xiph.org/mailman/listinfo/opus</a><br class="gmail_msg">
><br class="gmail_msg">
</blockquote></div></div></div></div></div>