Bug 75835 – Add vsma in VectorMath to handle vector scale multiply and add and use it in AudioBus

• Component: Web Audio
• Status: RESOLVED FIXED

[Wei James (wistoch)]

Add vsma in VectorMath to handle vector scale multiply and add and use it in AudioBus

[Wei James (wistoch)]

Created attachment 121630 [details]
Patch

[Raymond Toy]

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?

[Wei James (wistoch)]

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.

[Raymond Toy]

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.

[Chris Rogers]

Looks good.

[Jer Noble]

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.

[Chris Rogers]

(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?

[Jer Noble]

(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!

[Chris Rogers]

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);

[Wei James (wistoch)]

Created attachment 122338 [details]
Patch

[Wei James (wistoch)]

(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.

[Chris Rogers]

Thanks James, looks good.

[Kenneth Russell]

Comment on attachment 122338 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 122338 [details]
Patch

Clearing flags on attachment: 122338

Committed r104893: <http://trac.webkit.org/changeset/104893>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.