Bug 74750 – WebAudio: Add support for AudioNode "tailTime()" and "latencyTime()"

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Jer Noble]

WebCore: Add support for AudioNode "tailTime()"

[Jer Noble]

Created attachment 119674 [details]
Patch

[Jer Noble]

Created attachment 122836 [details]
Patch

[Jer Noble]

Created attachment 130663 [details]
Patch

Updated patch to remove the dependency on bug #74553.  Will upload the remaining work to a new bug, TBD.

[Jer Noble]

(In reply to comment #3)
> Updated patch to remove the dependency on bug #74553.  Will upload the remaining work to a new bug, TBD.

Actually, the order of patch dependencies will just switch.  #74553 is now marked as depending on this bug.

[Jer Noble]

Created attachment 130686 [details]
Patch

Added tailTime() overrides for more classes. (AudioDSPKernel, Biquad, EqualPowerPanner, HRTFPanner, Panner, BiquadDSPKernel, BiquadFilterNode, BiquadProcessor, DelayDSPKernel, WaveShaperDSPKernel.)

[Raymond Toy]

(In reply to comment #5)
> Created an attachment (id=130686) [details]
> Patch
> 
> Added tailTime() overrides for more classes. (AudioDSPKernel, Biquad, EqualPowerPanner, HRTFPanner, Panner, BiquadDSPKernel, BiquadFilterNode, BiquadProcessor, DelayDSPKernel, WaveShaperDSPKernel.)

Sounds good.  Haven't done a review of this code yet, but did notice a possible change that might be applied in this patch.  In AudioNode::finishDeref, there's a test for ConvolverNode and DelayNode so that their outputs are not disabled (along with a FIXME comment).  This patch should probably update the test to cover any node that has a non-zero tailTime().

[Raymond Toy]

Comment on attachment 130686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130686&action=review

> Source/WebCore/platform/audio/Biquad.h:85
> +    int tailSamples() const { return 2; }

2 is not the correct number of tail samples. :-)  The theoretical value is infinity, but pragmatic actual value depends on the filter coefficients and also depends on when we might consider the output close enough to zero.  As a first cut maybe just use 128?

> Source/WebCore/platform/audio/HRTFPanner.cpp:299
> +    return MaxDelayTimeSeconds;

This is not quite right.  We also need to add some time for the HRTF filter impulse responses themselves, and also an additional value due to the fact that the filters are implemented using a convolver with its own latency.  Perhaps as a first cut we can just use 512 sample frames because that's how much noteGrainOn always adds to handle the HRTF filters.  If we do just use 512, we should probably add a comment about doing a better job of figuring out the correct value.

> Source/WebCore/webaudio/AudioBasicProcessorNode.h:59
> +    const AudioProcessor* processor() const { return m_processor.get(); }

Is it necessary to do the const-correctness in this patch?  It seems independent of adding tailTime().

> Source/WebCore/webaudio/AudioContext.h:98
> +    float sampleRate() const { return m_destinationNode->sampleRate(); }

Same comment about const-correcness in this patch instead of a different one.  Applies also to the changes in AudioDestinationNode.h, and AudioNode.h.

> Source/WebCore/webaudio/BiquadProcessor.cpp:160
> +

Shouldn't the tailTime be the max tail time of all the kernels?  Isn't each kernel applied to a different channel, and hence the tail times don't sum?

> Source/WebCore/webaudio/ConvolverNode.cpp:156
> +    return m_reverb ? m_reverb->impulseResponseLength() / sampleRate() : 0;

You have the correct expression in the patch for bug 74533 (silence hint).  I think this patch should use the correct expression instead of in 74533.

> Source/WebCore/webaudio/DelayDSPKernel.cpp:139
> +    return max(m_desiredDelayFrames, m_maxDelayTime) / sampleRate();

Can the max value ever NOT be m_maxDelayTime?

> Source/WebCore/webaudio/DelayNode.cpp:48
> +    return delayProcessor()->delayTime()->value();

Just curious.  The delay time can change, so you're allowing the tailTime to change with that?  Do you think there might be issues if the delay time is increasing and we'll always process enough to capture that?

[Jer Noble]

Comment on attachment 130686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130686&action=review

>> Source/WebCore/platform/audio/Biquad.h:85
>> +    int tailSamples() const { return 2; }
> 
> 2 is not the correct number of tail samples. :-)  The theoretical value is infinity, but pragmatic actual value depends on the filter coefficients and also depends on when we might consider the output close enough to zero.  As a first cut maybe just use 128?

Yes, I'm afraid I don't have the math necessary to calculate the tail time of a BiquadFilter.  It may be more correct to just return positive-infinity from BiquadFilter::tailTime().  This will cause a performance problem once silence propagation goes in, but some of that performance can be regained by modifying the Biquad processor to detect when it has generated silence and set the output bus as silent appropriately.

>> Source/WebCore/platform/audio/HRTFPanner.cpp:299
>> +    return MaxDelayTimeSeconds;
> 
> This is not quite right.  We also need to add some time for the HRTF filter impulse responses themselves, and also an additional value due to the fact that the filters are implemented using a convolver with its own latency.  Perhaps as a first cut we can just use 512 sample frames because that's how much noteGrainOn always adds to handle the HRTF filters.  If we do just use 512, we should probably add a comment about doing a better job of figuring out the correct value.

This one is going to be even more difficult given that the impulse responses will change based on what azimuth and elevation are provided (and what previous azimuth and elevation parameters were!).

It might be easier to ask the HRTFDatabase what the maximum delay of its kernels is.  Then add in whatever latency is introduced by the convolvers.

>> Source/WebCore/webaudio/AudioBasicProcessorNode.h:59
>> +    const AudioProcessor* processor() const { return m_processor.get(); }
> 
> Is it necessary to do the const-correctness in this patch?  It seems independent of adding tailTime().

We can pull the const-correctness issues into their own patch.

>> Source/WebCore/webaudio/AudioContext.h:98
>> +    float sampleRate() const { return m_destinationNode->sampleRate(); }
> 
> Same comment about const-correcness in this patch instead of a different one.  Applies also to the changes in AudioDestinationNode.h, and AudioNode.h.

Ditto.

>> Source/WebCore/webaudio/BiquadProcessor.cpp:160
>> +
> 
> Shouldn't the tailTime be the max tail time of all the kernels?  Isn't each kernel applied to a different channel, and hence the tail times don't sum?

Whoops, yes that's right.  Changed.

>> Source/WebCore/webaudio/ConvolverNode.cpp:156
>> +    return m_reverb ? m_reverb->impulseResponseLength() / sampleRate() : 0;
> 
> You have the correct expression in the patch for bug 74533 (silence hint).  I think this patch should use the correct expression instead of in 74533.

Indeed, I'll pull it in.

>> Source/WebCore/webaudio/DelayDSPKernel.cpp:139
>> +    return max(m_desiredDelayFrames, m_maxDelayTime) / sampleRate();
> 
> Can the max value ever NOT be m_maxDelayTime?

Nope.  I'll get rid of the max().

>> Source/WebCore/webaudio/DelayNode.cpp:48
>> +    return delayProcessor()->delayTime()->value();
> 
> Just curious.  The delay time can change, so you're allowing the tailTime to change with that?  Do you think there might be issues if the delay time is increasing and we'll always process enough to capture that?

The alternative would be to ask the kernel what it's tailTime() is?  Sure.

[Chris Rogers]

Comment on attachment 130686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130686&action=review

> Source/WebCore/platform/audio/AudioDSPKernel.h:66
> +    virtual double tailTime() const = 0;

It's up to you how you want to stage the patches, but I think it would be a lot easier to kill two birds with one stone and also define the "latency()" method here and everywhere else since the changes are likely to occur in exactly the same places, and I think we'll need to have the latency() info (in addition to the tailTime) in order to implement silent hint propagation.

>>> Source/WebCore/platform/audio/Biquad.h:85
>>> +    int tailSamples() const { return 2; }
>> 
>> 2 is not the correct number of tail samples. :-)  The theoretical value is infinity, but pragmatic actual value depends on the filter coefficients and also depends on when we might consider the output close enough to zero.  As a first cut maybe just use 128?
> 
> Yes, I'm afraid I don't have the math necessary to calculate the tail time of a BiquadFilter.  It may be more correct to just return positive-infinity from BiquadFilter::tailTime().  This will cause a performance problem once silence propagation goes in, but some of that performance can be regained by modifying the Biquad processor to detect when it has generated silence and set the output bus as silent appropriately.

At this time, I wouldn't even bother defining tailSamples in this class (and the other lower-level Biquad classes), and instead just hard-code an "empirical" tailTime() value in BiquadFilterNode of say 200ms which should be more than sufficient.  We can add a FIXME there about possibly refining it later on.

> Source/WebCore/platform/audio/EqualPowerPanner.h:42
> +    virtual double tailTime() const OVERRIDE { return 0; }

Is this needed, since the default impl already returns 0?

>>> Source/WebCore/platform/audio/HRTFPanner.cpp:299
>>> +    return MaxDelayTimeSeconds;
>> 
>> This is not quite right.  We also need to add some time for the HRTF filter impulse responses themselves, and also an additional value due to the fact that the filters are implemented using a convolver with its own latency.  Perhaps as a first cut we can just use 512 sample frames because that's how much noteGrainOn always adds to handle the HRTF filters.  If we do just use 512, we should probably add a comment about doing a better job of figuring out the correct value.
> 
> This one is going to be even more difficult given that the impulse responses will change based on what azimuth and elevation are provided (and what previous azimuth and elevation parameters were!).
> 
> It might be easier to ask the HRTFDatabase what the maximum delay of its kernels is.  Then add in whatever latency is introduced by the convolvers.

All the impulse responses for all azimuth and elevation values should be of length fftSize()/2, so I think we can do:

MaxDelayTimeSeconds + fftSize()/2

> Source/WebCore/platform/audio/Panner.h:60
> +    virtual double tailTime() const = 0;

Does this need to be pure virtual -- maybe return 0 by default and then we don't need to implement for EqualPowerPanner

>>> Source/WebCore/webaudio/DelayNode.cpp:48
>>> +    return delayProcessor()->delayTime()->value();
>> 
>> Just curious.  The delay time can change, so you're allowing the tailTime to change with that?  Do you think there might be issues if the delay time is increasing and we'll always process enough to capture that?
> 
> The alternative would be to ask the kernel what it's tailTime() is?  Sure.

We might just have to return maxDelayTime because of Raymond's point.  Could bring up tricky bugs otherwise...

[Jer Noble]

(In reply to comment #9)
> (From update of attachment 130686 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130686&action=review
> 
> > Source/WebCore/platform/audio/AudioDSPKernel.h:66
> > +    virtual double tailTime() const = 0;
> 
> It's up to you how you want to stage the patches, but I think it would be a lot easier to kill two birds with one stone and also define the "latency()" method here and everywhere else since the changes are likely to occur in exactly the same places, and I think we'll need to have the latency() info (in addition to the tailTime) in order to implement silent hint propagation.

Sure.  I've updated the title of the bug to match.  (Though, I chose the name "latencyTime()" to explicitly differentiate between that and "latencySamples()" and "latencyFrames()".)

> >>> Source/WebCore/platform/audio/Biquad.h:85
> >>> +    int tailSamples() const { return 2; }
> >> 
> >> 2 is not the correct number of tail samples. :-)  The theoretical value is infinity, but pragmatic actual value depends on the filter coefficients and also depends on when we might consider the output close enough to zero.  As a first cut maybe just use 128?
> > 
> > Yes, I'm afraid I don't have the math necessary to calculate the tail time of a BiquadFilter.  It may be more correct to just return positive-infinity from BiquadFilter::tailTime().  This will cause a performance problem once silence propagation goes in, but some of that performance can be regained by modifying the Biquad processor to detect when it has generated silence and set the output bus as silent appropriately.
> 
> At this time, I wouldn't even bother defining tailSamples in this class (and the other lower-level Biquad classes), and instead just hard-code an "empirical" tailTime() value in BiquadFilterNode of say 200ms which should be more than sufficient.  We can add a FIXME there about possibly refining it later on.

Done.

> > Source/WebCore/platform/audio/EqualPowerPanner.h:42
> > +    virtual double tailTime() const OVERRIDE { return 0; }
> 
> Is this needed, since the default impl already returns 0?

No, and I've reorganized a lot of the headers to eliminate these extra declarations.

> >>> Source/WebCore/platform/audio/HRTFPanner.cpp:299
> >>> +    return MaxDelayTimeSeconds;
> >> 
> >> This is not quite right.  We also need to add some time for the HRTF filter impulse responses themselves, and also an additional value due to the fact that the filters are implemented using a convolver with its own latency.  Perhaps as a first cut we can just use 512 sample frames because that's how much noteGrainOn always adds to handle the HRTF filters.  If we do just use 512, we should probably add a comment about doing a better job of figuring out the correct value.
> > 
> > This one is going to be even more difficult given that the impulse responses will change based on what azimuth and elevation are provided (and what previous azimuth and elevation parameters were!).
> > 
> > It might be easier to ask the HRTFDatabase what the maximum delay of its kernels is.  Then add in whatever latency is introduced by the convolvers.
> 
> All the impulse responses for all azimuth and elevation values should be of length fftSize()/2, so I think we can do:
> 
> MaxDelayTimeSeconds + fftSize()/2

Changed.

> > Source/WebCore/platform/audio/Panner.h:60
> > +    virtual double tailTime() const = 0;
> 
> Does this need to be pure virtual -- maybe return 0 by default and then we don't need to implement for EqualPowerPanner

Again, the reorganization has fixed this one too.

> >>> Source/WebCore/webaudio/DelayNode.cpp:48
> >>> +    return delayProcessor()->delayTime()->value();
> >> 
> >> Just curious.  The delay time can change, so you're allowing the tailTime to change with that?  Do you think there might be issues if the delay time is increasing and we'll always process enough to capture that?
> > 
> > The alternative would be to ask the kernel what it's tailTime() is?  Sure.
> 
> We might just have to return maxDelayTime because of Raymond's point.  Could bring up tricky bugs otherwise...

Sure. Changed.

[Jer Noble]

Created attachment 131366 [details]
Patch

[Jer Noble]

The Biquad::latencySamples() might be wrong; I'd like to get an opinion on that.  And I made the AudioNode::tailTime() and ::latencyTime() functions pure virtual, which means a lot more boilerplate for subclasses, but with the benefit of ensuring new nodes have the correct values for those functions.

[Chris Rogers]

Comment on attachment 131366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131366&action=review

> Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:118
> +double AudioDSPKernelProcessor::tailTime() const

I think it will be sufficient to consider that the tailTime of each kernel is identical, so we can simply return the first kernel's tailTime()

> Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:129
> +double AudioDSPKernelProcessor::latencyTime() const

ditto

> Source/WebCore/platform/audio/Biquad.cpp:585
> +size_t Biquad::latencySamples() const

Normally a biquad's latency is considered to be 0.  Any tiny delays (group delays caused by phase response) are considered part of the effect itself and *not* actual latency (similar to DelayNode and ConvolverNode)

So I think we can remove this method, and simply return 0 in BiquadDSPKernel::latencyTime()

> Source/WebCore/platform/audio/Biquad.h:85
> +    size_t latencySamples() const;

as mentioned above, don't worry about adding this method

> Source/WebCore/platform/audio/HRTFPanner.cpp:299
> +    return MaxDelayTimeSeconds;

The HRTF processing is divided into two parts: a delay line, plus an FFTConvolver.  They each contribute to the tailTime, so we have:

MaxDelayTimeSeconds + (fftSize() / 2) / sampleRate();

It seems strange to see this value appear as part of the tailTime *and* for the latencyTime, but that's the nature of convolution as implemented by an FFTConvolver:
* FFTConvolver has a latency of fftSize()/2 because it uses overlap-add algorithm with FFT (but direct convolution algorithm has no latency)
* convolution in general has a tailTime equal to the length of the impulse response (which happens to equal fftSize()/2)

I think this is tricky enough that we should add comments here and in the latencyTime() method below to explain what's going on.

> Source/WebCore/platform/audio/Reverb.cpp:231
> +size_t Reverb::latencySamples() const

It shouldn't be necessary to find the max latency of all the convolvers because they should always all be equal.  It's sufficient to query the first one...

> Source/WebCore/platform/audio/ReverbConvolver.h:65
> +    size_t latencySamples() const { return m_minFFTSize / 2; }

I'd add a FIXME that the latency *should* be zero, and add a link to the bug:
https://bugs.webkit.org/show_bug.cgi?id=75564

> Source/WebCore/webaudio/AudioNode.h:139
>  

I think it would be worth adding short comments for each of these methods to make sure the definitions are clear.  You can probably improve the wording, but something like:

// tailTime() is the length of time (not counting latency time) where non-zero output may occur after continuous silent input.
// latencyTime() is the length of time it takes for non-zero output to appear after non-zero input is provided.  This only applies to processing delay
// which is an artifact of the processing algorithm chosen and is *not* part of the intrinsic desired effect.  For example, a "delay" effect is expected
// to delay the signal, and thus would not be considered latency.

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:142
> +    // an infinite tailTime. In practice, Biquad filters will not have a tailTime of longer

"will not" -> "will not usually (except for very high resonance values)"
We may consider detecting high-resonance cases in the future...

I'd move this comment to above line 37 (where you define MaxBiquadDelayTime)

[Raymond Toy]

Comment on attachment 131366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131366&action=review

> Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:123
> +        if (kernelTailTime > maxTailTime)

Maybe use std::max()?

> Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:134
> +        if (kernelLatencyTime > maxLatencyTime)

Maybe use std::max()?

> Source/WebCore/platform/audio/DynamicsCompressor.h:82
> +    double latencyTime() const { return m_compressor.latencyFrames()  / sampleRate(); }

Round-off error when dividing by float sampleRate().

> Source/WebCore/platform/audio/HRTFPanner.cpp:304
> +    return (fftSize() / 2) / sampleRate();

Maybe add MaxDelayTimeSeconds here?

Plus round-off issue when dividing by sampleRate() which is a float.

> Source/WebCore/platform/audio/Panner.h:61
> +    virtual double latencyTime() const = 0;

As Chris mentioned earlier, should these default to returning zero so that nothing needs to be done for the equal-power panner?  Only the HRTF panner would have a special implementation.

> Source/WebCore/platform/audio/Reverb.cpp:236
> +        if (maxLatency < convolverLatency)

Use std::max() here?

> Source/WebCore/webaudio/AudioNode.h:142
> +

Can we have some comments here to describe exactly what we mean by tailTime and latencyTime?

Also, is there a reason we don't define defaults here to return 0?  Do we really want to specify the implementation in every subclass? (Just curious.)

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:150
> +    return m_biquad.latencySamples() / sampleRate();

Round-off error here because sampleRate() is a float.  Probably want to cast this to a double.

Same issue in ConvolverNode.cpp for both tailTime() and latencyTime().

> Source/WebCore/webaudio/ConvolverNode.cpp:156
> +    return m_reverb ? m_reverb->impulseResponseLength() / sampleRate() : 0;

Round-off issue dividing by sampleRate() which is a float.

> Source/WebCore/webaudio/ConvolverNode.cpp:161
> +    return m_reverb ? m_reverb->latencySamples() / sampleRate() : 0;

Round-off issue dividing by sampleRate() which is a float.

[Jer Noble]

(In reply to comment #13)
> (From update of attachment 131366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131366&action=review
> 
> > Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:118
> > +double AudioDSPKernelProcessor::tailTime() const
> 
> I think it will be sufficient to consider that the tailTime of each kernel is identical, so we can simply return the first kernel's tailTime()

Changed.

> > Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:129
> > +double AudioDSPKernelProcessor::latencyTime() const
> 
> ditto

Ditto. :)

> > Source/WebCore/platform/audio/Biquad.cpp:585
> > +size_t Biquad::latencySamples() const
> 
> Normally a biquad's latency is considered to be 0.  Any tiny delays (group delays caused by phase response) are considered part of the effect itself and *not* actual latency (similar to DelayNode and ConvolverNode)
> 
> So I think we can remove this method, and simply return 0 in BiquadDSPKernel::latencyTime()

Okay, I had though that might be the case.  Changed.

> > Source/WebCore/platform/audio/Biquad.h:85
> > +    size_t latencySamples() const;
> 
> as mentioned above, don't worry about adding this method

Ditto.

> > Source/WebCore/platform/audio/HRTFPanner.cpp:299
> > +    return MaxDelayTimeSeconds;
> 
> The HRTF processing is divided into two parts: a delay line, plus an FFTConvolver.  They each contribute to the tailTime, so we have:
> 
> MaxDelayTimeSeconds + (fftSize() / 2) / sampleRate();
> 
> It seems strange to see this value appear as part of the tailTime *and* for the latencyTime, but that's the nature of convolution as implemented by an FFTConvolver:
> * FFTConvolver has a latency of fftSize()/2 because it uses overlap-add algorithm with FFT (but direct convolution algorithm has no latency)
> * convolution in general has a tailTime equal to the length of the impulse response (which happens to equal fftSize()/2)
> 
> I think this is tricky enough that we should add comments here and in the latencyTime() method below to explain what's going on.

It might be more self-explanitory if we passed the tailTime() and latencyTime() calls down to FFTConvolver, where the explanation above makes more sense.

> > Source/WebCore/platform/audio/Reverb.cpp:231
> > +size_t Reverb::latencySamples() const
> 
> It shouldn't be necessary to find the max latency of all the convolvers because they should always all be equal.  It's sufficient to query the first one...

Changed.

> > Source/WebCore/platform/audio/ReverbConvolver.h:65
> > +    size_t latencySamples() const { return m_minFFTSize / 2; }
> 
> I'd add a FIXME that the latency *should* be zero, and add a link to the bug:
> https://bugs.webkit.org/show_bug.cgi?id=75564

Sure thing.

> > Source/WebCore/webaudio/AudioNode.h:139
> >  
> 
> I think it would be worth adding short comments for each of these methods to make sure the definitions are clear.  You can probably improve the wording, but something like:
> 
> // tailTime() is the length of time (not counting latency time) where non-zero output may occur after continuous silent input.
> // latencyTime() is the length of time it takes for non-zero output to appear after non-zero input is provided.  This only applies to processing delay
> // which is an artifact of the processing algorithm chosen and is *not* part of the intrinsic desired effect.  For example, a "delay" effect is expected
> // to delay the signal, and thus would not be considered latency.

This sounds fine to me. :)

> > Source/WebCore/webaudio/BiquadDSPKernel.cpp:142
> > +    // an infinite tailTime. In practice, Biquad filters will not have a tailTime of longer
> 
> "will not" -> "will not usually (except for very high resonance values)"
> We may consider detecting high-resonance cases in the future...
> 
> I'd move this comment to above line 37 (where you define MaxBiquadDelayTime)

Changed.

[Jer Noble]

(In reply to comment #14)
> (From update of attachment 131366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131366&action=review
> 
> > Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:123
> > +        if (kernelTailTime > maxTailTime)
> 
> Maybe use std::max()?

As per Chris's suggestion, this is no longer doing a max.

> > Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:134
> > +        if (kernelLatencyTime > maxLatencyTime)
> 
> Maybe use std::max()?

Ditto.

> > Source/WebCore/platform/audio/DynamicsCompressor.h:82
> > +    double latencyTime() const { return m_compressor.latencyFrames()  / sampleRate(); }
> 
> Round-off error when dividing by float sampleRate().

Changed.

> > Source/WebCore/platform/audio/HRTFPanner.cpp:304
> > +    return (fftSize() / 2) / sampleRate();
> 
> Maybe add MaxDelayTimeSeconds here?
> 
> Plus round-off issue when dividing by sampleRate() which is a float.
> 
> > Source/WebCore/platform/audio/Panner.h:61
> > +    virtual double latencyTime() const = 0;
> 
> As Chris mentioned earlier, should these default to returning zero so that nothing needs to be done for the equal-power panner?  Only the HRTF panner would have a special implementation.
> 
> > Source/WebCore/platform/audio/Reverb.cpp:236
> > +        if (maxLatency < convolverLatency)
> 
> Use std::max() here?
> 
> > Source/WebCore/webaudio/AudioNode.h:142
> > +
> 
> Can we have some comments here to describe exactly what we mean by tailTime and latencyTime?
> 
> Also, is there a reason we don't define defaults here to return 0?  Do we really want to specify the implementation in every subclass? (Just curious.)

Originally, I made them pure virtual to ensure that all the subclasses had the correct values (in that leaving a node unspecified would be a compile-time error).  I think that's still a good idea.

> > Source/WebCore/webaudio/BiquadDSPKernel.cpp:150
> > +    return m_biquad.latencySamples() / sampleRate();
> 
> Round-off error here because sampleRate() is a float.  Probably want to cast this to a double.
> 
> Same issue in ConvolverNode.cpp for both tailTime() and latencyTime().

Changed in all places.

> > Source/WebCore/webaudio/ConvolverNode.cpp:156
> > +    return m_reverb ? m_reverb->impulseResponseLength() / sampleRate() : 0;
> 
> Round-off issue dividing by sampleRate() which is a float.
> 
> > Source/WebCore/webaudio/ConvolverNode.cpp:161
> > +    return m_reverb ? m_reverb->latencySamples() / sampleRate() : 0;
> 
> Round-off issue dividing by sampleRate() which is a float.

Changed.

[Jer Noble]

Created attachment 131411 [details]
Patch

[Raymond Toy]

Comment on attachment 131411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131411&action=review

Other than the one (or two) nits, this looks really nice.

> Source/WebCore/platform/audio/AudioDSPKernelProcessor.cpp:120
> +    return !m_kernels.isEmpty() ? m_kernels.first()->tailTime() : 0;

nit: Add comment here that we expect all the kernels to have the same tailTime.  Same for line 125.

[Raymond Toy]

(In reply to comment #16)
> 
> Originally, I made them pure virtual to ensure that all the subclasses had the correct values (in that leaving a node unspecified would be a compile-time error).  I think that's still a good idea.

Yes, this makes it obvious when something new is added that the latency and tail times need to be considered.

[Chris Rogers]

Comment on attachment 131411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131411&action=review

Looks almost ready - just some nits:

> Source/WebCore/platform/audio/DynamicsCompressor.h:82
> +    double latencyTime() const { return m_compressor.latencyFrames()  / static_cast<double>(sampleRate()); }

nit: extra space before /

> Source/WebCore/platform/audio/Reverb.cpp:231
> +size_t Reverb::latencySamples() const

"frames" is more accurate than "samples"

latencySamples() -> latencyFrames()

here and in similar places

> Source/WebCore/webaudio/BiquadDSPKernel.cpp:40
> +// not have a tailTime of longer than approx. 200ms. This value could possibly be calculated based

nit: fix funny wording here

[Jer Noble]

Created attachment 131428 [details]
Patch

[Jer Noble]

Thanks!

[Jer Noble]

Committed r110507: <http://trac.webkit.org/changeset/110507>