Bug 87430 – Use 32-byte alignment in AudioArray if using WEBAUDIO_FFMPEG

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Raymond Toy]

Use 32-byte alignment in AudioArray

[Raymond Toy]

Created attachment 143898 [details]
Patch

[Raymond Toy]

See also http://code.google.com/p/chromium/issues/detail?id=119374

[Chris Rogers]

Comment on attachment 143898 [details]
Patch

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

> Source/WebCore/platform/audio/AudioArray.h:63
> +        // 32-byte alignment for 256-bit SIMD. But Darwin (without FFMPEG) only needs 16-byte

Sorry Ray, I think it's best to simply remove this comment
WebKit style guidelines are to avoid obvious comments which reading the code can better answer

> Source/WebCore/platform/audio/AudioArray.h:65
> +#if OS(DARWIN) && !USE(WEBAUDIO_FFMPEG)

Please remove the OS(DARWIN) part.  Also I would reverse the #if and have 32 appear first (checking USE(WEBAUDIO_FFMPEG)

I think it reads better that way

[Raymond Toy]

Created attachment 143915 [details]
Patch

[Raymond Toy]

Comment on attachment 143898 [details]
Patch

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

>> Source/WebCore/platform/audio/AudioArray.h:63
>> +        // 32-byte alignment for 256-bit SIMD. But Darwin (without FFMPEG) only needs 16-byte
> 
> Sorry Ray, I think it's best to simply remove this comment
> WebKit style guidelines are to avoid obvious comments which reading the code can better answer

Comments removed.  Reading the code now just says ffmpeg uses 32 bytes instead of 16.  For some unknown reason.

>> Source/WebCore/platform/audio/AudioArray.h:65
>> +#if OS(DARWIN) && !USE(WEBAUDIO_FFMPEG)
> 
> Please remove the OS(DARWIN) part.  Also I would reverse the #if and have 32 appear first (checking USE(WEBAUDIO_FFMPEG)
> 
> I think it reads better that way

Yes, that's better.  Done.

[Chris Rogers]

Comment on attachment 143915 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Use 32-byte alignment in AudioArray

... if using WEBAUDIO_FFMPEG

> Source/WebCore/ChangeLog:8
> +        No new tests.

No new tests -> Covered by existing tests.

[Raymond Toy]

Comment on attachment 143915 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Use 32-byte alignment in AudioArray
> 
> ... if using WEBAUDIO_FFMPEG

Fixed.  Will fix bug title too.

>> Source/WebCore/ChangeLog:8
>> +        No new tests.
> 
> No new tests -> Covered by existing tests.

Fixed.

[Raymond Toy]

Created attachment 143917 [details]
Patch

[Chris Rogers]

Comment on attachment 143917 [details]
Patch

Thanks Ray

[WebKit Review Bot]

Comment on attachment 143917 [details]
Patch

Clearing flags on attachment: 143917

Committed r118455: <http://trac.webkit.org/changeset/118455>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.