Make sure that AudioArray is 16-byte aligned
Created attachment 102846 [details] Patch
this fixes: https://bugs.webkit.org/show_bug.cgi?id=63998 Audio arrays should be 16-byte aligned for maximum performance. In FFTFrameFFMPEG's case, the av_rdft_calc() function actually can crash if not given an aligned address, although this is not documented!
Comment on attachment 102846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102846&action=review Generally looks good but I strongly feel you should be using FastMalloc.h. > Source/WebCore/platform/audio/AudioArray.h:48 > + free(m_allocation); You should use fastMalloc and fastFree (in <wtf/FastMalloc.h>), as Vector.h does, to have the possibility of using WebKit's malloc validation, and to crash upon allocation failure on all platforms. > Source/WebCore/platform/audio/AudioArray.h:59 > + free(m_allocation); Here too. > Source/WebCore/platform/audio/AudioArray.h:68 > + T* allocation = static_cast<T*>(malloc(sizeof(T) * n + extraAllocationBytes)); Here too. > Source/WebCore/platform/audio/AudioArray.h:79 > + free(allocation); Here too.
Created attachment 102954 [details] Patch
Comment on attachment 102846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102846&action=review >> Source/WebCore/platform/audio/AudioArray.h:48 >> + free(m_allocation); > > You should use fastMalloc and fastFree (in <wtf/FastMalloc.h>), as Vector.h does, to have the possibility of using WebKit's malloc validation, and to crash upon allocation failure on all platforms. FIXED >> Source/WebCore/platform/audio/AudioArray.h:59 >> + free(m_allocation); > > Here too. FIXED >> Source/WebCore/platform/audio/AudioArray.h:68 >> + T* allocation = static_cast<T*>(malloc(sizeof(T) * n + extraAllocationBytes)); > > Here too. FIXED >> Source/WebCore/platform/audio/AudioArray.h:79 >> + free(allocation); > > Here too. FIXED
Comment on attachment 102954 [details] Patch Looks good.
Comment on attachment 102954 [details] Patch Clearing flags on attachment: 102954 Committed r92408: <http://trac.webkit.org/changeset/92408>
All reviewed patches have been landed. Closing bug.
Sorry I'm late to the party. I have some questions and remarks about the patch. I assume you added the loop in an attempt to prevent over-allocating to align the data in case it aligns correctly by chance. 1) Since allocations are already 4 bytes aligned, you will need to add 4, 8 or 12 bytes to correct the alignment if it's not aligned correctly. Why are you adding "alignment" (16 bytes) to the allocation, rather than "alignment - 4"? Your code over-allocates 12, 8 or 4 bytes for an average of 6 bytes. If you use "alignment - 4", this will drop to 3 bytes. 2) Why use a loop when the code is only going to be executed twice? It seems the code would be much cleaner if you unrolled the loop. Also, why use a local variable to terminate it, rather than break? m_allocation = static_cast<T*>(fastMalloc(sizeof(T) * n)); m_alignedData = alignedAddress(m_allocation, alignment); if (m_alignedData != m_allocation) { fastFree(m_allocation); m_allocation = static_cast<T*>(fastMalloc(sizeof(T) * n + alignment - 4)); m_alignedData = alignedAddress(m_allocation, alignment); } m_size = n; zero(); 3) If you immediately over-allocate "alignment - 4", there is no need for a "loop" or "if" at all, simplifying the code even further. In this case you will over allocate 12, 8, 4 or 0 bytes for an average of 6, which is equivalent to the current code. m_allocation = static_cast<T*>(fastMalloc(sizeof(T) * n + alignment - 4)); m_alignedData = alignedAddress(m_allocation, alignment); m_size = n; zero();