input sample rate should not be used as the decoder rate

[chris-rudmin]

https://github.com/Rantanen/node-opus/blob/master/lib/Decoder.js#L57
The sample rate defined in the header here is strictly informational and should not be used to determine the decoder sample rate (nor encoder). See https://wiki.xiph.org/OggOpus#ID_Header

[chris-rudmin]

I'm not sure if in node there is a way to determine the device sample rate and implement the logic as specified in the xiph wiki:

An Ogg Opus player SHOULD select the playback sample rate according to the following procedure: If the hardware supports 48 kHz playback, decode at 48 kHz, else if the hardware's highest available sample rate is a supported rate, decode at this sample rate, else if the hardware's highest available sample rate is less than 48 kHz, decode at the next higher supported rate and resample, else decode at 48 kHz and resample.

[rotem925]

That's indeed an issue,
Also, when you emit the format event. You should not emit the original sample rate, but the encoded sample rate.

[Rantanen]

Pushed #21 to npm, so this should be fixed.

[Rantanen]

(Oh, and thanks for reporting it!)

[rotem925]

Hi,
The latest version in npm is still 0.2.2..

On Nov 7, 2016 1:29 PM, "Mikko Rantanen" notifications@github.com wrote:

Pushed #21 #21 to npm, so
this should be fixed.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#20 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPMQjxitfpMQCpXN0GC3UkkW6PgO-G6ks5q7wumgaJpZM4Kq3D5
.

[Rantanen]

Oh dammit.

The publish hadn't gone through for some reason or another and I didn't actually confirm the message. Published now again - and this time I actually read what npm publish said in the end. :)

[rotem925]

Hehe,
Looks good now 😃 thanks!

On Nov 7, 2016 8:21 PM, "Mikko Rantanen" notifications@github.com wrote:

Oh dammit.

The publish hadn't gone through for some reason or another and I didn't
actually confirm the message. Published now again - and this time I
actually read what npm publish said in the end. :)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#20 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPMQuM0aWxqmluSPhqhWkmUg0z3c3O7ks5q72w8gaJpZM4Kq3D5
.

[rotem925]

Hi,
version 0.2.3 still contains the previous version.
Can you please make sure you release the latest master branch?

Thanks,
Rotem

On 7 Nov 2016, at 20:23, Rotem rotem925@gmail.com wrote:

Hehe,
Looks good now 😃 thanks!

On Nov 7, 2016 8:21 PM, "Mikko Rantanen" <notifications@github.com mailto:notifications@github.com> wrote:
Oh dammit.

The publish hadn't gone through for some reason or another and I didn't actually confirm the message. Published now again - and this time I actually read what npm publish said in the end. :)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub #20 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAPMQuM0aWxqmluSPhqhWkmUg0z3c3O7ks5q72w8gaJpZM4Kq3D5.

[Rantanen]

Oh crap. So sorry for that!

Doing a git rebase really shouldn't have been that hard. For some reason I completely forgot that, being all satisfied with just fetching the merge.

Okay it finally should be there. At least npm install node-opus gave me a version with the following changes in:

        this.emit( 'format', {
        channels: this.channels,
        sampleRate: this.rate,
...
        this.encoder = new OpusEncoder( this.rate, this.channels );

.. and it even seemed to pull that off npm instead of using some pre-existing npm link 😒

[rotem925]

And it works great now!!!

Thank you!

Rotem

On 8 Nov 2016, at 13:58, Mikko Rantanen notifications@github.com wrote:

Oh crap. So sorry for that!

Doing a git rebase really shouldn't have been that hard. For some reason I completely forgot that, being all satisfied with just fetching the merge.

Okay it finally should be there. At least npm install node-opus gave me a version with the following changes in:

    this.emit( 'format', {
    channels: this.channels,
    sampleRate: this.rate,

...
this.encoder = new OpusEncoder( this.rate, this.channels );
.. and it even seemed to pull that off npm instead of using some pre-existing npm link 😒

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub #20 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAPMQmcnLXrFInh69Dr-hALpxErYkonfks5q8GP0gaJpZM4Kq3D5.

[Rantanen]

Glad to hear. Sorry for the delays again!