<div dir="ltr">Hello Tim et all,<div><br></div><div>Attached is a revised patch for opusfile which more gracefully addresses the listed concerns. Note that this patch is dependent on the most recent patch I sent out for opus (<a href="http://lists.xiph.org/pipermail/opus/2018-March/004146.html">http://lists.xiph.org/pipermail/opus/2018-March/004146.html</a>)</div><div><br></div><div>Awaiting your feedback. :)</div><div><br></div><div>Cheers,</div><div>Drew</div><div><br></div><div><br></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Mar 6, 2018 at 9:14 PM Drew Allen <<a href="mailto:bitllama@google.com">bitllama@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Excellent, thanks Tim for writing back so quickly!<div class="gmail_extra"><br clear="all"><div><div class="m_3085716250451052690gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Cheers,<div>Drew</div></div></div></div>
<br><div class="gmail_quote"></div></div></div><div dir="ltr"><div class="gmail_extra"><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>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></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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_callback() 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></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Make sense and an easy fix. </div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I'll take note of these suggestions and cleanup the style accordingly. :)</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"></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>
</blockquote></div></div>