Bug 87430

Summary: Use 32-byte alignment in AudioArray if using WEBAUDIO_FFMPEG
Product: WebKit Reporter: Raymond Toy <rtoy>
Component: New BugsAssignee: Raymond Toy <rtoy>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, feature-media-reviews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Raymond Toy
Reported 2012-05-24 14:33:20 PDT
Use 32-byte alignment in AudioArray
Attachments
Patch (1.39 KB, patch)
2012-05-24 14:34 PDT, Raymond Toy 		no flags
DetailsFormatted DiffDiff
Patch (1.24 KB, patch)
2012-05-24 16:11 PDT, Raymond Toy 		no flags
DetailsFormatted DiffDiff
Patch (1.28 KB, patch)
2012-05-24 16:24 PDT, Raymond Toy 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Raymond Toy
Comment 1 2012-05-24 14:34:43 PDT
Created attachment 143898 [details] Patch
Raymond Toy
Comment 2 2012-05-24 14:37:33 PDT
See also http://code.google.com/p/chromium/issues/detail?id=119374
Chris Rogers
Comment 3 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
Raymond Toy
Comment 4 2012-05-24 16:11:43 PDT
Created attachment 143915 [details] Patch
Raymond Toy
Comment 5 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.
Chris Rogers
Comment 6 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.
Raymond Toy
Comment 7 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.
Raymond Toy
Comment 8 2012-05-24 16:24:20 PDT
Created attachment 143917 [details] Patch
Chris Rogers
Comment 9 2012-05-24 16:44:03 PDT
Comment on attachment 143917 [details] Patch Thanks Ray
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2012-05-24 18:07:34 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.