Bug 75835 - Add vsma in VectorMath to handle vector scale multiply and add and use it in AudioBus
Summary: Add vsma in VectorMath to handle vector scale multiply and add and use it in ...
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: 74345
  Show dependency treegraph
 
Reported: 2012-01-09 01:03 PST by Wei James (wistoch)
Modified: 2012-01-12 20:28 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.94 KB, patch)
2012-01-09 01:14 PST, Wei James (wistoch)
no flags Details | Formatted Diff | Diff
Patch (5.78 KB, patch)
2012-01-12 16:49 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 Wei James (wistoch) 2012-01-09 01:03:23 PST
Add vsma in VectorMath to handle vector scale multiply and add and use it in AudioBus
Comment 1 Wei James (wistoch) 2012-01-09 01:14:23 PST
Created attachment 121630 [details]
Patch
Comment 2 Raymond Toy 2012-01-11 09:50:29 PST
Comment on attachment 121630 [details]
Patch

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

> Source/WebCore/platform/audio/VectorMath.cpp:142
> +

Are we missing the case where n (framesToProcess) is not a multiple of 4?  If framesToProcess is 5, tailFrames = 1 and endP = destP + 4.  The while loop will be executed once, processing 4 frames.  We're missing the last frame, right?
Comment 3 Wei James (wistoch) 2012-01-11 16:41:34 PST
Comment on attachment 121630 [details]
Patch

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

>> Source/WebCore/platform/audio/VectorMath.cpp:142
>> +
> 
> Are we missing the case where n (framesToProcess) is not a multiple of 4?  If framesToProcess is 5, tailFrames = 1 and endP = destP + 4.  The while loop will be executed once, processing 4 frames.  We're missing the last frame, right?

if the n is not a multiple of 4, in the line 148, n will be assigned to tailFrames, and the code after the #endif will be executed to handle the last frame.
Comment 4 Raymond Toy 2012-01-12 09:49:40 PST
Comment on attachment 121630 [details]
Patch

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

>>> Source/WebCore/platform/audio/VectorMath.cpp:142
>>> +
>> 
>> Are we missing the case where n (framesToProcess) is not a multiple of 4?  If framesToProcess is 5, tailFrames = 1 and endP = destP + 4.  The while loop will be executed once, processing 4 frames.  We're missing the last frame, right?
> 
> if the n is not a multiple of 4, in the line 148, n will be assigned to tailFrames, and the code after the #endif will be executed to handle the last frame.

Sorry about that.  Looks good, then.
Comment 5 Chris Rogers 2012-01-12 14:55:59 PST
Looks good.
Comment 6 Jer Noble 2012-01-12 15:40:45 PST
The patch in bug #74345 also adds vsma support.  If both patches are approved, we'll have to make sure not to step on each others' toes.
Comment 7 Chris Rogers 2012-01-12 15:53:23 PST
(In reply to comment #6)
> The patch in bug #74345 also adds vsma support.  If both patches are approved, we'll have to make sure not to step on each others' toes.

Agreed.  I think it's easiest if we can land this patch first.  And then #74345 can be simplified quite a bit.  Does that seem reasonable?
Comment 8 Jer Noble 2012-01-12 16:09:40 PST
(In reply to comment #7)
> (In reply to comment #6)
> > The patch in bug #74345 also adds vsma support.  If both patches are approved, we'll have to make sure not to step on each others' toes.
> 
> Agreed.  I think it's easiest if we can land this patch first.  And then #74345 can be simplified quite a bit.  Does that seem reasonable?

Yup!
Comment 9 Chris Rogers 2012-01-12 16:31:52 PST
Comment on attachment 121630 [details]
Patch

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

> Source/WebCore/platform/audio/VectorMath.cpp:95
> +#if defined(__ppc__) || defined(__i386__)

Sorry for the last minute change, but Jer Noble just mentioned that some of these vector functions don't have a problem with vec_translate.h.

So we should simplify this, by removing #if defined(__ppc__) || defined(__i386__) and always simply and directly call:

vDSP_vsma(sourceP, sourceStride, scale, destP, destStride, destP, destStride, framesToProcess);
Comment 10 Wei James (wistoch) 2012-01-12 16:49:12 PST
Created attachment 122338 [details]
Patch
Comment 11 Wei James (wistoch) 2012-01-12 16:49:39 PST
(In reply to comment #9)
> (From update of attachment 121630 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121630&action=review
> 
> > Source/WebCore/platform/audio/VectorMath.cpp:95
> > +#if defined(__ppc__) || defined(__i386__)
> 
> Sorry for the last minute change, but Jer Noble just mentioned that some of these vector functions don't have a problem with vec_translate.h.
> 
> So we should simplify this, by removing #if defined(__ppc__) || defined(__i386__) and always simply and directly call:
> 
> vDSP_vsma(sourceP, sourceStride, scale, destP, destStride, destP, destStride, framesToProcess);

thanks. patch updated.
Comment 12 Chris Rogers 2012-01-12 16:51:24 PST
Thanks James, looks good.
Comment 13 Kenneth Russell 2012-01-12 19:19:30 PST
Comment on attachment 122338 [details]
Patch

rs=me
Comment 14 WebKit Review Bot 2012-01-12 20:28:08 PST
Comment on attachment 122338 [details]
Patch

Clearing flags on attachment: 122338

Committed r104893: <http://trac.webkit.org/changeset/104893>
Comment 15 WebKit Review Bot 2012-01-12 20:28:14 PST
All reviewed patches have been landed.  Closing bug.