Bug 77609 - AudioBus need to support stereo->mono down mix in copyFrom sumFrom etc.
Summary: AudioBus need to support stereo->mono down mix in copyFrom sumFrom etc.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wei James (wistoch)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-01 23:22 PST by Raymond
Modified: 2012-02-06 15:00 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.84 KB, patch)
2012-02-02 00:13 PST, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2012-02-02 23:23 PST, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (7.53 KB, patch)
2012-02-03 00:57 PST, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (7.69 KB, patch)
2012-02-03 19:29 PST, Wei James (wistoch)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond 2012-02-01 23:22:08 PST
Hi

Usually this is not a big issue. But in the situation that :

A OfflineContext with mono channel buffer is connected to a RealtimeAnalyserNode, things go bad. Since RealtimeAnalyserNode is hardcoded to stereo, in the debug mode, this will lead to crash by ASSERT in AudioBuscopyFrom. And the best case is in release mode, it just return, thus no music is outputed. And this could also happen for other nodes if source and destination's channels not match.

But in this very case, the user might never know what is going on, for even the source buffer is mono, offlineContext buffer is mono, it still go wrong, he will never know that realtimeAnalyserNode rune everything.

I think the solution is :

1. make RealtimeAnalyserNode more clever and can adjust its output channel to suit for input channel
2. support down mix in audioBus

2 is a must. while 1 might be a addon.
Comment 1 Raymond 2012-02-01 23:30:06 PST
a crash example can be found at :
https://bugs.webkit.org/show_bug.cgi?id=77515#c3
Comment 2 Wei James (wistoch) 2012-02-02 00:13:21 PST
Created attachment 125088 [details]
Patch
Comment 3 Chris Rogers 2012-02-02 11:49:00 PST
Thanks James, the changes look good.  Could you please write a very simple layout test which demonstrates that the ASSERT no longer happens?  I think it could be a very simple test with a mono OfflineAudioContext and a RealtimeAnalyserNode (or DynamicsCompressorNode also I think could work) as Raymond describes in his initial comment.
Comment 4 Wei James (wistoch) 2012-02-02 23:23:06 PST
Created attachment 125271 [details]
Patch
Comment 5 Wei James (wistoch) 2012-02-02 23:24:49 PST
(In reply to comment #3)
> Thanks James, the changes look good.  Could you please write a very simple layout test which demonstrates that the ASSERT no longer happens?  I think it could be a very simple test with a mono OfflineAudioContext and a RealtimeAnalyserNode (or DynamicsCompressorNode also I think could work) as Raymond describes in his initial comment.

chris, patch update including a layout test. please review. thanks
Comment 6 Wei James (wistoch) 2012-02-03 00:57:52 PST
Created attachment 125281 [details]
Patch
Comment 7 Chris Rogers 2012-02-03 10:37:47 PST
Comment on attachment 125281 [details]
Patch

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

Looks good, if you add a few minor explanatory comments:

> LayoutTests/webaudio/stereo2mono-down-mixing.html:53
> +    context = new webkitAudioContext(1, sampleRate * renderLengthSeconds, sampleRate);

could add comment:

// Explicitly create a mono AudioContext so that the destination is mono.

> LayoutTests/webaudio/stereo2mono-down-mixing.html:58
> +

could add comment:

// We create an analyser here, because it has a hard-coded stereo output.
Comment 8 Wei James (wistoch) 2012-02-03 19:29:36 PST
Created attachment 125467 [details]
Patch
Comment 9 Wei James (wistoch) 2012-02-03 19:31:32 PST
(In reply to comment #7)
> (From update of attachment 125281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125281&action=review
> 
> Looks good, if you add a few minor explanatory comments:
> 
> > LayoutTests/webaudio/stereo2mono-down-mixing.html:53
> > +    context = new webkitAudioContext(1, sampleRate * renderLengthSeconds, sampleRate);
> 
> could add comment:
> 
> // Explicitly create a mono AudioContext so that the destination is mono.
> 
> > LayoutTests/webaudio/stereo2mono-down-mixing.html:58
> > +
> 
> could add comment:
> 
> // We create an analyser here, because it has a hard-coded stereo output.

thanks. patch updated to including the comments. thanks
Comment 10 Chris Rogers 2012-02-03 20:14:01 PST
Looks good.
Comment 11 Kenneth Russell 2012-02-06 13:25:42 PST
Comment on attachment 125467 [details]
Patch

rs=me
Comment 12 WebKit Review Bot 2012-02-06 15:00:47 PST
Comment on attachment 125467 [details]
Patch

Clearing flags on attachment: 125467

Committed r106858: <http://trac.webkit.org/changeset/106858>
Comment 13 WebKit Review Bot 2012-02-06 15:00:54 PST
All reviewed patches have been landed.  Closing bug.