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

added feature of manually setting "BUFFERSIZE" in amplitude analysis #419

Closed
wants to merge 2 commits into from

Conversation

endurance21
Copy link
Collaborator

@endurance21 endurance21 commented Mar 9, 2020

solves #417

not only feature is added but also example have been updated !

solved first p5-sound js

areas of impact
examples/amplitude_analysis/sketch.js
lib/p5.sound.js
lib/p5.sound.js.map
lib/p5.sound.min.js
lib/p5.sound.min.js.map
src/amplitude.js

@endurance21 endurance21 changed the title Amplitude added feature of manually setting "BUFFERSIZE" of FFT data used in amplitude analysis Mar 9, 2020
@endurance21
Copy link
Collaborator Author

hey @therewasaguy !
after exploring a lot and playing around with P5.js-sound library , i pointed out a new feature and
have tried to implement the same!
please have a look!
thanks!

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

As far as new features go, a key decision from the recent p5 Contributors Conference is that "p5.js will not add any new features except those that increase access" (more here)

So let's keep in mind that as p5 contributors. New features add complexity and we should focus on making the existing features as stable and accessible as can be.

That said, if this new feature came with a unit test, that would help :)

src/amplitude.js Show resolved Hide resolved
@endurance21
Copy link
Collaborator Author

endurance21 commented Mar 16, 2020

@therewasaguy
It must be power of 2, thank you for pointing out 😅. I will be correcting it.
more accurately
" Its value can be a power of 2 value in the range 256–16384. " , source MDN /web Audio Api

@endurance21 endurance21 changed the title added feature of manually setting "BUFFERSIZE" of FFT data used in amplitude analysis added feature of manually setting "BUFFERSIZE" in amplitude analysis Mar 16, 2020
@@ -4,13 +4,15 @@

var soundFile;
var amplitude;
var bufferSize = 2048; // a value which can be multiple of 2 , eg. 256 , 512 , 1024 , 2048 ..etc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this comment should be updated to read "power of 2"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya ofcourse !!


// Set to 2048 for now. In future iterations, this should be inherited or parsed from p5sound's default
this.bufferSize = safeBufferSize(2048);
this.bufferSize = safeBufferSize(bufferSize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment above this line should be deleted or updated. I'm curious if you considered the other options mentioned in the comment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@therewasaguy
actually comment must be modified 😄
and
as far as inheritance part is concerned i think , i need to modify p5.sound pass a optional argument "bufferSize " like i did in here , but right now let's leave it and i will make it another pr !
what do you say ?

*/

p5.Amplitude.prototype.setBufferSize = function(s) {
if (s%2==0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a good practice to use triple equals so that we check for type as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually i have modified it in my local repo , i will be pushing it with your recommendations keeping in mind 😃

@endurance21
Copy link
Collaborator Author

endurance21 commented Mar 25, 2020

@therewasaguy
I have read the w3 specs about web audio api and here is what i get
The bufferSize parameter determines the buffer size in units of sample-frames. If it's not passed in, or if the value is 0, then the implementation will choose the best buffer size for the given environment, which will be constant power of 2 throughout the lifetime of the node. Otherwise if the author explicitly specifies the bufferSize, it must be one of the following values: 256, 512, 1024, 2048, 4096, 8192, 16384. This value controls how frequently the audioprocess event is dispatched and how many sample-frames need to be processed each call. Lower values for bufferSize will result in a lower (better) latency. Higher values will be necessary to avoid audio breakup and glitches. It is recommended for authors to not specify this buffer size and allow the implementation to pick a good buffer size to balance between latency and audio quality
But i think p5 user barely cares about how frequently onaudioprocess task is dispatched and
Neither i find any glitches with or without setting bufferSize
Thus keeping present requirement in mind, i think we should be more focused on building our library more robust with documentations and caring more about our current api's.
(as you said)

@therewasaguy
Copy link
Member

hey @endurance21, I've added a card for this to our GSoC backlog. It would need resolving of merge conflicts and the changes mentioned above, but before doing that...it sounds like we're on the same page that this feature isn't as much of a priority right now. How do you feel about closing this so that we can focus?

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.

2 participants