Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.0 - Custom Binding #44

Open
wants to merge 97 commits into
base: master
Choose a base branch
from
Open

2.0 - Custom Binding #44

wants to merge 97 commits into from

Conversation

vectrixdevelops
Copy link

@vectrixdevelops vectrixdevelops commented Jun 23, 2017

This implements an entire remake of the current audio-speaker API with a custom binding. The aim is to make this a more maintained version for the AudioJS ecosystem. This uses the pre-built binding options first and if that does not exist for your OS version it will attempt to build it manually. This also contains a browser version that will be used if installed with the --no-optional flag.

This is a work in progress.

@vectrixdevelops
Copy link
Author

I think resolving mpg123 errors could come in the NAPI branch? @jamen @dfcreative

@dy
Copy link
Member

dy commented Jun 25, 2017

@jamen @connorhartley hey I think I am stuck with the thing - I try to output float32 samples to audio-mpg123 - and I get silence. Things went pearshape after introducing pcm-convert, although by itself module works exactly same way as pcm-util/convert, but redesigned to simpler and faster API.

Some disclaimer. There is dtype package, where we can indicate data types way easier as just a string eg. uint8, and that became a standard practice in regl and ndarray. So I think that we could be using a format property here to indicate format as such dtype string. But turns out that messes up output. Although the sine wave I had before the introducing pcm-convert was no better.

I will try to dig deeper what's the problem.

UPD. Figured out that in some reason we have to change endianness from 'le' to 'be' before feeding the data.

@dy
Copy link
Member

dy commented Jun 25, 2017

@connorhartley @jamen ok, hear proper sound now 🎉

Spotted some bugs though.

  • if we set format to anything but int16, we get noises
  • if we set channels: 2 for the generator/speaker, we get high-pitched sine.

@jamen
Copy link
Contributor

jamen commented Jun 25, 2017

@dfcreative I tried to run the tests. Could u publish pcm-convert@1.0.1, and then add it in here because it is missing, I was also missing buffer-to-arraybuffer.

@jamen
Copy link
Contributor

jamen commented Jun 25, 2017

Otherwise it is playing good for me too. 😄

@dy
Copy link
Member

dy commented Jun 25, 2017

@jamen ok I would make sure there is no bugs outputting any type of any format before publishing (if it is possible). Gonna work tomorrow a bit on that.

@dy
Copy link
Member

dy commented Jun 25, 2017

@jamen @connorhartley we are almost ready to merge. A couple of things to solve.

  1. Now seems like audio-mpg123 is incapable of rendering anything but int16 format, because changing it to any other breaks normal sound output. Should we convert any data type internally to int16 and forget about other formats? Same thing we do with existing audio-speaker impl. Although that would be nice to make audio-mpg123 produce sound from other formats.
  2. If we are going to use internal conversion to int16, we may want to simplify options. Specifically, we don't have to define format at all, since we are feeding only audio-buffers.
  3. There is a bug with chunkSize: right now it is NaN, and if we try to make it a number, we get really bad sound. Help needed.
  4. Not sure I completely understand the meaning of autoFlush - can we elaborate that in readme?

@jamen
Copy link
Contributor

jamen commented Jun 25, 2017

  • I don't mind audio-speaker only accepting AudioBuffers at the moment. But I do agree, it would be nice if the backend could accept different buffer/arraybuffer formats for not only its purposes here, so I would say definitely support that as we continue to make the backend more user-friendly.
  • That usage is actually pretty appealing to me. I took a look at the readme and it looks quite a bit more minimal now. Thanks to the 2.0 signature, dtype, and then now finding the format isn't working.
  • I'm personally not a fan of autoFlush being in the options. I think it should just be implied by this being the official speaker package, because it provides a good default behavior IMO... Whereas in audio-mpg123, you can flush/open/close/etc. all manually yourself for a more raw, non-streamy way to interact with the speaker. I talked with @connorhartley though and he thought it would be good to at least keep it in, so maybe he has some ideas. 😄

@dy
Copy link
Member

dy commented Jun 25, 2017

@jamen no, I am afraid I don't understand not the reason for it to be, but the meaning of that. What does it do, in short? What is the difference between flush and write? (looking with the eyes of user)
I hope it is not leaking API?

@jamen
Copy link
Contributor

jamen commented Jun 25, 2017

@dfcreative It calls out123_drain on the binding after doing out123_play, which will properly drain between writes. This might have more adverse affects on larger buffers compared to streamed ones (I could be wrong). But, try commenting all the occurences of mpg123.flush out, and run the tests a few times, see if the playback is buggier than before. It was for me: the noise barely played, lena snippet is shorter than with, and the sine was garbled at the beginning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants