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

adding configurable target bitrate #11

Merged
merged 5 commits into from
Apr 29, 2015

Conversation

lepinsk
Copy link

@lepinsk lepinsk commented Apr 28, 2015

I've tried to stick close to the coding style used throughout the project.

Rather than fall back on a default target bitrate in recorder.js, if new Recorder is initialized without a bitrate key in the configuration object, the bitrate simply isn't set (which means the encoder goes with its default). I figured this was the best course of action, given that the encoder has different default bitrates depending on the selected encoder application.

@chris-rudmin
Copy link
Owner

Thanks for the great contribution! Can I suggest a few changes before I merge it?

  • Be consistent with camel casing (bitRateLocation instead of bitrateLocation)
  • Add the bitRate option on the recordOpus config object instead of the main config object as it will have no effect on Wav recordings
  • Document the config option in the readme

@lepinsk
Copy link
Author

lepinsk commented Apr 28, 2015

Sure thing. Good catches.

* moved bitRate inside recordOpus
* added documentation to README.md
* fixed casing for bitRateLocation
@lepinsk
Copy link
Author

lepinsk commented Apr 29, 2015

That should do the trick.

@chris-rudmin
Copy link
Owner

Thanks, after those changes the example.html page stops working. Do you think we really need it in the example?

Edit, the example is working. My bad

@lepinsk
Copy link
Author

lepinsk commented Apr 29, 2015

If you want me to drop it from the example I'm happy to do so. Just lemme know.

chris-rudmin added a commit that referenced this pull request Apr 29, 2015
Adding configurable target bitrate
@chris-rudmin chris-rudmin merged commit 66eaf31 into chris-rudmin:master Apr 29, 2015
@lepinsk
Copy link
Author

lepinsk commented Apr 29, 2015

I've got one more commit where I remove the configurable bitrate from the example.html page – feel free to pull in lepinsk/Recorderjs@8527797 if you want it.

@chris-rudmin
Copy link
Owner

Nah, lets keep it.

On 28 April 2015 at 20:14, Julian Lepinski notifications@github.com wrote:

I've got one more commit where I remove the configurable bitrate from the
example.html page – feel free to pull in lepinsk/Recorderjs@8527797
lepinsk@8527797
if you want it.


Reply to this email directly or view it on GitHub
#11 (comment)
.

@lepinsk
Copy link
Author

lepinsk commented Apr 29, 2015

Sounds good – for the record, I was a bit confused by the dual use of recordOpus as bool / config object (which is probably why I left bitrate out of it when I was first testing things). Perhaps having it in the example will make that part more clear.

@chris-rudmin
Copy link
Owner

Yes, good point, I agree the dual bool/config is not clear. For future versions, I might drop the wav support. Most people who are interested in this project seem to be interested for the opus encoding.

chris-rudmin added a commit that referenced this pull request Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants