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

Clarify interaction between user-invoked suspend and autoplay policy #1802

Closed
hoch opened this issue Dec 19, 2018 · 24 comments
Closed

Clarify interaction between user-invoked suspend and autoplay policy #1802

hoch opened this issue Dec 19, 2018 · 24 comments
Assignees
Labels
w3c-privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response.
Milestone

Comments

@hoch
Copy link
Member

hoch commented Dec 19, 2018

We still have the PR under the review, but I think this deserve another look by WG.

In Chrome, the autoplay policy automatically suspends the context when UA is not allowed to start it. This "automatic" suspension by autoplay policy will be lifted off when 1) context.resume() call or 2) source.start() call upon user activation.

However, the problem here is that there is no way to distinguish between automatic suspension and user-invoked suspension. Therefore, even if user explicitly suspended the context, the current design will unblock the context and resume the rendering.

Consider the following example:

const context = new AudioContext();
const source = new OscillatorNode(context);
source.connect(context.destination);

context.suspend(); // Explicit suspension

someButton.addEventListener('click', () => {
  console.log(context.state); // A
  source.start();
  console.log(context.state); // B
});

Apparently violating user's explicit request for suspension is not acceptable. So A and B should both be suspended. But this still brings a couple of questions:

  1. Then what happens to source.start()? Do we still have to stash it somewhere and play it when the suspension is lifted off? Or should it be disregarded completely?
  2. How do we differentiate a suspension from autoplay policy and user code? If we have to introduce another flag for this, now we have to deal with 3 internal flags; was allowed to start, is allowed to start, and user suspension. I think this needs a careful review to figure out all the corner cases.

cc: @mounirlamouri

@padenot
Copy link
Member

padenot commented Jan 11, 2019

Answering in reverse order:

  1. IMO, having the context blocked for autoplay reasons is orthogonal to suspending the context. It shares some of the mechanism underneath, but that's about it. It's important that the user intent is respected. We need multiple flags to know whether the source of the "suspend" is the autoplay policy, or the user.
  2. When source.start(), the context can initiate a transition from "suspended" to "running". However, it's also "suspended" explicitly, no sound should occur. start() starts the audio at t=currentTime, which is 0 and is not moving. When the page "resume()"s manually, this node will output sound. No change here.

@alastor0325

@hoch
Copy link
Member Author

hoch commented Jan 11, 2019

Agreed on both points. But also it means the current PR might have to be re-written with new flags.

I am doing some thought experiments about introducing a new context state autoplayblocked, which can be useful to developers and also it gets rid of the need of new internal flags.

@mounirlamouri
Copy link
Contributor

The only case that my change doesn't take into account is when suspend() was called when the context wasn't running. It seems a bit odd to me that calling suspend() on a blocked audio context should have a side effect that it doesn't actually resume when start() is called and I would be very surprised that either Chrome or Safari implement the autoplay policy this way. In my opinion, it is cleaner to avoid the side effect and only avoid resuming from a start() call when the audio context actually ran at some point. This is what is spec'd and I think what is implemented by all.

@hoch
Copy link
Member Author

hoch commented Jan 11, 2019

Okay, let's clarify a bit more.

The only case that my change doesn't take into account is when suspend() was called when the context wasn't running.

Calling suspend() on a suspended context technically will do nothing. This is specced.

It seems a bit odd to me that calling suspend() on a blocked audio context should have a side effect that it doesn't actually resume when start() is called

I am confused with this sentence. Calling suspend() on a blocked audio context will do nothing. What side effect are you referring here?

In my opinion, it is cleaner to avoid the side effect and only avoid resuming from a start() call when the audio context actually ran at some point.

What if you call suspend() when it is already blocked by autoplay policy? Then the context did not run at all since its construction, so start() call will resume the context.

Not elegant, but a simple solution would be having a flag in AudioContext, something like was suspended by user. source.start() call will check two flags,was suspended by user and is allowed to play, and then resume the context rendering when they are false and true respectively. Any suspend() call will make the former true and start() call will not be able to resume the context. Again, not elegant.

@mounirlamouri
Copy link
Contributor

The side effect I'm referring to is that, if I understand correctly, the issue is that the following will resume the audio context and you believe it shouldn't:

let ac = new AudioContext();
ac.state; // 'suspended'
ac.suspend();
let n= new OscillatorNode(ac);
n.connect(ac.destination);

b.onclick = () => { n.start(); }

My concern here is that suspend() happens when ac is already suspended and this should be a no-op. If I understand correctly, the change that is being suggested is to make this prevent auto-resume when start() is called. Is this correct?

@hoch
Copy link
Member Author

hoch commented Jan 14, 2019

My concern here is that suspend() happens when ac is already suspended and this should be a no-op.

These are two different types of suspension, and I think it needs to be handled differently.

If I understand correctly, the change that is being suggested is to make this prevent auto-resume when start() is called.

Yes. If user wants a context to be suspended explicitly, the intent should be honored. The question is how this behavior should be documented in the spec.

