Add multi-channels support for CopyWithGainFrom in AudioBus

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond]

Add multi-channels support for CopyWithGainFrom in AudioBus

[Raymond]

Created attachment 130972 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130972 [details]
Patch

Attachment 130972 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11894398

[Raymond]

Hi Chris

This patch is aim to enable multi-channles support in CopyWithGainFrom, so that GainNode can handle 5.1 channels.

1. At present CopyWithGainFrom will only been called from the case that channels' topology matching. So I separate the function into matching/unmatching case. In the matching case, all 1-6 channels scenario are handled. And in the unmatching case, reserve the original mono->stereo case. By this way, if further up/down mix case is needed. it will be more easy to be add and more easy to read the code.

2. to minimize the impact of looping channels for gain processing, I define a lot of macros to expand for different channels, thus eliminate the need for a inner loop. while it do increase the code size, hope this won't be a issue.

By the way, With another few code tweaked in AudioDestination Node, I managed to play a 5.1 audio in a real 5.1 audio card. But there are some issue I report at :
http://code.google.com/p/chromium/issues/detail?id=117333 Seems alsa output on 5.1 in chromium is not working well. But anyway, it's not webaudio specific issue. Webaudio part seems working correctly

[Build Bot]

Comment on attachment 130972 [details]
Patch

Attachment 130972 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11894400

[Raymond]

Created attachment 130977 [details]
Patch

[Raymond]

build issue fixed.

[Chris Rogers]

Hi Raymond, I'm glad you're looking into one of the last multi-channel implementation details (AudioGainNode).  Also, glad to hear you're starting to play around with a 5.1 sound card.  It'll be very interesting to get this all working!

In looking at AudioGainNode I see that it's only necessary to implement:
processMatchingBusWithGainFrom() because the number of channels of an AudioGainNode input is the same number as the output.  Furthermore, AudioGainNode only uses copyWithGainFrom() and *not* sumWithGainFrom() so let's not worry about the multi-channel SUM case right now (we may never need it in the engine...).

I'm a bit concerned about the complexity of the macros that you're proposing in this patch.  I'd like to propose a much simpler solution.  Keep the existing code for the mono and stereo cases.  Then for N -> N copy do the following:

1) If gain has already converged/snapped to targetGain then just create simple loops (iterating over the N channels) calling into vsmul() for copy or vsma() for sum
2) If gain hasn't yet converged to targetGain, then (with very simple code) calculate an array of gain values (using the GAIN_DEZIPPER macro or similar), then simply call the existing copyWithSampleAccurateGainValuesFrom() method.  You can include a FIXME that we don't handle the SUM case, but I think we'll never need it anyway.

Given (1) and (2) I believe the code can be written in a very straight-forward (easy to read) manner and should even be fairly efficient.

[Raymond]

Created attachment 131556 [details]
Patch

[Raymond]

Hi Chris

Thanks for the review.

I simplify the macro according to your comments. Remove the codes that handle sumToBus and unmatching channels case. While, I still reserve the original overall code structure that invoke PROCESS_WITH_GAIN with different func macro. In this way, if we later by any chance want to handle sumToBus and unmatching channels case (maybe some node other than GainNode will call it? or some other function can share the code?) It will be easy.

And If we really believe these two cases will never be needed. We can further simplify the code by remove all those macros ( including PROCESS_WITH_GAIN ) and do loop directly in copyWithGainFrom in the future. How do you think about it?

[Chris Rogers]

