Bug 45077

Summary: Add EqualPowerPanner files
Product: WebKit Reporter: Chris Rogers <crogers>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, commit-queue, dglazkov, eric.carlson, japhet, jer.noble, kbr, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Rogers 2010-09-01 17:48:57 PDT
Add EqualPowerPanner files
Comment 1 Chris Rogers 2010-09-01 17:50:14 PDT
Created attachment 66307 [details]
Patch
Comment 2 Kenneth Russell 2010-09-07 17:44:03 PDT
Comment on attachment 66307 [details]
Patch

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

> WebCore/platform/audio/EqualPowerPanner.cpp:85
> +    const double kDezipperK = 0.0005;
This or similar constants are replicated in a few places in the code, for example in the AudioBus (where it's ten times as large). Can this be refactored? Also, what about a FIXME indicating that it's sample rate dependent?

> WebCore/platform/audio/EqualPowerPanner.cpp:98
> +    m_gainR = gainR;
Why does this class not need to expose these gains externally, like the AudioBus does via the lastMixGain outgoing argument in processWithGainFromMonoStereo and processWithGainFrom?
Comment 3 Chris Rogers 2010-10-01 14:22:04 PDT
Created attachment 69518 [details]
Patch
Comment 4 Chris Rogers 2010-10-01 14:27:08 PDT
(In reply to comment #2)
> (From update of attachment 66307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66307&action=prettypatch
> 
> > WebCore/platform/audio/EqualPowerPanner.cpp:85
> > +    const double kDezipperK = 0.0005;
> This or similar constants are replicated in a few places in the code, for example in the AudioBus (where it's ten times as large). Can this be refactored? Also, what about a FIXME indicating that it's sample rate dependent?

I've uploaded a patch for AudioUtilities which contains a discreteTimeConstantForSampleRate() function and am now using this.
The actual time-constant value (non sample-rate dependent) is chosen empirically for panning and may be different than volume smoothing.

> 
> > WebCore/platform/audio/EqualPowerPanner.cpp:98
> > +    m_gainR = gainR;
> Why does this class not need to expose these gains externally, like the AudioBus does via the lastMixGain outgoing argument in processWithGainFromMonoStereo and processWithGainFrom?

The smoothing state is best kept inside this class.  The reason they have to be kept externally in the AudioBus case is because many different sources may be summing into the same AudioBus, thus each source must keep track of its own state.  In the case of EqualPowerPanner, there is only a single source, so it makes logical sense to keep the state inside this class.
Comment 5 Kenneth Russell 2010-10-04 15:13:50 PDT
Comment on attachment 69518 [details]
Patch

Looks good to me, but please update the license text before committing.
Comment 6 Chris Rogers 2010-10-04 15:32:03 PDT
Created attachment 69700 [details]
Patch
Comment 7 Chris Rogers 2010-10-04 15:32:38 PDT
Was previously R+, just fixed the license.
Comment 8 Kenneth Russell 2010-10-04 15:47:01 PDT
Comment on attachment 69700 [details]
Patch

Looks good to me.
Comment 9 WebKit Commit Bot 2010-10-05 00:45:22 PDT
Comment on attachment 69700 [details]
Patch

Clearing flags on attachment: 69700

Committed r69089: <http://trac.webkit.org/changeset/69089>
Comment 10 WebKit Commit Bot 2010-10-05 00:45:28 PDT
All reviewed patches have been landed.  Closing bug.