Fix: Implemented STOP button behavior to toggle audio playback #353

tarunsinghofficial · 2024-03-08T08:21:08Z

The web audio samples lack a stop button functionality, making it difficult for users to pause or stop audio playback once it has started. This limitation hinders the user experience and can be frustrating for individuals testing the demos.

Fix: This PR addresses the issue by implementing a stop button feature in the web audio samples.

Changes Made

Updated build_info.json to reflect changes in the project.
Modified index.njk and main.js files for several audio worklet demos:
- audio-worklet-node-options: Updated options handling for audio worklet nodes.
- hello-audio-worklet: Improved initialization and error handling.
- noise-generator: Enhanced noise generation functionality.
- one-pole-filter: Refinements to the one-pole filter algorithm.
- volume-meter: Improved volume meter visualization and accuracy.
Updated styles.css for better styling and consistency across demos.

Purpose

The purpose of this PR is to ensure that the web audio samples provide a smooth and engaging experience for users. By addressing reported bugs and implementing enhancements, we aim to make the demos more useful and accessible to developers interested in web audio technologies.

Screenshots

4-4f8b-4650-bbdf-f652b23fcba0)

…nfo.json, index.njk, and main.js files for various audio worklet demos. Also, updated styles.css.

google-cla · 2024-03-08T08:21:12Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

hoch

We appreciate your excellent contribution! You scoped the work and created a meaningful PR in a good shape.

My suggestion is more of a directional change for the PR, but I believe it is necessary. Please take a look and let me know if you have more questions.

hoch · 2024-03-13T17:20:50Z

src/audio-worklet/basic/hello-audio-worklet/main.js

@@ -3,24 +3,54 @@
 // found in the LICENSE file.

 const audioContext = new AudioContext();
+let oscillator; 


To make it clear, how about oscillatorNode?

Thank you for the suggestion. I've updated the variable name to oscillatorNode for better clarity. Please let me know if you have any further suggestions.

hoch · 2024-03-13T17:22:55Z

