Just to confirm, I would use opeint_* for all the OpusGenericEncoder-related functions? On Tue, Mar 20, 2018 at 8:38 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> Hi Mark, Drew, > > On 03/20/2018 02:40 AM, Mark Harris wrote: > > + int _oge_use_projection(int channel_mapping); > > > > These functions are part of libopusenc, so I'd expect them to have an > > ope prefix like the other functions in the libopusenc library. > > I'd like to avoid using the ope_ prefix for functions that's aren't in > the public API. Right now there are other functions with a leading > underscore, so we'll have to fix them as well (not in this patch of > course). Maybe an "opeint_" prefix would do the job here (unless anyone > has a better idea)? > > > +int ope_encoder_deferred_init_with_mapping(OggOpusEnc *enc, int > > family, int streams, > > int coupled_streams, const unsigned char *mapping) { > > int ret; > > int i; > > > > This code is allowing family 253 for a deferred init, but does not > > create a projection encoder in that case, so it looks like it will > > fail when writing the id header since it won't be able to get the > > demixing matrix. It should probably not be allowing family 253 here. > > Actually, in the case of ope_encoder_deferred_init_with_mapping(), I > think it's probably best to just keep the existing code (not use > wrappers), since that function cannot be used by the projection encoder > at all. > > Jean-Marc > > > > - Mark > > > > > > On Mon, Mar 19, 2018 at 3:00 PM, Drew Allen <bitllama at google.com> wrote: > >> Hi Jean-Marc, > >> > >> I've modified my patches for libopus and libopusenc based on your > >> suggestions. > >> > >> Cheers, > >> Drew > >> > >> On Mon, Mar 19, 2018 at 2:05 PM Jean-Marc Valin <jmvalin at jmvalin.ca> > wrote: > >>> > >>> Hi Drew, > >>> > >>> I think the libopusenc patch is better, but there's still a few issues > >>> left: > >>> 1) The static MAX_PACKET_BUFFER_SIZE value is still problematic because > >>> if you link libopusenc with a new version of libopus that supports > >>> higher order projection or just more projection channels for order 3, > >>> then you will overflow the buffer. I think what you'd want is a > >>> _ope_opus_header_get_size() call that would return how large the header > >>> *actually* is. Then you can use that value instead of > >>> MAX_PACKET_BUFFER_SIZE in init_stream() > >>> 2) I think the remaining if()s in ope_encoder_ctl() can also be removed > >>> by adding another ctl() macro (like _oge_ctl()) with an extra argument. > >>> In the case of OPUS_MULTISTREAM_GET_ENCODER_STATE_REQUEST, you can > >>> simply use _oge_ctl(enc->st, > >>> OPUS_MULTISTREAM_GET_ENCODER_STATE(stream_id, value)) > >>> 3) On libopus itself, why "#define OPUS_HAVE_OPUS_PROJECTION_H 9000" > >>> instead of just "#define OPUS_HAVE_OPUS_PROJECTION_H"? > >>> > >>> Cheers, > >>> > >>> Jean-Marc > >>> > >>> On 03/19/2018 02:53 PM, Drew Allen wrote: > >>>> > >>>> On Mon, Mar 19, 2018 at 11:52 AM Drew Allen <bitllama at google.com > >>>> <mailto: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 > >>>> > >>>> > >>>> > >>>> _______________________________________________ > >>>> opus mailing list > >>>> opus at xiph.org > >>>> http://lists.xiph.org/mailman/listinfo/opus > >>>> > >> > >> > >> _______________________________________________ > >> opus mailing list > >> opus at xiph.org > >> http://lists.xiph.org/mailman/listinfo/opus > >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20180320/18c2de74/attachment.html>
On 03/20/2018 11:51 AM, Drew Allen wrote:> Just to confirm, I would use opeint_* for all the > OpusGenericEncoder-related functions?Correct. Or you can also use oge_ if you like. Just don't use something that starts with an underscore.> On Tue, Mar 20, 2018 at 8:38 AM Jean-Marc Valin <jmvalin at jmvalin.ca > <mailto:jmvalin at jmvalin.ca>> wrote: > > Hi Mark, Drew, > > On 03/20/2018 02:40 AM, Mark Harris wrote: > > + int _oge_use_projection(int channel_mapping); > > > > These functions are part of libopusenc, so I'd expect them to have an > > ope prefix like the other functions in the libopusenc library. > > I'd like to avoid using the ope_ prefix for functions that's aren't in > the public API. Right now there are other functions with a leading > underscore, so we'll have to fix them as well (not in this patch of > course). Maybe an "opeint_" prefix would do the job here (unless anyone > has a better idea)? > > > +int ope_encoder_deferred_init_with_mapping(OggOpusEnc *enc, int > > family, int streams, > > int coupled_streams, const unsigned char *mapping) { > > int ret; > > int i; > > > > This code is allowing family 253 for a deferred init, but does not > > create a projection encoder in that case, so it looks like it will > > fail when writing the id header since it won't be able to get the > > demixing matrix. It should probably not be allowing family 253 here. > > Actually, in the case of ope_encoder_deferred_init_with_mapping(), I > think it's probably best to just keep the existing code (not use > wrappers), since that function cannot be used by the projection encoder > at all. > > Jean-Marc > > > > - Mark > > > > > > On Mon, Mar 19, 2018 at 3:00 PM, Drew Allen <bitllama at google.com > <mailto:bitllama at google.com>> wrote: > >> Hi Jean-Marc, > >> > >> I've modified my patches for libopus and libopusenc based on your > >> suggestions. > >> > >> Cheers, > >> Drew > >> > >> On Mon, Mar 19, 2018 at 2:05 PM Jean-Marc Valin > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> wrote: > >>> > >>> Hi Drew, > >>> > >>> I think the libopusenc patch is better, but there's still a few > issues > >>> left: > >>> 1) The static MAX_PACKET_BUFFER_SIZE value is still problematic > because > >>> if you link libopusenc with a new version of libopus that supports > >>> higher order projection or just more projection channels for > order 3, > >>> then you will overflow the buffer. I think what you'd want is a > >>> _ope_opus_header_get_size() call that would return how large the > header > >>> *actually* is. Then you can use that value instead of > >>> MAX_PACKET_BUFFER_SIZE in init_stream() > >>> 2) I think the remaining if()s in ope_encoder_ctl() can also be > removed > >>> by adding another ctl() macro (like _oge_ctl()) with an extra > argument. > >>> In the case of OPUS_MULTISTREAM_GET_ENCODER_STATE_REQUEST, you can > >>> simply use _oge_ctl(enc->st, > >>> OPUS_MULTISTREAM_GET_ENCODER_STATE(stream_id, value)) > >>> 3) On libopus itself, why "#define OPUS_HAVE_OPUS_PROJECTION_H 9000" > >>> instead of just "#define OPUS_HAVE_OPUS_PROJECTION_H"? > >>> > >>> Cheers, > >>> > >>> Jean-Marc > >>> > >>> On 03/19/2018 02:53 PM, Drew Allen wrote: > >>>> > >>>> On Mon, Mar 19, 2018 at 11:52 AM Drew Allen > <bitllama at google.com <mailto:bitllama at google.com> > >>>> <mailto:bitllama at google.com <mailto: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 > >>>> > >>>> > >>>> > >>>> _______________________________________________ > >>>> opus mailing list > >>>> opus at xiph.org <mailto:opus at xiph.org> > >>>> http://lists.xiph.org/mailman/listinfo/opus > >>>> > >> > >> > >> _______________________________________________ > >> opus mailing list > >> opus at xiph.org <mailto:opus at xiph.org> > >> http://lists.xiph.org/mailman/listinfo/opus > >> >
Attached is an updated patch based on Jean-Marc and Mark's comments. :) Cheers, Drew On Tue, Mar 20, 2018 at 9:20 AM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> On 03/20/2018 11:51 AM, Drew Allen wrote: > > Just to confirm, I would use opeint_* for all the > > OpusGenericEncoder-related functions? > > Correct. Or you can also use oge_ if you like. Just don't use something > that starts with an underscore. > > > > On Tue, Mar 20, 2018 at 8:38 AM Jean-Marc Valin <jmvalin at jmvalin.ca > > <mailto:jmvalin at jmvalin.ca>> wrote: > > > > Hi Mark, Drew, > > > > On 03/20/2018 02:40 AM, Mark Harris wrote: > > > + int _oge_use_projection(int channel_mapping); > > > > > > These functions are part of libopusenc, so I'd expect them to have > an > > > ope prefix like the other functions in the libopusenc library. > > > > I'd like to avoid using the ope_ prefix for functions that's aren't > in > > the public API. Right now there are other functions with a leading > > underscore, so we'll have to fix them as well (not in this patch of > > course). Maybe an "opeint_" prefix would do the job here (unless > anyone > > has a better idea)? > > > > > +int ope_encoder_deferred_init_with_mapping(OggOpusEnc *enc, int > > > family, int streams, > > > int coupled_streams, const unsigned char *mapping) { > > > int ret; > > > int i; > > > > > > This code is allowing family 253 for a deferred init, but does not > > > create a projection encoder in that case, so it looks like it will > > > fail when writing the id header since it won't be able to get the > > > demixing matrix. It should probably not be allowing family 253 > here. > > > > Actually, in the case of ope_encoder_deferred_init_with_mapping(), I > > think it's probably best to just keep the existing code (not use > > wrappers), since that function cannot be used by the projection > encoder > > at all. > > > > Jean-Marc > > > > > > > - Mark > > > > > > > > > On Mon, Mar 19, 2018 at 3:00 PM, Drew Allen <bitllama at google.com > > <mailto:bitllama at google.com>> wrote: > > >> Hi Jean-Marc, > > >> > > >> I've modified my patches for libopus and libopusenc based on your > > >> suggestions. > > >> > > >> Cheers, > > >> Drew > > >> > > >> On Mon, Mar 19, 2018 at 2:05 PM Jean-Marc Valin > > <jmvalin at jmvalin.ca <mailto:jmvalin at jmvalin.ca>> wrote: > > >>> > > >>> Hi Drew, > > >>> > > >>> I think the libopusenc patch is better, but there's still a few > > issues > > >>> left: > > >>> 1) The static MAX_PACKET_BUFFER_SIZE value is still problematic > > because > > >>> if you link libopusenc with a new version of libopus that > supports > > >>> higher order projection or just more projection channels for > > order 3, > > >>> then you will overflow the buffer. I think what you'd want is a > > >>> _ope_opus_header_get_size() call that would return how large the > > header > > >>> *actually* is. Then you can use that value instead of > > >>> MAX_PACKET_BUFFER_SIZE in init_stream() > > >>> 2) I think the remaining if()s in ope_encoder_ctl() can also be > > removed > > >>> by adding another ctl() macro (like _oge_ctl()) with an extra > > argument. > > >>> In the case of OPUS_MULTISTREAM_GET_ENCODER_STATE_REQUEST, you > can > > >>> simply use _oge_ctl(enc->st, > > >>> OPUS_MULTISTREAM_GET_ENCODER_STATE(stream_id, value)) > > >>> 3) On libopus itself, why "#define OPUS_HAVE_OPUS_PROJECTION_H > 9000" > > >>> instead of just "#define OPUS_HAVE_OPUS_PROJECTION_H"? > > >>> > > >>> Cheers, > > >>> > > >>> Jean-Marc > > >>> > > >>> On 03/19/2018 02:53 PM, Drew Allen wrote: > > >>>> > > >>>> On Mon, Mar 19, 2018 at 11:52 AM Drew Allen > > <bitllama at google.com <mailto:bitllama at google.com> > > >>>> <mailto:bitllama at google.com <mailto: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 > > >>>> > > >>>> > > >>>> > > >>>> _______________________________________________ > > >>>> opus mailing list > > >>>> opus at xiph.org <mailto:opus at xiph.org> > > >>>> http://lists.xiph.org/mailman/listinfo/opus > > >>>> > > >> > > >> > > >> _______________________________________________ > > >> opus mailing list > > >> opus at xiph.org <mailto:opus at xiph.org> > > >> http://lists.xiph.org/mailman/listinfo/opus > > >> > > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xiph.org/pipermail/opus/attachments/20180320/2131faf5/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: libopusenc-Support-for-Ambisonics.patch Type: text/x-patch Size: 17918 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20180320/2131faf5/attachment-0001.bin>