Bug 81092 – add stereo source support in EqualPowerPanner

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

[Wei James (wistoch)]

add stereo source support in EqualPowerPanner

[Wei James (wistoch)]

Created attachment 131810 [details]
Patch

[Chris Rogers]

Comment on attachment 131810 [details]
Patch

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

James, thanks for thinking about this and working on it!

> Source/WebCore/platform/audio/EqualPowerPanner.cpp:112
> +        *destinationR++ = static_cast<float>((inputL + inputR) / 2 * gainR);

There's a better technique for panning stereo sources where the stereo information is not lost.  Here's how the algorithm works:

* If the azimuth is 0 then pass the signal through unchanged: sourceL -> destL, sourceR -> destR
* If the azimuth is -90 then only pass the left: sourceL -> destL, silence -> destR
* If the azimuth is +90 then only pass the right: silence -> destL, sourceR -> destR
* If -90 <= azimuth <= 0 then: sourceL -> destL, and "equal-power pan" sourceR by transforming the "azimuth" value from the range -90 -> 0 into the range -90 -> +90 and then use the normal equal-power panning equation as in mono
* if  0 <= azimuth <= +90 then similar to previous case but reverse the roles of left and right

It should be possible to code this algorithm so that the logic is clear to read and understand.

Also, I would create a separate code-path for the simple mono case (that's in the current code)

[Chris Rogers]

I'm sorry, I made a mistake about the fully panned -90 and +90 positions.  In these cases, please follow the panning rules in the last two steps.  I hope this gives the general idea.

[Wei James (wistoch)]

Created attachment 132548 [details]
Patch

[Chris Rogers]

Comment on attachment 132548 [details]
Patch

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

James, thanks for the updated patch:

> Source/WebCore/platform/audio/EqualPowerPanner.cpp:99
> +            desiredGainR = sin(0.5 * piDouble * desiredPanPosition);

This is close, but is not accounting for the right channel's contribution to the mix completely.  For example, consider if azimuth==-90.  With this current code the right channel will not even appear in the mix at all.  But what we want is for the right channel input to appear in the mix completely panned to the left (and summed with the left channel).

outputL = inputL + inputR * cos(0.5 * piDouble * desiredPanPosition);
outputR = inputR * sin(0.5 * piDouble * desiredPanPosition);

And similarly for the 0->+90 case below (with roles of left and right reversed)

It will probably be easiest to write this code if you use separate while loops for the mono vs. stereo cases replacing the while() loop on line 124.

[Wei James (wistoch)]

Comment on attachment 132548 [details]
Patch

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

>> Source/WebCore/platform/audio/EqualPowerPanner.cpp:99
>> +            desiredGainR = sin(0.5 * piDouble * desiredPanPosition);
> 
> This is close, but is not accounting for the right channel's contribution to the mix completely.  For example, consider if azimuth==-90.  With this current code the right channel will not even appear in the mix at all.  But what we want is for the right channel input to appear in the mix completely panned to the left (and summed with the left channel).
> 
> outputL = inputL + inputR * cos(0.5 * piDouble * desiredPanPosition);
> outputR = inputR * sin(0.5 * piDouble * desiredPanPosition);
> 
> And similarly for the 0->+90 case below (with roles of left and right reversed)
> 
> It will probably be easiest to write this code if you use separate while loops for the mono vs. stereo cases replacing the while() loop on line 124.

thanks for the guidance. I will fix it. thanks

[Wei James (wistoch)]

Created attachment 132783 [details]
Patch

[Chris Rogers]

Comment on attachment 132783 [details]
Patch

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

> Source/WebCore/platform/audio/EqualPowerPanner.cpp:54
> +    unsigned inputChannelNumber = inputBus->numberOfChannels();

inputChannelNumber -> numberOfChannels    (or numberOfInputChannels)

> Source/WebCore/platform/audio/EqualPowerPanner.cpp:125
> +            float inputR = *sourceR++;

Why are we accessing two inputs (inputL and inputR) when this is the mono case?

> Source/WebCore/platform/audio/EqualPowerPanner.cpp:148
> +                *destinationR++ = static_cast<float>(inputR + inputR * gainR);

I think this should be (inputR + inputL * gainR)

Also please check layout test below to see why it was passing with this mistake

> LayoutTests/webaudio/resources/panner-model-testing.js:66
> +    channelNumber = sourceChannelNumber;

sourceChannelNumber -> numberOfSourceChannels
channelNumber -> numberOfChannels

[Wei James (wistoch)]

Created attachment 132989 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 132783 [details]
Patch

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

>> Source/WebCore/platform/audio/EqualPowerPanner.cpp:54
>> +    unsigned inputChannelNumber = inputBus->numberOfChannels();
> 
> inputChannelNumber -> numberOfChannels    (or numberOfInputChannels)

fixed. thanks

>> Source/WebCore/platform/audio/EqualPowerPanner.cpp:125
>> +            float inputR = *sourceR++;
> 
> Why are we accessing two inputs (inputL and inputR) when this is the mono case?

In previous patch, I want to reduce the code redundancy so merge three while loop into one. 

fixed in this patch. thanks.

>> Source/WebCore/platform/audio/EqualPowerPanner.cpp:148
>> +                *destinationR++ = static_cast<float>(inputR + inputR * gainR);
> 
> I think this should be (inputR + inputL * gainR)
> 
> Also please check layout test below to see why it was passing with this mistake

sorry for this mistake. fixed. 

because both inputL and inputR have the same value. so the test pass without error.

>> LayoutTests/webaudio/resources/panner-model-testing.js:66
>> +    channelNumber = sourceChannelNumber;
> 
> sourceChannelNumber -> numberOfSourceChannels
> channelNumber -> numberOfChannels

fixed. thanks

[Wei James (wistoch)]

(In reply to comment #10)
> (From update of attachment 132783 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132783&action=review
> 
> >> Source/WebCore/platform/audio/EqualPowerPanner.cpp:54
> >> +    unsigned inputChannelNumber = inputBus->numberOfChannels();
> > 
> > inputChannelNumber -> numberOfChannels    (or numberOfInputChannels)
> 
> fixed. thanks
> 
> >> Source/WebCore/platform/audio/EqualPowerPanner.cpp:125
> >> +            float inputR = *sourceR++;
> > 
> > Why are we accessing two inputs (inputL and inputR) when this is the mono case?
> 
> In previous patch, I want to reduce the code redundancy so merge three while loop into one. 
> 
> fixed in this patch. thanks.
> 
> >> Source/WebCore/platform/audio/EqualPowerPanner.cpp:148
> >> +                *destinationR++ = static_cast<float>(inputR + inputR * gainR);
> > 
> > I think this should be (inputR + inputL * gainR)
> > 
> > Also please check layout test below to see why it was passing with this mistake
> 
> sorry for this mistake. fixed. 
> 
> because both inputL and inputR have the same value. so the test pass without error.
> 
> >> LayoutTests/webaudio/resources/panner-model-testing.js:66
> >> +    channelNumber = sourceChannelNumber;
> > 
> > sourceChannelNumber -> numberOfSourceChannels
> > channelNumber -> numberOfChannels
> 
> fixed. thanks

chris, is this patch ok? thanks

[Chris Rogers]

Comment on attachment 132989 [details]
Patch

Thanks James!

[WebKit Review Bot]

Comment on attachment 132989 [details]
Patch

Clearing flags on attachment: 132989

Committed r113889: <http://trac.webkit.org/changeset/113889>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.