Thanks! 2 down, 2 to go. :D :D :D On Wed, Mar 21, 2018 at 11:19 PM Jean-Marc Valin <jmvalin at jmvalin.ca> wrote:> Thanks, the libopus and the libopusenc patches are now merged. > > Cheers, > > Jean-Marc > > On 03/20/2018 12:36 PM, Drew Allen wrote: > > 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 > > <mailto: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> > > > <mailto: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> > > > <mailto: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> > > <mailto: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>> > > > >>>> <mailto: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> > > <mailto: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> <mailto: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/20180322/6c77981c/attachment.html>
Hello all! I've attached an updated patch for opusfile, based on formatting suggestions I got for the opus and libopusenc patches. Cheers! Drew On Thu, Mar 22, 2018 at 9:19 AM Drew Allen <bitllama at google.com> wrote:> Thanks! 2 down, 2 to go. :D :D :D > > On Wed, Mar 21, 2018 at 11:19 PM Jean-Marc Valin <jmvalin at jmvalin.ca> > wrote: > >> Thanks, the libopus and the libopusenc patches are now merged. >> >> Cheers, >> >> Jean-Marc >> >> On 03/20/2018 12:36 PM, Drew Allen wrote: >> > 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 >> > <mailto: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> >> > > <mailto: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> >> > > <mailto: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> >> > <mailto: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>> >> > > >>>> <mailto: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> >> > <mailto: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> <mailto: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/20180326/2de6ff0d/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: opusfile-Support-Ambisonics.patch Type: text/x-patch Size: 20897 bytes Desc: not available URL: <http://lists.xiph.org/pipermail/opus/attachments/20180326/2de6ff0d/attachment-0001.bin>
Apologies! That patch will not work correctly with Opus 1.2. Will send an updated patch shortly!! On Mon, Mar 26, 2018 at 11:56 AM Drew Allen <bitllama at google.com> wrote:> Hello all! > > I've attached an updated patch for opusfile, based on formatting > suggestions I got for the opus and libopusenc patches. > > Cheers! > Drew > > On Thu, Mar 22, 2018 at 9:19 AM Drew Allen <bitllama at google.com> wrote: > >> Thanks! 2 down, 2 to go. :D :D :D >> >> On Wed, Mar 21, 2018 at 11:19 PM Jean-Marc Valin <jmvalin at jmvalin.ca> >> wrote: >> >>> Thanks, the libopus and the libopusenc patches are now merged. >>> >>> Cheers, >>> >>> Jean-Marc >>> >>> On 03/20/2018 12:36 PM, Drew Allen wrote: >>> > 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 >>> > <mailto: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> >>> > > <mailto: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> >>> > > <mailto: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> >>> > <mailto: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>> >>> > > >>>> <mailto: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> >>> > <mailto: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> <mailto: >>> 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/20180326/846cd737/attachment.html>