Bug 87430 - Use 32-byte alignment in AudioArray if using WEBAUDIO_FFMPEG
Summary: Use 32-byte alignment in AudioArray if using WEBAUDIO_FFMPEG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond Toy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-24 14:33 PDT by Raymond Toy
Modified: 2012-05-24 18:07 PDT (History)
3 users (show)

See Also:

Attachments
Patch (1.39 KB, patch)
2012-05-24 14:34 PDT, Raymond Toy 		no flags Details | Formatted Diff | Diff
Patch (1.24 KB, patch)
2012-05-24 16:11 PDT, Raymond Toy 		no flags Details | Formatted Diff | Diff
Patch (1.28 KB, patch)
2012-05-24 16:24 PDT, Raymond Toy 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond Toy 2012-05-24 14:33:20 PDT 
Use 32-byte alignment in AudioArray
Comment 1 Raymond Toy 2012-05-24 14:34:43 PDT 
Created attachment 143898 [details]
Patch
Comment 2 Raymond Toy 2012-05-24 14:37:33 PDT 
See also http://code.google.com/p/chromium/issues/detail?id=119374
Comment 3 Chris Rogers 2012-05-24 15:43:42 PDT 
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
Comment 4 Raymond Toy 2012-05-24 16:11:43 PDT 
Created attachment 143915 [details]
Patch
Comment 5 Raymond Toy 2012-05-24 16:14:16 PDT 
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.
Comment 6 Chris Rogers 2012-05-24 16:16:12 PDT 
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.
Comment 7 Raymond Toy 2012-05-24 16:23:38 PDT 
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.
Comment 8 Raymond Toy 2012-05-24 16:24:20 PDT 
Created attachment 143917 [details]
Patch
Comment 9 Chris Rogers 2012-05-24 16:44:03 PDT 
Comment on attachment 143917 [details]
Patch

Thanks Ray
Comment 10 WebKit Review Bot 2012-05-24 18:07:29 PDT 
Comment on attachment 143917 [details]
Patch

Clearing flags on attachment: 143917

Committed r118455: <http://trac.webkit.org/changeset/118455>
Comment 11 WebKit Review Bot 2012-05-24 18:07:34 PDT 
All reviewed patches have been landed.  Closing bug.