<div dir="ltr">Excellent, thanks Tim for writing back so quickly!<div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Cheers,<div>Drew</div></div></div></div>
<br><div class="gmail_quote">On Tue, Mar 6, 2018 at 6:25 PM, Timothy B. Terriberry <span dir="ltr"><<a href="mailto:tterribe@xiph.org" target="_blank">tterribe@xiph.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Drew Allen wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Please feel free to ask any questions or give any feedback you might<br>
have. :)<br>
</blockquote>
<br></span>
A few comments from a quick glance through the opusfile patch:<br>
<br>
You unconditionally #include <opus_projection.h> in the public header, but you haven't updated the libopus version requirement in <a href="http://configure.ac" rel="noreferrer" target="_blank">configure.ac</a>. Ideally, of course, libopusfile would continue to work with older versions of libopus, just with a reduced feature set. The best would be if libopus itself provided a #define that its users could check at compile time to see if the projection API was available. See the definition of OP_SOFT_CLIP in opusfile's src/internal.h for the hacks I've had to do to detect other libopus API additions (hopefully we can avoid things like that here).<br></blockquote><div>I think this is relatively easy to add to libopus so I'll wrap the function calls and the #include around this #define. This will also allow us to not have to explicitly enable ambisonics. :) <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The addition of fields to the OpusHead struct is a breaking ABI change (specifically, all callers of opus_head_parse() would need to be rebuilt from source).<br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Is OPUS_DEMIXING_MATRIX_SIZE_MAX future-proof? I seem to recall much larger matrices being allowed according to the spec. You'll note that OPUS_CHANNEL_COUNT_MAX is 255, even though opusfile currently only supports mappings with up to 8 channels (see the private OP_NCHANNELS_MAX used internally, for comparison). The idea was that I did not want to break ABI when we added support for new mappings.<br>
<br></blockquote><div>I will change the define to make it future-proof as well as make it a separate struct (OpusHeadExt, for example) and make the call the opus_head_parse_ext() in order not to break the ABI.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
One way to go about this without breaking ABI might be to add support for "extended" mapping data. I.e., we could create an opus_head_parse_ext() that takes an extra buffer to return the extended mapping data into. Since it looks like you're not parsing this matrix anyway, but returning it as a chunk of binary data, hopefully this wouldn't be too unergonomic. The limit on the caller's buffer size could be the limit imposed by the size of an Ogg page. That should be future-proof.<br>
<br>
I'm not sure if it would make sense to have the existing opus_head_parse() work for the ambisonics mapping families, or just fail cleanly with an error. I'm not sure what you could usefully do without the matrix (specifically, what would you put in the mapping[] array that existing callers would do something sane with?).<br></blockquote><div>I would think that it's not a good idea to try to decode the streams without handling the demixing matrix, so I would lean towards a graceful fail in that case. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Similarly, changing the argument to op_decode_cb_func also breaks ABI. One way to avoid this might be to add an op_set_projection_decode_callb<wbr>ack() function, which takes a new callback function pointer that takes the OpusProjectionDecoder instead. Then an application would register both callbacks, and libopusfile would call the correct one for the current link in the file. If you have an ambisonics link, but no projection callback registered, then it simply wouldn't call a decode callback for packets in that link. That behavior might be a little surprising to existing applications, but since previously libopusfile couldn't decode those links at all, hopefully it wouldn't be too bad.<br>
<br></blockquote><div>Make sense and an easy fix. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
There are a few more style consistency issues (sort system includes alphabetically, always use braces for if bodies, unless the if clause and the body both fit on the same line, prefix function names with op_ instead of _). In general, try to follow the local style.<br>
<br></blockquote><div>I'll take note of these suggestions and cleanup the style accordingly. :)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I haven't reviewed the implementation changes in detail, but let's work out the high-level things first.<br>
</blockquote></div><br></div><div class="gmail_extra">Thanks again Tim, I'll send over another patch once all of these have been addressed.</div></div>