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