Hi Drew,
Some comments on the libopusenc patch:
+ 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.
+ if (_oge_use_projection(h->channel_mapping))
+ {
+ len=27+(h->channels*(h->nb_streams+h->nb_coupled)*2);
+ }
+ else
+ {
+ len=27+h->channels;
+ }
The fixed part of the header is 19 bytes; where does the 27 come from here?
+struct OpusGenericEncoder {
+ OpusMSEncoder *ms;
+#ifdef OPUS_HAVE_OPUS_PROJECTION_H
+ OpusProjectionEncoder *pr;
+#endif
+ int family;
+};
opus_multistream.h needs to be included for OpusMSEncoder.
opus_projection.h can't be relied upon to include it since
opus_projection.h may not be available.
+OpusGenericEncoder *_oge_surround_create(
+ int Fs, int channels, int channel_mapping, int *nb_streams,
+ int *nb_coupled, unsigned char *stream_map, int application,
+ int *ret) {
+ OpusGenericEncoder *st;
+ st = malloc(sizeof(*st));
+#ifdef OPUS_HAVE_OPUS_PROJECTION_H
+ if(_oge_use_projection(channel_mapping)){
+ int ci;
+ st->pr=opus_projection_ambisonics_encoder_create(Fs, channels,
+ channel_mapping, nb_streams, nb_coupled, application, ret);
+ for (ci = 0; ci < channels; ci++) {
+ stream_map[ci] = ci;
+ }
+ }
+ else
+#endif
+ {
+ st->ms=opus_multistream_surround_encoder_create(Fs, channels,
+ channel_mapping, nb_streams, nb_coupled, stream_map, application, ret);
+ }
+ st->family=channel_mapping;
+ return st;
+}
This will crash if malloc() fails. The caller checks for st == NULL,
but that is too late and its intent is to check whether
opus_*_encoder_create() returned NULL; if st == NULL it expects the
error code to be filled in with the error from
opus_*_encoder_create(). This function does NOT return NULL as
expected by the caller if opus_*_encoder_create() fails.
Rather than allocating this OpusGenericEncoder structure separately
and pointing to it from the OggOpusEnc structure, this structure or
its two pointer fields could simply be added to OggOpusEnc. This
would also avoid an unnecessary level of indirection in every place
where the encoder is used.
+ st->family=0;
The family field in this structure is set to the wrong family in
_oge_create(). However the real family is in the OggOpusEnc
structure; the family field in this structure is not actually used
except to compare the value to 253 to determine whether to use the
"ms" or "pr" pointer. It would be simpler to omit the
incorrectly-set
family field and just check whether the corresponding pointer is
non-NULL.
-int ope_encoder_deferred_init_with_mapping(OggOpusEnc *enc, int
family, int streams,
+int ope_encoder_deferred_init_with_mapping(OggOpusEnc *enc, int
family, int streams,
int coupled_streams, const unsigned char *mapping) {
int ret;
int i;
- OpusMSEncoder *st;
+ OpusGenericEncoder *st;
if (enc->st!=NULL) {
return OPE_TOO_LATE;
}
if (family < 0 || family > 255) return OPE_BAD_ARG;
- else if (family != 1 && family != 255) return OPE_UNIMPLEMENTED;
+ else if (family != 1 &&
+ #ifdef OPUS_HAVE_OPUS_PROJECTION_H
+ family != 253 && family != 254 &&
+ #endif
+ family != 255) return OPE_UNIMPLEMENTED;
else if (streams <= 0 || streams>255 || coupled_streams<0 ||
coupled_streams >= 128 || streams+coupled_streams > 255) return
OPE_BAD_ARG;
- st=opus_multistream_encoder_create(48000, enc->channels, streams,
coupled_streams, mapping, OPUS_APPLICATION_AUDIO, &ret);
+ st=_oge_create(48000, enc->channels, streams, coupled_streams,
mapping, OPUS_APPLICATION_AUDIO, &ret);
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.
- 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
>
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 >>
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>