Friendly ping for the opus-tools patch... ---------- Forwarded message --------- From: Drew Allen <bitllama at google.com> Date: Mon, Mar 19, 2018 at 2:53 PM Subject: Re: [PATCH] Support for Ambisonics To: opus at xiph.org <opus at xiph.org> On Mon, Mar 19, 2018 at 11:52 AM Drew Allen <bitllama at google.com> wrote:> Hello all, > > Sorry for the delay (got really sick last week). > > Attached are updated patches for libopus, libopusenc, opusfile and > opus-tools. > > Note that the patches for libopusenc, opusfile and opus-tools are > dependent on the patch for libopus. > > Please let me know if you have any additional followup comments or > questions. > > Cheers, > Drew >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20180730/1d63ab74/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: opusfile-Support-for-Ambisonics.patch Type: application/x-patch Size: 20945 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20180730/1d63ab74/attachment-0003.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: libopusenc-Support-for-Ambisonics.patch Type: application/x-patch Size: 17291 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20180730/1d63ab74/attachment-0004.bin> -------------- next part -------------- A non-text attachment was scrubbed... Name: opus-tools-Support-for-Ambisonics.patch Type: application/x-patch Size: 13236 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20180730/1d63ab74/attachment-0005.bin>
Hi Drew, Sorry for the delay. FYI the patch that you attached is not your latest version. This thread that you replied to is an older thread; the latest version is on a different thread. The patch does not use the mapping family numbers used by libopus and libopusenc; could you update it to use family 2 and 3 rather than 254 and 253? The patch also appears to break encoding of surround files and non-Ambisonic files with more than 8 channels. It will attempt to use mapping family 0 but that is only valid for mono and stereo. In this patch, an additional use_permute argument is added to a number of functions that indicates whether to perform channel permutation. It would be preferable to simply add this as a field in the oe_enc_opt structure, which is already an argument and already contains other options like this. When I brought this up in the past you made a patch that did that, however your latest patches are still using the additional argument. Can you please drop the additional argument and incorporate your earlier patch that uses the struct field? That should simplify this patch. Requiring the user to specify internal Ogg Opus mapping family numbers may be okay for a quick hack to experiment with ambisonics, but doesn't seem like the best solution for adding official support for ambisonics in opus-tools, which is intended for use by end users. Ideally no additional options would be needed and opusenc would be able to identify a WAV file containing Ambix-compatible ambisonic channels and by default convert such a file to a compressed ambisonic output file in the format described by draft-ietf-codec-ambisonics. However it appears that there is unfortunately no agreed upon metadata to identify such WAV files, and the user will need a way to manually override the WAV file's metadata to indicate that the input channels are ambisonic channels. If the user will have to manually specify some command line option, it seems like an option that explicitly overrides the meaning of the input channels, like "--channels ambix" to specify that the input channels are Ambix-compatible channels (ACN order, SN3D normalization) would be preferable to overriding the mapping family number of the output. Besides being cryptic, setting a mapping family number directly is more likely to mislead the user into thinking that the option affects only the mapping family number that is written in the output file and that the input file and encoding is otherwise unaffected. This is of course false; for example the normal permutation of the input channels, coupling, and bitrate allocation will not be performed if it is known that the input channels are ambisonic channels. An option that overrides the meaning of the input channels would also allow a future version to support other conventions if needed, e.g. "--channels fuma" to convert ambisonic channels from FuMa format, or "--channels ambix,stereo" / "--channels stereo,ambix" to convert an input file with non-diegetic stereo either after or before the ambisonic channels, or to force a particular surround configuration, or force discrete unpermuted channels that would otherwise be permuted and treated as a surround configuration. Such an option would also allow the existing --downmix-mono and --downmix-stereo options to be supported for ambisonic input, in case the user did not actually want it encoded as ambisonics. (There is no need to support these in the first version, however if not supported it should report an error if these options are used with ambisonic input, and should not downmix as if the channels were surround channels.) In theory those options could be used with an option that specifies ambisonics input using a mapping family number, but that would be especially confusing because nowhere would it actually write that mapping family number; it would be used only to determine the meaning of the input channels. Can you explain in terms that an end user would understand how they should choose between mapping family 2 and 3? Ideally the user should not have to know the difference or be made to choose between them. Is there a reason to not use mapping family 3 for order 1, 2, and 3, and mapping family 2 otherwise? If it is a user option the user should be given some guidance. The opusenc manual page should also document any other new options. Finally, Google's IPR disclosure at https://datatracker.ietf.org/ipr/3200/ discloses that Google has applied for a patent related to this technology and will grant a "Royalty-Free, Reasonable and Non-Discriminatory License to All Implementers". Since this would be an implementation of this specification it is my understanding that Google expects the implementation to be bound by the terms of its patent license. For Opus itself, the relevant disclosures include the full text of the patent license directly in the IPR disclosure, and the Opus repository contains a file that links to each of them: https://gitlab.xiph.org/xiph/opus/raw/master/LICENSE_PLEASE_READ.txt If the full text of the patent license can be added to Google's IPR disclosure then a similar file can be created for opus-tools. Alternatively the full text of the patent license could be included directly in the file as part of the patch. Thanks, - Mark On Mon, Jul 30, 2018 at 11:40 AM, Andrew Allen <bitllama at google.com> wrote:> Friendly ping for the opus-tools patch... > > > ---------- Forwarded message --------- > From: Drew Allen <bitllama at google.com> > Date: Mon, Mar 19, 2018 at 2:53 PM > Subject: Re: [PATCH] Support for Ambisonics > To: opus at xiph.org <opus at xiph.org> > > > > On Mon, Mar 19, 2018 at 11:52 AM Drew Allen <bitllama at google.com> wrote: >> >> Hello all, >> >> Sorry for the delay (got really sick last week). >> >> Attached are updated patches for libopus, libopusenc, opusfile and >> opus-tools. >> >> Note that the patches for libopusenc, opusfile and opus-tools are >> dependent on the patch for libopus. >> >> Please let me know if you have any additional followup comments or >> questions. >> >> Cheers, >> Drew
Since the opusenc and opusinfo changes were independent I split them up and landed the opusinfo changes (with updated mapping family numbers). - Mark On Thu, Sep 6, 2018 at 4:22 AM, Mark Harris <mark.hsj at gmail.com> wrote:> Hi Drew, > > Sorry for the delay. > > FYI the patch that you attached is not your latest version. This > thread that you replied to is an older thread; the latest version is > on a different thread. > > The patch does not use the mapping family numbers used by libopus and > libopusenc; could you update it to use family 2 and 3 rather than 254 > and 253? > > The patch also appears to break encoding of surround files and > non-Ambisonic files with more than 8 channels. It will attempt to use > mapping family 0 but that is only valid for mono and stereo. > > In this patch, an additional use_permute argument is added to a number > of functions that indicates whether to perform channel permutation. > It would be preferable to simply add this as a field in the oe_enc_opt > structure, which is already an argument and already contains other > options like this. When I brought this up in the past you made a > patch that did that, however your latest patches are still using the > additional argument. Can you please drop the additional argument and > incorporate your earlier patch that uses the struct field? That > should simplify this patch. > > Requiring the user to specify internal Ogg Opus mapping family numbers > may be okay for a quick hack to experiment with ambisonics, but > doesn't seem like the best solution for adding official support for > ambisonics in opus-tools, which is intended for use by end users. > Ideally no additional options would be needed and opusenc would be > able to identify a WAV file containing Ambix-compatible ambisonic > channels and by default convert such a file to a compressed ambisonic > output file in the format described by draft-ietf-codec-ambisonics. > However it appears that there is unfortunately no agreed upon metadata > to identify such WAV files, and the user will need a way to manually > override the WAV file's metadata to indicate that the input channels > are ambisonic channels. > > If the user will have to manually specify some command line option, it > seems like an option that explicitly overrides the meaning of the > input channels, like "--channels ambix" to specify that the input > channels are Ambix-compatible channels (ACN order, SN3D normalization) > would be preferable to overriding the mapping family number of the > output. Besides being cryptic, setting a mapping family number > directly is more likely to mislead the user into thinking that the > option affects only the mapping family number that is written in the > output file and that the input file and encoding is otherwise > unaffected. This is of course false; for example the normal > permutation of the input channels, coupling, and bitrate allocation > will not be performed if it is known that the input channels are > ambisonic channels. > > An option that overrides the meaning of the input channels would also > allow a future version to support other conventions if needed, e.g. > "--channels fuma" to convert ambisonic channels from FuMa format, or > "--channels ambix,stereo" / "--channels stereo,ambix" to convert an > input file with non-diegetic stereo either after or before the > ambisonic channels, or to force a particular surround configuration, > or force discrete unpermuted channels that would otherwise be permuted > and treated as a surround configuration. > > Such an option would also allow the existing --downmix-mono and > --downmix-stereo options to be supported for ambisonic input, in case > the user did not actually want it encoded as ambisonics. (There is no > need to support these in the first version, however if not supported > it should report an error if these options are used with ambisonic > input, and should not downmix as if the channels were surround > channels.) In theory those options could be used with an option that > specifies ambisonics input using a mapping family number, but that > would be especially confusing because nowhere would it actually write > that mapping family number; it would be used only to determine the > meaning of the input channels. > > Can you explain in terms that an end user would understand how they > should choose between mapping family 2 and 3? Ideally the user should > not have to know the difference or be made to choose between them. Is > there a reason to not use mapping family 3 for order 1, 2, and 3, and > mapping family 2 otherwise? If it is a user option the user should be > given some guidance. > > The opusenc manual page should also document any other new options. > > Finally, Google's IPR disclosure at > https://datatracker.ietf.org/ipr/3200/ discloses that Google has > applied for a patent related to this technology and will grant a > "Royalty-Free, Reasonable and Non-Discriminatory License to All > Implementers". Since this would be an implementation of this > specification it is my understanding that Google expects the > implementation to be bound by the terms of its patent license. For > Opus itself, the relevant disclosures include the full text of the > patent license directly in the IPR disclosure, and the Opus repository > contains a file that links to each of them: > https://gitlab.xiph.org/xiph/opus/raw/master/LICENSE_PLEASE_READ.txt > If the full text of the patent license can be added to Google's IPR > disclosure then a similar file can be created for opus-tools. > Alternatively the full text of the patent license could be included > directly in the file as part of the patch. > > Thanks, > - Mark > > > On Mon, Jul 30, 2018 at 11:40 AM, Andrew Allen <bitllama at google.com> wrote: >> Friendly ping for the opus-tools patch... >> >> >> ---------- Forwarded message --------- >> From: Drew Allen <bitllama at google.com> >> Date: Mon, Mar 19, 2018 at 2:53 PM >> Subject: Re: [PATCH] Support for Ambisonics >> To: opus at xiph.org <opus at xiph.org> >> >> >> >> On Mon, Mar 19, 2018 at 11:52 AM Drew Allen <bitllama at google.com> wrote: >>> >>> Hello all, >>> >>> Sorry for the delay (got really sick last week). >>> >>> Attached are updated patches for libopus, libopusenc, opusfile and >>> opus-tools. >>> >>> Note that the patches for libopusenc, opusfile and opus-tools are >>> dependent on the patch for libopus. >>> >>> Please let me know if you have any additional followup comments or >>> questions. >>> >>> Cheers, >>> Drew