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

Updates to PR353 implemented stop button #362

Conversation

duojet2ez
Copy link
Contributor

@duojet2ez duojet2ez commented May 14, 2024

[DO NOT MERGE] - we will break this down to multiple PRs.

I updated PR 353 with necessary changes for merge. Edited main.js files in:

noise-generator, one-pole-filter, volume-meter, hello-audio-worklet, audio-worklet-node-options

Specifically, I used audioContext.suspend() to allow the user to pause the sound with a stop button. And I added an isModuleLoaded check to avoid redundantly calling addModule. Also added isPlaying to toggle the playing state.

Copy link
Member

@hoch hoch 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 your contribution? Generally looks good, but some minor changes might be necessary.

If you have a fork, can you create a live test web page?

src/audio-worklet/basic/audio-worklet-node-options/main.js Outdated Show resolved Hide resolved
src/audio-worklet/basic/audio-worklet-node-options/main.js Outdated Show resolved Hide resolved
src/audio-worklet/basic/audio-worklet-node-options/main.js Outdated Show resolved Hide resolved
src/audio-worklet/basic/noise-generator/main.js Outdated Show resolved Hide resolved
@duojet2ez
Copy link
Contributor Author

duojet2ez commented May 21, 2024

@hoch ok all changes should be good. I tested locally... I am in the process of setting up a live test web page. Does github host that? Also... hopefully i am following protocol with git/github for making these changes

@duojet2ez
Copy link
Contributor Author

@hoch Just wondering if you can give clarification on a live test page for this?

@hoch
Copy link
Member

hoch commented Jul 11, 2024

Apologies for missing your comment from May 31! Have you found a way of doing the live testing?

If you already have a fork, you should be able to deploy the tip-of-tree from the GitHub repo.

Lastly, this PR touches too many files and that makes it the progress more difficult. We should break this down to multiple smaller PRs and incrementally review/merge them. If you can minimize this PR down to 1 example project, what would it be?

@duojet2ez
Copy link
Contributor Author

@hoch Thanks for the feedback! I tried to deploy my forked web-audio-samples website with heroku but was running into problems. I am assuming that's what you mean by "deploy" and "live testing"? I wasn't sure so I didn't spend too much time on that.

I think it's a great idea to break up into smaller PR's as this touches many files as you noticed. Can I make a PR just for the Hello Audio Worklet example?

@hoch
Copy link
Member

hoch commented Jul 11, 2024

I didn't meant to use the external tool like "heroku". Just like this repository, you should be able to serve the your forked repository on GitHub.

Yes. Focusing on a single example is definitely my preference.

@duojet2ez
Copy link
Contributor Author

Great! I'll work on this

@hoch
Copy link
Member

hoch commented Jul 31, 2024

@duojet2ez Since now we merged the first part, I think we can break this down to multiple PRs and try to review/merge one by one. There will be small diffs on them, so the process will be much faster!

I'll also change the description/title to reflect the current state of this PR.

@tarunsinghofficial
Copy link
Contributor

tarunsinghofficial commented Aug 1, 2024

@duojet2ez Since now we merged the first part, I think we can break this down to multiple PRs and try to review/merge one by one. There will be small diffs on them, so the process will be much faster!

I'll also change the description/title to reflect the current state of this PR.

Hi @hoch!

I had a busy schedule earlier and saw that the original PR was closed. So, I'm interested in contributing to the PR's small parts. Let me know where to start with.

Thanks

@duojet2ez
Copy link
Contributor Author

@tarunsinghofficial

The stop feature for the hello-audio-worklet example was just merged in. We have the following examples left:

  • noise-generator
  • one-pole-filter
  • volume-meter
  • audio-worklet-node-options

Which would you like to work on?

Also the hello-audio-worklet example was approved with the following code

const audioContext = new AudioContext();
let isModuleLoaded = false;
let isPlaying = false;
let isGraphReady = false;
let oscillatorNode = null;

const loadGraph = (context) => {
  oscillatorNode = new OscillatorNode(context);
  const bypasser = new AudioWorkletNode(context, 'bypass-processor');
  oscillatorNode.connect(bypasser).connect(context.destination);
  oscillatorNode.start();
};

