-
Notifications
You must be signed in to change notification settings - Fork 166
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
cancelAndHoldAtTime() algorithm can be made clearer and simpler #2437
Comments
I'm assigning this to myself so that I don't forget to think about this. |
There is no need to in practice: either the algorithm inserts its own It's a bit confusing indeed, I'll see whether we can align the non-normative description and and the normative algorithm.
A non-normative description could read: E1 is the event (if any) right before or at Tc, E2 is the event (if any) right after or Tc. In other words, there are two conditions in each clause: E1 MUST be an event, we name its event time T1, and T1 MUST be lower or equal to Tc. As for rephrasing the algorithm, I reckon it might be simpler, but the current one works. |
There are two possibilities:
The algorithm points to the first one, the description (before your recent change) – to the second.
Yes, that's the intended meaning. But that's not what it says:
It does not say T1 has to be a time when an event happens. It just says it's a number satisfying T1 ≤ Tc. If you look at the description, the graphs, etc., the meaning becomes clear, but the above definition is formally inaccurate. It would be better if it said:
The only problem with this is that "the event" suggests there's only one event at a given time, which I believe is not the case. There can be a linearRamp that ends at T2, followed by a setValue or setTarget at the same time T2. Which of these is "the event"? Perhaps it should say:
In its current form, it doesn't capture the intended meaning, as I've shown above. A program implemented to precisely follow the formal description would not work as intended. After giving it some thought, I guess I would suggest removing the algorithm altogether. It's unnecessarily low-level. It mentions various automation functions by name. All the E1/E2 mess is there because the algorithm tries to figure out (from event order) a simple thing – which automation will be in progress at time Tc. Because it's so low-level, if someone ever decides to make changes to automation functions (example*), they will have to review and edit the cancelAndHold algorithm, likely adding more special cases to it. Is that really the purpose of a spec? Why not abstract it away and say:
Then it's up to the implementation to figure out what the value at Tc will be (according to the currently scheduled sequence of automation events) and how to rewrite ramps to ensure that the specified end state is achieved. *) e.g. define what happens if a linearRamp is in progress and we schedule setTargetAtTime to start before the end of the ramp – oops, now setTarget is the earliest event > Tc and the algorithm no longer works! |
@padenot That's awesome, thank you for taking my arguments into consideration. Hope it will result in a simpler, easier-to-maintain spec. |
Yes, we chose an algorithm to make it clearer how things worked. If you think hard enough, you could have derived all of the results, but that would mean each implementor would have to think really hard and not make mistakes, especially on choosing if it's And if two implementations disagree, then they get to argue on the interpretation, basically having to describe the algorithm decisions to each other in detail. So in that respect, I'd like to keep the algorithms, but I confess on not having followed these discussions too closely since I'm technically still not back at work. (Which is not to say I won't respond to questions; I just might be a bit slow.) |
As pointed out earlier, the algorithm was incorrect at times, and had contradiction with the introductory text. I agree that in general, precise algorithms are preferred, but the one we had had holes, and expressing it in prose with steps is easier here. |
There has been quite a bit of discussion in #1796. Let me make a different proposal on how it cancelAndHold might be speced in a simpler way that does what I think people would like.
When time WDYT? |
One issue with this approach is if a dev calls cancelAndHoldAtTime(t) and then adds automations with times after But I'm also guessing that most devs don't cancel events in the future. It's pretty much cancel at currentTime, but there is probably still a small window where automations added after cancel would get cancelled unexpectedly. Not sure what to do about that except to have a separate list for events added after calling cancel. This is really messy and not something I would want to spec. Too many corner cases like canceling in the future, adding more events afterwards, then cancelling at a different time further into the future, and so on. |
This is too much of a change in behaviour I feel. |
Which change is too big? Both https://github.com/WebAudio/web-audio-api/issues/1791#issuecomment-458591662 and https://github.com/WebAudio/web-audio-api/issues/1791#issuecomment-458760213? What you've proposed doesn't match what chrome does. AFAIK, only chrome has this, so I'm willing to make chrome take the hit with the change if we come up with a new scheme that's relatively simple to understand and does what people would want. |
As said during the teleconf, at this point, I'd like to encode in the spec what Chrome is doing. |
I think inserting a special cancel event (that Chrome does) makes some sense. But the corner cases aren't adequately describe in the spec and I think Chrome is a bit confused on what is supposed to happen and probably shouldn't be a guide for the behavior. I think inserting a cancel event works for anything that might get scheduled before the cancel event. For events scheduled after the cancel event, I'm not sure what should happen. This needs discussion, I think. An example:
What is the output? |
To me, the final state should be, for
|
This makes sense to me. But this also basically means that cancelAndHold has to remember that it canceled linearRamp(v1, t+5). Chrome does this. Why? Consider this:
The interpretation used by Chrome is that hold doesn't happen until t+4, so everything before t+4 should behave as if nothing had happened until t+4. So we will hold the value somewhere between v3 and v1 (instead of v0 and v1). I think it's certainly easier if cancel computed the hold value of the timeline event list at the time of the call, but I think that's probably not what people would expect. |
I think it's expected that it cancels and holds at time |
So it holds the value somewhere between v3 and v1? |
I think what Chrome does is the right thing to do here. |
Sounds good. I think there's few corner cases to consider. Consider the slightly modified example from https://github.com/WebAudio/web-audio-api/issues/1791#issuecomment-465664559:
What happens now? Obviously the linearRamp(v3) is removed, and I think we want to remove the cancel event at t+4, but what does the automation look like from time t+3 to t+5? Is it the same as if the second cancel never happened? What if instead of cancel(t+4), we did cancelAndHoldAtTime(t+3.5)? What does the curve look like now? |
Another corner case:
What does the curve look like now? When What about this slightly different case:
Without the final ramp, I think we all agree the held value should be Maybe the best answer is that the final ramp has no affect until the |
My take, after working on a synth library for a couple of weeks:
This neatly sorts out @rtoy's edge cases. In the first case, we'll ramp from the initial value to v0 during the first second, then stay there for a second, then ramp from v0 to v1 during the third second. (I believe that's what Chrome currently does.) The second case is simply rejected as meaningless because it doesn't make sense to enqueue events before the beginning of time. |
To elaborate a bit: If this were a new API, I would suggest that value events have to be enqueued in chronological order, with cancellations being the only way to rewind and change the timeline. I don't think that would be a terribly intrusive change (I can't think of a use case where one would really need to enqueue events out of order), but it would probably break some existing code. |
The proposal looks interesting, but I don't quite understand exactly what is happening in item 2:
So at the time And for
the spec already says the times are clamped to current time so scheduled things in the past makes them actually behave as if you scheduled them at |
I guess I shouldn't have called it beginning_of_time :) The idea is that it's the beginning of time as far as value-related calls are concerned, i.e., it may actually be far ahead of currentTime. Let's call it cutoff_time from now. To elaborate on my reasoning, I see two fundamental use cases: First, there might be fixed or generated content (e.g., from a sequencer), in which case the caller knows all events in advance, and so they can easily be scheduled in chronological order. Second, events might arrive in real time, e.g., from a MIDI keyboard, in which case they're in chronological order because currentTime only moves forward. The only events that make sense outside of chronological order are cancellations: In the first use case, one might choose to stop or replace a scheduled sequence. In the second use case, a scheduled envelope might be retriggered. (The latter is the reason I got interested in this matter.) So, in my ideal world, cutoff_time would be the time of the most recent event (for both value events and cancellation events), and cancellations are the only events whose time can predate cutoff_time. Since there's probably a fair amount of legacy code that expects to schedule events out of order, enforcing full chronological order may not be an option, and so my proposal might be considered a partial chronological order --- disallow non-cancellation events that predate the most recent cancellation. Imposing this kind of partial chronological order shouldn't be too intrusive because it's not even clear what it should mean to violate it (as your edge cases illustrate), and so I hope that there's no legacy code that depends on this.
That's sort of the point of introducing a cutoff time. With a cutoff time, the value v0 at t0 is well-defined when cancelAndHoldAtTime is called because you can't schedule value-related events that predate t0. (As far the implementation is concerned, you don't even have to compute this value when the cancellation is called; it's enough if the implementation behaves as it setValueAtTime(v0, t0) had been called, and for that it's enough to know that a well-defined v0 exists.) |
Historically, this is not how cancelAndHold was expected to work. Any events inserted between currentTime and cancelTime are expected to work and that would change the v0 value. I suspect no one does this, but I have no metrics to show that. The implementation of cancelAndHold would have been much simpler if we did what you say, but it might be too late to change it now. |
I think to make progress on this, we need to decide what the following bit of code should do:
I think there are two options here.
The second option is what Chrome does; it is consistent with how automations can be inserted any where in the timeline at any time. This part of the spec isn't changeable now. The first is not what Chrome does, but is relatively simple to explain, but is not consistent how the rest of the automations are handled. Choosing option 1 or 2 will let us proceed with defining cancelAndHold more precisely. I also propose that if we do not have a clear consensus on options 1 or 2 (or any other proposal), then the spec stays as it is, warts and all. We need a decision on this by end of April or so to allow time to update the spec in time for publication. If you have opinions on this please speak up! |
My initial feeling is there's no strongly intuitive best solution in this case, and so I'd look for what is most consistent with the behavior in other, more intuitive, situations. If the inserted ramp is after the hold time. e.g.
Then I'd expect the hold to occur on the ramp from (1,2) to (3,3). i.e. at value 2.5, with the final ramp from (2,2.5) to (2.5,10). Essentially the To me that seems more in line with option 1. |
I would like option 1, but that's only because that's how I initially expected it to work, and because I think it's easier to understand. I think either option would work for the use cases that I know about, so unless someone knows a scenario where only one option would work, then I think the simpler option would be best (which IMO is option 1). |
I am absolutely in favor of option 1. To be honest, I'm not sure where the idea of treating
If It would also be unintuitive. Imagine you tell your assistant "Clear out my schedule after April 15". Then you say "OK, now schedule a trip on April 20-22". Which of the following would you prefer:
|
@tszynalski I think you misunderstand when events are removed. Let's say at time @karlt For your example, I agree, and I think option 1 and 2 will behave consistently producing the same answer. And I think option 1 is a strict subset of option 2. If you don't insert new events before the cancel time, then both options behave the same. The question is what happens if you do insert an event. And as I mentioned, Chrome does option 2. And fundamentally, |
@rtoy Perhaps your idea (for option 2) is that the cancel part should act immediately, but the hold part should be a scheduled event? In your example, it doesn't matter either way, because the behavior is undefined:
Perhaps it would be a better example if you replaced the last ramp with setTargetAtTime(t=1.5). |
@tszynalski Sorry for being unclear. I disagree with your statement:
If you remove the cancel, then the timeline is well-defined. You should get
You can try the following snippet at https://hoch.github.io/canopy:
Chrome and Firefox behave consistently here. If you uncomment the cancel line for chrome, then it holds the value that I would expect, from comparing the curves with and without cancel. If option 1 were in place, the held value would be different. |
@rtoy
If the cancellation happens immediately, then Then we have Given the above, I don't see how the held value could be anything other than 2.5. Do you see an alternative possibility? If we don't cancel immediately, then @karlt 's example and my example will behave unintuitively, as far as I am concerned. If I do |
@rtoy You may be right that the spec is well-defined with respect to scheduling ramps before existing ramps end. But perhaps it does not explain things clearly enough. For example, I'm not sure what the spec says on this:
...then at t=3, when the ramp is already in progress, we call:
What happens? Do we get the first ramp from v=0,t=0, to v=3,t=3, then the new ramp from v=12,t=3 to v=20,t=5 (discontinuity at t=3), then the first ramp again from v=20,t=5 to v=10,t=10? So one ramp can get executed (and recalculated) multiple times? |
There has been some discussion of how newly added events would affect the curve for time that has already passed. See from about #1712 (comment) |
I strongly prefer Option 1, for the same reason that @tszynalski explained --- the cancellation should be about events that were enqueued before the cancellation; it would be surprising to cancel events that haven't been enqueued yet. |
No one has proposed that The issue is the value that is held. Is it the value for the timeline at time of the call, or the value of the timeline at time For those favoring option 1 because of the misunderstanding on when Option 2 is basically what Chrome is doing today. And it's that way because we allowed for people to insert events. I didn't actually expect people to do that, but it is allowed, and I also expected people to do only |
To allow time to craft a design and write the necessary documentation, please respond with your thoughts on this issue by April 21 to allow time for discussing the corner cases and writing the spec text. Sorry for imposing a hard deadline, but we are running out of time to get to spec recommendation. |
With option 2, "The held value is somewhere between 3 and 10", from where does "3" derive? The linearRampToValueAtTime(3, 3) no longer exists because events after time = 2 have been cancelled. For this example with linear ramps, if events, with time after the cancellation time, added after cancel but before currentTime reaches the cancellation time, don't modify value at the cancellation time (which is what "hold" implies, I assume), then why would they modify the value at cancellation time if they have a time before the cancellation time? The situation seems somewhat different with setTargetAtTime. If a setTargetAtTime curve has been set up to be held at a cancellation time, and then a new event is added between the start of the setTarget curve and the cancellation time, then there is a question as to whether there is any part of the curve that can still be held. I see the merit in an option 2 equivalent with setTargetAtTime, but I'm having trouble making sense of it with linear ramps. |
The "3" comes from the linearRamp(10,3) which was cancelled, but we need to remember that to figure out what value to use when cancelling. And this is kind of a consequence of inserting a cancelAndHold event so that we hold the automation at the given time, instead of computing the value at the time of the call (option 1). |
FWIW, the difference in behavior between Option 1 and Option 2 doesn't matter to me because I will always enqueue my automation events in chronological order, except for cancellations. I got interested in this topic because I ran into a related bug, and I still prefer Option 1 because it seems more likely to help resolve this bug. Philosophically speaking, I find simplicity in core APIs vastly more important than convenience; support for out-of-order events complicates the API, presumably to make life easier for some users of the API, and I think that's the wrong tradeoff. I suppose we're stuck with out-of-order events in general, but at least in the case of cancellations we have an opportunity to tighten this up a bit. |
@nettoyeurny Thanks for the comments. As mentioned Option 1 is a strict subset of Option 2 and are equivalent if you never scheduling anything before the cancel time has arrived. It's certainly simpler, but inserting events in any order is how automations have worked since the very beginning so we can't really remove that ability. I only favor Option 2 because it supports that ability (with added complexity on the implementer, not the developer). |
In a their own way, both options support the ability to add events with times before tc, but the behavior provided is different. When E2 is not a ramp and E1 is setTarget, implicit insertion of the setValueAtTime event during the cancelAndHoldAtTime() call would produce a hold value based on the events existing when cancelAndHoldAtTime() was called. If further events are then added with times less than tc, then there can be a discontinuity at tc. I suspect it would be more useful to hold the value of the curve indicated by the new events so that the held value is the value that the curve reaches at tc. I would be in favor of this change. This sounds somewhat like option 2 and could be implemented with a special cancelAndHold event. When E2 is a ramp, then rewriting the ramp during the cancelAndHoldAtTime() call produces the behavior that I would expect: it cancels all events with times greater than or equal to tc and generates a replacement event so that the curve before tc is unchanged. This is option 1. In this situation, option 2 would be unexpected to me in that it essentially maintains a fork of events after tc to use for determining the held value. The events after tc are in that sense not actually cancelled. I would recommend leaving the algorithm as is for case with E2 a ramp (rewriting the ramp), but changing the E2 not ramp and E1 setTarget case to hold the value that the curve eventually reaches. However, I don't feel that that adding events with times less than tc is a common use case, and so this behavior is not too important to me. What I would be keen to avoid though is changing the E2 is a ramp case, but not changing the E2 not ramp and E1 setTarget case, because that would seem to me inconsistent and the worst of both worlds. Perhaps another option to consider may be to postpone cancelAndHoldAtTime until the next version of the spec. |
@karlt Yeah, if you call cancelAndHoldAtTime(Tc) and then setTargetAtTime(Ts) (but not a ramp) with Ts < Tc, then there will be a discontinuity at Tc. But it seems this can be easily avoided by calling cancelAndHoldAtTime(Ts) beforehand to clear the event queue. This problem is not exclusive to cancelAndHold(). There might have been regular setValueAtTime() events scheduled that could produce a discontinuity. In general, if you're not sure what events might be scheduled, you should probably call cancelAndHoldAtTime() before setTargetAtTime(). It doesn't seem unreasonable to me. |
From today's working group teleconf, we've decided that there isn't enough time to do a good job with this in time for V1. We'll tackle this in V2 and spec it correctly. So moving this to v.next, hi-priority. |
We've implemented a synth in Chrome, that utilizes cancelAndHoldAtTime to create glitch-free ADSR envelopes. And from a synthesizer perspective, option 2 is crucial in creating predictable and glitch-free behavior for the end user. With synths there is often a need to modulate the same destination with multiple sources that are triggered by the end user in an unpredictable order, which makes it impossible to ensure that automation events are called in chronological order. Option 2 takes that into consideration, and ensures that the hold value will take into account any and all automation that is actually occurring when the hold time is reached. |
Virtual F2F resolution:
|
is cancelAndHoldAtTime(t0) meant to introduce an event equivalent to setValueAtTime(currentvalue, t0) ? (where currentvalue is the value at that time point). Reading this discussion and testing this stuff in chrome leaves me uncertain. That would be intuitive, otherwise subsequent usages of linearRampToValueAtTime(newValue, t1) are going to be cumbersome, forcing you to manually call setValueAtTime and possibly doing some off-the-side book-keeping calculations. |
https://webaudio.github.io/web-audio-api/#AudioParam-methods
setValueAtTime(value, time)
followed bycancelAndHoldAtTime(time);
will cancel the setValueAtTime() call.This is poorly phrased because the largest number satisfying T1 ≤ Tc is Tc. It looks like you're only interested in "an event at time Tc". Similarly, the second item seems to refer to the frame immediately following Tc.
Furthermore, everything is phrased in terms of events, which is quite confusing, because for some functions the "event time" is the starting time for the automation (e.g.
setTarget()
), for others it's the end time (linear/exponential ramp), and you always have to remind yourself of that distinction when you see a phrase like "event time". This dual nature of "event time" is really the reason for much of the complexity of the spec, but in this particular case I believe it can be sidestepped. Here's my suggestion for how to rewrite it:Only one automation can be in progress at a given time, so this is unambiguous. It is also quite clear what it means for an automation "to be in progress" (an automation is in progress if it will determine the next value of the AudioParam).
The text was updated successfully, but these errors were encountered: