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

Don't try to take log2 of 0 when using mono audio channels. #26611

Closed
wants to merge 1 commit into from

Conversation

@jdm
Copy link
Member

jdm commented May 22, 2020

This avoids a panic when loading rooms in Hubs.

@highfive
Copy link

highfive commented May 22, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/baseaudiocontext.rs
  • @KiChjang: components/script/dom/baseaudiocontext.rs
@jdm
Copy link
Member Author

jdm commented May 22, 2020

@bors-servo try=wpt

@highfive
Copy link

highfive commented May 22, 2020

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@bors-servo
Copy link
Contributor

bors-servo commented May 22, 2020

Trying commit b643451 with merge 5f25b15...

bors-servo added a commit that referenced this pull request May 22, 2020
Don't try to take log2 of 0 when using mono audio channels.

This avoids a panic when loading rooms in Hubs.
@bors-servo
Copy link
Contributor

bors-servo commented May 22, 2020

☀️ Test successful - status-taskcluster
State: approved= try=True

@Manishearth
Copy link
Member

Manishearth commented May 22, 2020

We also shouldn't be using log2 at all, ideally we can do (x - 1).count_ones() + 1, conditional on x.count_ones() being one

@jdm jdm added this to In progress in Hubs support May 22, 2020
@jdm
Copy link
Member Author

jdm commented May 26, 2020

I now can't reproduce this panic on Hubs without this PR applied, and I haven't been able to reproduce the panic in a page using a mono audio buffer:

<script>
var context = new AudioContext();
var buffer = context.createBuffer(1, 22050, 22050);
for (var channel = 0; channel < buffer.numberOfChannels; channel++) {
  // This gives us the actual ArrayBuffer that contains the data
  var nowBuffering = buffer.getChannelData(channel);
  for (var i = 0; i < buffer.length; i++) {
    // Math.random() is in [0; 1.0]
    // audio needs to be in [-1.0; 1.0]
    nowBuffering[i] = Math.random() * 2 - 1;
  }
}
var source = context.createBufferSource();
source.buffer = buffer;
source.connect(context.destination);
source.start();
</script>

Since I don't really want to merge changes that can't clearly show an improvement, I'm going to close this PR. We can reopen it if we find a way to trigger the problem again.

@jdm jdm closed this May 26, 2020
@atouchet atouchet removed this from In progress in Hubs support May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.