(In reply to comment #9)
> Hi Chris
> 
> Thanks for the review.
> 
> I simplify the macro according to your comments. Remove the codes that handle sumToBus and unmatching channels case. While, I still reserve the original overall code structure that invoke PROCESS_WITH_GAIN with different func macro. In this way, if we later by any chance want to handle sumToBus and unmatching channels case (maybe some node other than GainNode will call it? or some other function can share the code?) It will be easy.
> 
> And If we really believe these two cases will never be needed. We can further simplify the code by remove all those macros ( including PROCESS_WITH_GAIN ) and do loop directly in copyWithGainFrom in the future. How do you think about it?

Hi Raymond, the more I think about it, it seems unlikely that we'll ever need the extra logic to handle sumToBus and unmatching channels case.  If that changes in the future then we can always add the more complex logic.  But, I think for now (because this seems quite unlikely to me) the best path is to keep a sharp focus on keeping the code as simple and straightforward as possible (for readability and maintainability).  So I like your idea about further simplifying, removing the macros, etc.  We always have the patch (with macros) on record in this bug if we need to re-visit.

Also, it would be nice to handle arbitrary numbers of channels using loops, and not limit ourselves to 5.1.  I see you have a constant "MaxBusChannels = 6;" but now AudioContext thinks it can go higher (see AudioContext::maxNumberOfChannels()).  We can't reference AudioContext from AudioBus (layering violation) but we should make our MaxChannels value the same (32) and probably only need to use it when call ASSERT in the constructor...

[Raymond]

Created attachment 132202 [details]
Patch

[Raymond]

(In reply to comment #10)
Hi Chris

Patch updated. And I think, since we simplify the code structure. It might be good time for us to improve the gain dezipper code now. So I also add a FIXME there to remind it.

[Chris Rogers]

Comment on attachment 132202 [details]
Patch

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

> Source/WebCore/platform/audio/AudioBus.cpp:323
> +    // If it is copying from the same bus and no need to change gain, just return

nit: period at end of sentence

> Source/WebCore/platform/audio/AudioBus.cpp:324
> +    if (this == &sourceBus && *lastMixGain == targetGain && targetGain == 1.0)

WebKit style nit: 1.0 -> 1

> Source/WebCore/platform/audio/AudioBus.cpp:327
> +    unsigned numberOfChannels = this->numberOfChannels();

We need to have both an ASSERT and an early return doing a sanity check: numberOfChannels <= MaxBusChannels
because of the loop below

> Source/WebCore/platform/audio/AudioBus.cpp:348
> +    float gainValues[AudioNode::ProcessingSizeInFrames];

Two things:

1. For arrays of this size, it's much better to use an OwnPtr<AudioFloatArray> instead of a stack-based array
2. There is a layering violation here because this class (AudioBus) should not know anything about AudioNode which lives outside of the WebCore/platform directory.
Also, we shouldn't make any assumptions here that the array size of AudioNode::ProcessingSizeInFrames is sufficiently large, but instead should be allocated to the exact size (based on framesToProcess and/or framesToDezipper)

> Source/WebCore/platform/audio/AudioBus.cpp:363
> +    for (k = 0; k < framesToDezipper; ++k) {

can we remove line 361 "int k = 0" and define "k" directly "for (unsigned k = 0"

> Source/WebCore/platform/audio/AudioBus.cpp:365
> +        gain = DenormalDisabler::flushDenormalFloatToZero(gain);

Please add FIXME that we can probably get rid of this DenormalDisabler::flushDenormalFloatToZero() call if we are clever enough in calculating the framesToDezipper value if totalDesiredGain==0.
In the case where both "gain" and "totalDesiredGain" are larger than the denormal threshold, then it should never be necessary to even check for denormals, since all intermediate values should also not be denormals.

> Source/WebCore/platform/audio/AudioBus.cpp:375
>      } else {

WebKit style nit: remove braces { } on single-line else clause....

> Source/WebCore/platform/audio/AudioBus.cpp:378
>  

Just to make things more clear, I would wrap the following for() loop in:

if (framesToDezipper < framesToProcess)

> Source/WebCore/platform/audio/AudioBus.cpp:380
> +        vsmul(sources[index], 1, &gain, destinations[index], 1, framesToProcess - k);

It seems odd to use the loop variable "k" here -- instead "framesToDezipper" is probably better.

[Raymond]

Created attachment 132529 [details]
Patch

[Raymond]

(In reply to comment #13)
Hi Chris

Thanks for the review. Patch updated according to your comments.

And :

> > Source/WebCore/platform/audio/AudioBus.cpp:327
> > +    unsigned numberOfChannels = this->numberOfChannels();
> 
> We need to have both an ASSERT and an early return doing a sanity check: numberOfChannels <= MaxBusChannels
> because of the loop below

I remove them because you have mentioned that we only need to access it in the constructor ;) And we had a ASSERT there in the constructor,But For Release case, to make it safe, I add:

if (numberOfChannels > MaxBusChannels)
    numberOfchannels = MaxBusChannels;

This way, we won't need to check numberOfChannels in copyWithGainFrom.

How do you think about this solution?

[Chris Rogers]

Comment on attachment 132529 [details]
Patch

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

Raymond, thanks for the latest fixes.  I have a few more comments:

> Source/WebCore/platform/audio/AudioBus.cpp:59
> +        numberOfChannels = MaxBusChannels;

I don't think we should proceed at this point since this is an anomalous condition.  Instead we should just return early and allocate nothing.

> Source/WebCore/platform/audio/AudioBus.cpp:68
> +    m_gainValues = adoptPtr(new AudioFloatArray(length));

We shouldn't allocate m_gainValues here since it will often be unused and could be potentially very large (for example, loading large audio assets into an AudioBus)

> Source/WebCore/platform/audio/AudioBus.cpp:81
> +        }

I would just revert these changes in AudioBus::setChannelMemory().
I think this is getting too complex.

> Source/WebCore/platform/audio/AudioBus.cpp:334
> +    unsigned numberOfChannels = this->numberOfChannels();

For safety/security reasons, we still need to have an ASSERT and early return if numberOfChannels is too big.  It's true that the constructor checked this, but an ASSERT is for safety reasons and is supposed to check for conditions that never *should* be possible given the way the code is understood to work.  So simply having the check in the constructor is not sufficient, especially when we're writing values into stack-based arrays

> Source/WebCore/platform/audio/AudioBus.cpp:366
> +

Instead of initializing m_gainValues in the constructor and setChannelMemory(), we can simply *lazily* initialize it here (taking care that if m_gainValues is already initialized, that it is of a large enough size)

> Source/WebCore/platform/audio/AudioBus.cpp:379
> +        gainValues = m_gainValues->data();

Can't we simply remove 378:379 and directly use "m_gainValues->data()" on line 382

> Source/WebCore/platform/audio/AudioBus.cpp:381
> +        for (unsigned index = 0; index < numberOfChannels; ++index) {

"index" -> "channelIndex"

> Source/WebCore/platform/audio/AudioBus.cpp:388
> +

Please add comment:

// Apply constant gain after de-zippering has converged on target gain.

> Source/WebCore/platform/audio/AudioBus.cpp:390
> +        for (unsigned index = 0; index < numberOfChannels; ++index)

"index" -> "channelIndex"

> Source/WebCore/platform/audio/AudioBus.h:139
> +    OwnPtr<AudioFloatArray> m_gainValues; // Store gain values array for dezipper frames.

Maybe a better name would be "m_dezipperGainValues" and then you can remove the comment because the variable name is self-explanatory.

[Raymond]

Created attachment 133172 [details]
Patch

[Raymond]

(In reply to comment #16)
Hi Chris

Thanks for the review. Patch updated.

> > Source/WebCore/platform/audio/AudioBus.cpp:379
> > +        gainValues = m_gainValues->data();
> 
> Can't we simply remove 378:379 and directly use "m_gainValues->data()" on line 382

I have thought that, inside a loop, m_gainValues->data() will been retrieved for several time, so fetch it out before hand. Anyway, updated according to your comments. Maybe the compiler is clever enough to optimize this?

[Raymond]

Hi Chris

Anything else I need to do for this patch ;)

[Raymond]

Hi Chris

Any more comments on this patch?

[Chris Rogers]

(In reply to comment #20)
> Hi Chris
> 
> Any more comments on this patch?

Sorry, I'll try to get to this as soon as possible.

[Raymond]

Created attachment 137830 [details]
Patch

[Raymond]

Hi Chris

Patch updated to sync with the latest code change in audiobus.cc

[Raymond]

Hi Chris
Any news on this patch? ;) We have some other patches for audioBus coming, it will be better to be submit after this one is done to save the rebase work.

[Chris Rogers]

(In reply to comment #24)
> Hi Chris
> Any news on this patch? ;) We have some other patches for audioBus coming, it will be better to be submit after this one is done to save the rebase work.

Hi Raymond, sorry for the delay.  Work has been very heavy recently.  I'll try my best to review as soon as I can.

[Chris Rogers]

Comment on attachment 137830 [details]
Patch

Thanks Raymond - nice simplification!  Sorry I've taken so long on this one.

[WebKit Review Bot]

Comment on attachment 137830 [details]
Patch

Clearing flags on attachment: 137830

Committed r116596: <http://trac.webkit.org/changeset/116596>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.