const startAudio = async (context) => {
  if (!isModuleLoaded) {
    await context.audioWorklet.addModule('bypass-processor.js');
    isModuleLoaded = true;
  }
  if (!isGraphReady) {
    loadGraph(audioContext);
    isGraphReady = true;
  }
};

// A simplem onLoad handler. It also handles user gesture to unlock the audio
// playback.
window.addEventListener('load', async () => {
  const buttonEl = document.getElementById('button-start');
  buttonEl.disabled = false;

  buttonEl.addEventListener('click', async () => {
    if (!isPlaying) {
      await startAudio(audioContext);
      isPlaying = true;
      buttonEl.textContent = 'Playing...';
      buttonEl.classList.remove('start-button');
      audioContext.resume();
    } else {
      audioContext.suspend();
      isPlaying = false;
      buttonEl.textContent = 'START';
      buttonEl.classList.add('start-button');
    }
  });
});

So that's the general format/naming convention we should probably continue to use. It shouldn't be too hard to apply that now to the other examples with minor changes Thanks!! Your idea for this was great.

@hoch
Copy link
Member

hoch commented Aug 1, 2024

@tarunsinghofficial Thanks for your continued interest. As @duojet2ez suggested, it would be great if we can work on each example one by one - and it would be much easier to make progress faster.

@tarunsinghofficial
Copy link
Contributor

@tarunsinghofficial Thanks for your continued interest. As @duojet2ez suggested, it would be great if we can work on each example one by one - and it would be much easier to make progress faster.

Yes, got it. We can work one by one on example.

@tarunsinghofficial
Copy link
Contributor

@tarunsinghofficial

The stop feature for the hello-audio-worklet example was just merged in. We have the following examples left:

  • noise-generator
  • one-pole-filter
  • volume-meter
  • audio-worklet-node-options

Which would you like to work on?

Also the hello-audio-worklet example was approved with the following code

const audioContext = new AudioContext();
let isModuleLoaded = false;
let isPlaying = false;
let isGraphReady = false;
let oscillatorNode = null;

const loadGraph = (context) => {
  oscillatorNode = new OscillatorNode(context);
  const bypasser = new AudioWorkletNode(context, 'bypass-processor');
  oscillatorNode.connect(bypasser).connect(context.destination);
  oscillatorNode.start();
};

const startAudio = async (context) => {
  if (!isModuleLoaded) {
    await context.audioWorklet.addModule('bypass-processor.js');
    isModuleLoaded = true;
  }
  if (!isGraphReady) {
    loadGraph(audioContext);
    isGraphReady = true;
  }
};

// A simplem onLoad handler. It also handles user gesture to unlock the audio
// playback.
window.addEventListener('load', async () => {
  const buttonEl = document.getElementById('button-start');
  buttonEl.disabled = false;

  buttonEl.addEventListener('click', async () => {
    if (!isPlaying) {
      await startAudio(audioContext);
      isPlaying = true;
      buttonEl.textContent = 'Playing...';
      buttonEl.classList.remove('start-button');
      audioContext.resume();
    } else {
      audioContext.suspend();
      isPlaying = false;
      buttonEl.textContent = 'START';
      buttonEl.classList.add('start-button');
    }
  });
});

So that's the general format/naming convention we should probably continue to use. It shouldn't be too hard to apply that now to the other examples with minor changes Thanks!! Your idea for this was great.

Great. I'll work on the leftover examples. Will check out all of them one by one.

Thanks for appreciating!

@tarunsinghofficial
Copy link
Contributor

Hi @hoch @duojet2ez, I have updated the one-pole filter and noise-generator examples with the required changes. Let me know if they're correct. I hope it resolves the issues.

Also, will update the other two examples.

Thanks

@tarunsinghofficial
Copy link
Contributor

Hi @hoch @duojet2ez

I have made the required changes to all 4 examples, and PR #390 has been created with all the updations. It's now ready to be merged.

Thanks

@hoch hoch closed this in #390 Aug 20, 2024
@hoch hoch closed this in 15af0c2 Aug 20, 2024
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.

3 participants