src/audio-worklet/basic/audio-worklet-node-options/main.js

  buttonEl.addEventListener('click', async () => {
-    const waveformType =
+    if (!oscillatorProcessor) { // If audio is not playing, start audio
+      const waveformType =
        document.querySelector('#demo-select-waveform-type').value;


nit: 4-space indent

src/audio-worklet/basic/audio-worklet-node-options/main.js

hoch · 2024-03-13T17:26:14Z

src/audio-worklet/basic/hello-audio-worklet/main.js

@@ -3,24 +3,54 @@
 // found in the LICENSE file.

 const audioContext = new AudioContext();
+let oscillator; 
+let isPlaying = false; 

 const startAudio = async (context) => {
  await context.audioWorklet.addModule('bypass-processor.js');


Ditto here; calling addModule every call of startAudio is not recommended.

Done the requested changes as per the feedback.

hoch · 2024-03-13T17:26:35Z

src/audio-worklet/basic/hello-audio-worklet/main.js

  const bypasser = new AudioWorkletNode(context, 'bypass-processor');
  oscillator.connect(bypasser).connect(context.destination);
  oscillator.start();
 };

+const stopAudio = () => {


We should leave the rest and simply suspend the context.

Thanks for the suggestion. I've modified the stopAudio() function to suspend the audio context instead of stopping and disconnecting the oscillator node. Please review the changes and let me know if they meet the requirements.

This also has the same problem with src/audio-worklet/basic/audio-worklet-node-options/main.js above. See my comments for the explanation.

…tions files

tarunsinghofficial · 2024-03-14T16:59:36Z

src/audio-worklet/basic/hello-audio-worklet/main.js

@@ -3,21 +3,22 @@
 // found in the LICENSE file.

 const audioContext = new AudioContext();
-let oscillator; 
+let oscillatorNode; 


Changed the name to oscillatorNode

tarunsinghofficial · 2024-03-22T15:50:33Z

Any update on the PR?

hoch

Thanks for updating the PR, but I think we have a design problem to solve:
addModule() can only be called once throughout the document lifecycle.

My recommendation is to leave the most of the code intact, and use suspend and resume for starting/stopping the AudioContext.

hoch · 2024-03-22T22:07:05Z

src/audio-worklet/basic/audio-worklet-node-options/main.js

-    audioContext.resume();
-    buttonEl.disabled = true;
-    buttonEl.textContent = 'Playing...';
+    if (!oscillatorProcessor) { // If audio is not playing, start audio


nit: Let's move the comment below the if().

hoch · 2024-03-22T22:10:21Z

src/audio-worklet/basic/audio-worklet-node-options/main.js

-  await context.audioWorklet.addModule('oscillator-processor.js');
-  const oscillatorProcessor =
+  if (!oscillatorProcessor) {
+    await context.audioWorklet.addModule('oscillator-processor.js');


This can't be called multiple times in any scenario. I think you'll get an error if you do this. Have you checked?

Yes, it can't be called multiple times, so I have modified it. No, I didn't get any error though!

src/audio-worklet/basic/audio-worklet-node-options/main.js

hoch · 2024-03-22T22:12:20Z

src/audio-worklet/basic/audio-worklet-node-options/main.js

+      await startAudio(audioContext, { waveformType, frequency });
+      audioContext.resume();
+      buttonEl.textContent = 'STOP';
+    } else { // If audio is playing, stop audio


nit: Ditto on comments.

hoch · 2024-03-22T22:13:37Z

src/audio-worklet/basic/hello-audio-worklet/main.js

  const bypasser = new AudioWorkletNode(context, 'bypass-processor');
  oscillator.connect(bypasser).connect(context.destination);
  oscillator.start();
 };

+const stopAudio = () => {


This also has the same problem with src/audio-worklet/basic/audio-worklet-node-options/main.js above. See my comments for the explanation.

hoch · 2024-03-22T22:14:45Z

src/audio-worklet/basic/noise-generator/main.js

-    buttonEl.disabled = true;
-    buttonEl.textContent = 'Playing...';
-  }, false);
+    if (!isPlaying) { // If not playing, start audio


nit: Google's JS style doesn't use this type of inline comments.

moved the comments to next line.

… noise-generator

tarunsinghofficial · 2024-03-26T14:51:09Z

Hi @hoch! I have made the required changes, let me know if they commit is now goog and ready to be merged.
Thanks

tarunsinghofficial · 2024-04-02T17:01:08Z

Hi @hoch

Any update on this PR? I know that I created this issue under the GSoC tag, but please don't close this PR as it is great improvement in the web audio samples for chrome labs.

Thanks

hoch · 2024-04-02T17:03:49Z

Of course. We'll continue to work on this. The team is currently a bit behind schedule, so reviewing this PR will take some time. Thanks for your patience!

tarunsinghofficial · 2024-04-10T11:25:22Z

Of course. We'll continue to work on this. The team is currently a bit behind schedule, so reviewing this PR will take some time. Thanks for your patience!

Hi @hoch

Any update on this PR?

tarunsinghofficial · 2024-04-17T18:42:35Z

Is this PR still ongoing?

hoch · 2024-04-18T07:56:38Z

Yes. We will review the PR soon. Thanks for your patience.

hoch

@tarunsinghofficial Thanks for your patience, and we appreciate your contribution. The PR still seems to have outstanding issues, and it needs more review and local testing.

src/audio-worklet/basic/audio-worklet-node-options/main.js

hoch · 2024-04-27T00:53:32Z

src/audio-worklet/basic/hello-audio-worklet/main.js

-  }, false);
+    if (!oscillatorNode) {
+      await startAudio(audioContext);
+      audioContext.resume();


Once oscillatorNode is valid, resume() doesn't get called from anywhere. How does this recover from suspended status?

I modified the button click event handler to check if the AudioContext is suspended and call resume() accordingly.

hoch · 2024-04-27T00:56:13Z

src/audio-worklet/basic/noise-generator/main.js

-  }, false);
+    if (!isPlaying) { 
+      // If not playing, start the audio.
+      await startAudio(audioContext);


This will call L10 (addModule) multiple times depending on isPlaying. Have you tested locally? Loading the same script twice should throw an exception.

I have updated the code to ensure that the module is added only once by using a flag (moduleAdded) to track whether the module has already been loaded.

I believe this example is pretty much the same with the other oscillator example. Why do we need a different flag (moduleAdded) only for this example?

hoch · 2024-04-27T00:56:31Z

src/audio-worklet/basic/one-pole-filter/main.js

-    buttonEl.textContent = 'Playing...';
-  }, false);
+    if (!isPlaying) { 
+      await startAudio(audioContext);


Ditto here.

added the flag check here same as above.

hoch · 2024-04-27T00:57:08Z

src/audio-worklet/basic/volume-meter/main.js

-    buttonEl.disabled = true;
-    buttonEl.textContent = 'Playing...';
+    if (!mediaStream) { // If audio is not playing, start audio
+      await startAudio(audioContext, meterEl);


I made the requested changes here:

#362

added the flag check here too.

…let, one-pole-filter, volume-meter, noise-generator

tarunsinghofficial · 2024-05-26T07:06:44Z

Hi @hoch, I hope you're doing well! I have made all the necessary changes, and tested them locally, and as far as I know, it is ready to be merged to the main branch since all the requested changes have been completed.

hoch

Apologies for the slow response, but I think this PR needs more work. Overall, many examples have a same structure, so perhaps it's better to break down the PR into smaller ones and focus on getting a small thing done first?

hoch · 2024-07-01T09:11:49Z

src/audio-worklet/basic/hello-audio-worklet/main.js

+      isPlaying = false;
+      buttonEl.textContent = 'START';
+      buttonEl.classList.add('start-button');
+    }


Similarly to the previous comment, the logic between L24~L40 is slightly confusing.

I think it's possible to split the check of oscillatorNode out of this if/else block, so the block only handles the context state.

hoch · 2024-07-01T09:13:11Z

src/audio-worklet/basic/hello-audio-worklet/main.js

+      await audioContext.resume();
+      isPlaying = true;
+      buttonEl.textContent = 'Playing...';
+      buttonEl.classList.remove('start-button');


L26-L29 are exactly same with L31-L34. Perhaps this block can be simplified further?

hoch · 2024-07-01T09:15:17Z

src/audio-worklet/basic/noise-generator/main.js


 const startAudio = async (context) => {
-  await context.audioWorklet.addModule('noise-generator.js');
+


Please remove this empty line.

hoch · 2024-07-01T09:16:49Z

src/audio-worklet/basic/noise-generator/main.js

-  }, false);
+    if (!isPlaying) { 
+      // If not playing, start the audio.
+      await startAudio(audioContext);


I believe this example is pretty much the same with the other oscillator example. Why do we need a different flag (moduleAdded) only for this example?

Updated several files related to web audio samples, including build_i…

bf3ea58

…nfo.json, index.njk, and main.js files for various audio worklet demos. Also, updated styles.css.

hoch self-requested a review March 11, 2024 17:24

hoch requested changes Mar 13, 2024

View reviewed changes

Include changes made to hello-audio-worklet and audio-worklet-node-op…

894b234

…tions files

tarunsinghofficial commented Mar 14, 2024

View reviewed changes

tarunsinghofficial requested a review from hoch March 14, 2024 17:03

hoch requested changes Mar 22, 2024

View reviewed changes

added the changes to hello-audio-worklet, audio-worklet-node-options,…

2df8734

… noise-generator

tarunsinghofficial requested a review from hoch March 26, 2024 14:50

hoch reviewed Apr 27, 2024

View reviewed changes

hoch added the gsoc24 Google Summer of Code 2024 label May 15, 2024

fixed the requested changes. Modified the main.js of hello-audio-work…

00ddbef

…let, one-pole-filter, volume-meter, noise-generator

tarunsinghofficial requested a review from hoch June 13, 2024 04:52

hoch reviewed Jul 1, 2024

View reviewed changes


		const startAudio = async (context) => {
		await context.audioWorklet.addModule('noise-generator.js');

Fix: Implemented STOP button behavior to toggle audio playback #353

Are you sure you want to change the base?

Fix: Implemented STOP button behavior to toggle audio playback #353

Conversation

tarunsinghofficial commented Mar 8, 2024 • edited Loading

Changes Made

Purpose

Screenshots

google-cla bot commented Mar 8, 2024

hoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarunsinghofficial Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarunsinghofficial commented Mar 22, 2024

hoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarunsinghofficial commented Mar 26, 2024

tarunsinghofficial commented Apr 2, 2024

hoch commented Apr 2, 2024

tarunsinghofficial commented Apr 10, 2024

tarunsinghofficial commented Apr 17, 2024

hoch commented Apr 18, 2024

hoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarunsinghofficial commented May 26, 2024

hoch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarunsinghofficial commented Mar 8, 2024 •

edited

Loading

tarunsinghofficial Mar 14, 2024 •

edited

Loading