@hoch
Copy link
Member Author

hoch commented Jan 14, 2019

@mounirlamouri I still think we have some confusions. Consider this case on an AudioContext C:

  1. C is suspended by UA at construction because is allowed to play flag is false.
  2. source.start() is called from user interaction.
  3. is allowed to play is true, and then C is automatically resumed because it was suspended by UA.

The step 3 is the current behavior of Chrome. The current PR does not cause any action (i.e. start rendering) other than setting two flags. I am not sure if that's right. The initial start() call upon an UA suspended context will automatically resume it, so the prose should contain something more than flipping flags.

@mounirlamouri WDTY about this point?

The solution that I am thinking is:

  1. C is suspended by UA at construction because is allowed to play flag is false.
  2. C is suspended by context.suspend() called from user code.
    2.1. This sets is suspended by user flag to true.
  3. source.start() is called from user interaction.
  4. is allowed to play becomes true, but C cannot be resumed because is suspended by user is true.
  5. context.resume() is called from non-user interaction code.
    5.1. This sets is suspended by user to false.
  6. is allowed to play is true and is suspended by user is false, so the rendering will be resumed.

In short, we need an additional flag of is suspended by user and this flag only gets updated by context.suspend() and context.resume() method.

@hoch
Copy link
Member Author

hoch commented Jan 14, 2019

Two directions:

  1. Make this as an explicit algorithm in the spec.
  2. Or we can simply say "user-invoked suspension will be honored" in the prose somewhere.

@padenot
Copy link
Member

padenot commented Jan 15, 2019

#1802 (comment) is the correct way to implement this. It needs to be explicit.

In fact, this is more or less what we will be doing soon in Gecko for JavaScript breakpoints, for example: the audio stops when execution is paused, but the state is different than suspend/resume/close from the user.

@mounirlamouri
Copy link
Contributor

At the end of the day, I don't feel very strongly either way but our implementation was trying to stay close to WebKit's and the spec was obviously following it. I'm not sure how safe it is to spec something that doesn't match current implementations but maybe @jernoble would be willing to change WebKit's?

@hoch
Copy link
Member Author

hoch commented Jan 15, 2019

I have not tried yet, but how does WebKit handle the example I posted above? If WebKit somehow does it "correctly" by not resuming a context that is explicitly suspended, we might have missed some internal details in the PR.

@mounirlamouri
Copy link
Contributor

My understanding is that WebKit does the same as Chrome which should be the same as the PR I sent.

@hoch
Copy link
Member Author

hoch commented Jan 15, 2019

Yes, I just tried the example with Safari (desktop) and it has the same issue. It does not honor the user-invoked suspension. I think it is a wrong behavior and not what developers want. (crbug entry)

@rtoy
Copy link
Member

rtoy commented Jan 15, 2019

I agree @hoch . If a developer explicitly suspended a context, I think he'd be very surprised to have start() magically resume the context.

@padenot
Copy link
Member

padenot commented Feb 14, 2019

Gecko has implemented respecting the author's intent, and is not resuming automatically.

@mdjp mdjp added this to the Web Audio V1 milestone Feb 21, 2019
@hoch
Copy link
Member Author

hoch commented Feb 25, 2019

@jernoble @mounirlamouri We would like to proceed with the PR #1823. PTAL.

@padenot
Copy link
Member

padenot commented Mar 14, 2019

I've been informed by jer that this is a Safari bug, and is not intentional, so we'll proceed with the spec as proposed here: respecting the author's intent, and not resuming the context if it's been suspended.

@hoch hoch removed this from In WG Discussion in V1 Mar 14, 2019
@mounirlamouri
Copy link
Contributor

@jernoble @mounirlamouri We would like to proceed with the PR #1823. PTAL.

I've approved the CL. Thanks @hoch

@svgeesus
Copy link
Contributor

svgeesus commented Jul 1, 2019

Reopening; the Director want to know whether there are privacy implications for this issue which would merit review.

@svgeesus svgeesus added the w3c-privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. label Jul 1, 2019
@hoch
Copy link
Member Author

hoch commented Jul 1, 2019

I do not think so. This is simple a spec bug which UA could bypass a suspend() call with a user gesture. We agreed to fix the oversight. There's no issue in the regard of fingerprinting or general privacy.

@svgeesus
Copy link
Contributor

svgeesus commented Jul 2, 2019

I tend to agree. The problem is not that the user can't suspend or that there is no auto-suspend; it is that when auto-suspend is cancelled (by user interaction), that also accidentally resumes user-invoked suspension.

@rtoy
Copy link
Member

rtoy commented Sep 18, 2019

What needs to be done here?

@padenot
Copy link
Member

padenot commented Sep 26, 2019

Probably just re-closing it.

@rtoy
Copy link
Member

rtoy commented Sep 26, 2019

Closing.

Re-open if needed.

@rtoy rtoy closed this as completed Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
w3c-privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response.
Projects
None yet
Development

No branches or pull requests

6 participants