[Ffmpeg-devel] [RFC] aligned av_realloc

[Hervé W.]

Hi,

I'd like some feedback on this patch, especially regarding the case
without MEMALIGN-HACK but with memalign().

The changes:
for the case memalign-hack:
*changed the type of 'diff' from int to long. In this patch diff gets
used in exactly the same way as in av_malloc and per r4911: "long is
better than int for pointer differences"

*ptr is reallocated. If the data isn't aligned, it gets moved in the
allocated space, and diff is updated.

with memalign:
*memalign is called, the original data is copied and the old ptr is
av_free'd. My concerns regarding this method and an alternative
solution are 'documented' in the comments.

without hack or memalign:
*as regular realloc. The memalign hack isn't needed (or wanted), nor
is memalign, therefore malloc is aligned on 16 (or does not need to
be?) and, presumably, the same applies to realloc.

-V
-------------- next part --------------
Index: libavcodec/mem.c
===================================================================
--- libavcodec/mem.c	(revision 5560)
+++ libavcodec/mem.c	(working copy)
@@ -101,8 +101,9 @@
  */
 void *av_realloc(void *ptr, unsigned int size)
 {
+    void *old_ptr;
 #ifdef MEMALIGN_HACK
-    int diff;
+    long  diff;
 #endif
 
     /* let's disallow possible ambiguous cases */
@@ -110,11 +111,59 @@
         return NULL;
 
 #ifdef MEMALIGN_HACK
-    //FIXME this isn't aligned correctly, though it probably isn't needed
+    /* this should now be aligned correctly to 16 */
     if(!ptr) return av_malloc(size);
     diff= ((char*)ptr)[-1];
-    return realloc(ptr - diff, size + diff) + diff;
-#else
+    ptr -= diff;
+
+    if(!realloc(ptr, size+16))
+    {
+        ptr += diff;
+        return NULL;
+    }
+    if( ( ((-(long)ptr - 1)&15) + 1) == diff )
+        return (ptr += diff);
+
+    old_ptr = ptr + diff;   /* old_ptr points to the start of the (not aligned) data */
+    diff= ((-(long)ptr - 1)&15) + 1;
+    ptr += diff;            /* ptr points to where the start of the (aligned) data will be*/
+
+    memmove(ptr, old_ptr, size);
+    ((char*)ptr)[-1]= diff;
+    return ptr;
+#elif defined (HAVE_MEMALIGN)
+    old_ptr = ptr;
+    ptr = memalign(16,size);
+    if(!ptr)
+    {
+        ptr = old_ptr;
+        return NULL;
+    }
+    memcpy(ptr, old_ptr, size);     /*  is this correct (on all architectures)?
+                                        depending on the size, old_ptr might
+                                        go out of allocated space. */
+/*                                  //  alternative:
+//#elif defined (HAVE_MEMALIGN)     //  realloc, so that ptr points to allocated
+    if(!realloc(ptr, size))         //  memory of the desired size, if realloc
+        return NULL;                //  is possible
+    if( !(((long)ptr)&15) )         //  if lucky, it's already aligned
+        return (ptr);
+
+    old_ptr = ptr;                  //  if it isn't aligned
+    ptr = memalign(16,size);        //  allocate a new aligned block of memory
+    if(!ptr)                        //  if possible
+    {
+        ptr = old_ptr;
+        return NULL;
+    }
+    memcpy(ptr, old_ptr, size);     //  and move the memory there
+*/
+
+    av_free(old_ptr);
+    return ptr;
+#else   /*  the memalign hack isn't needed, nor is memalign, therefore malloc is
+            aligned on 16 (or does not need to be?) and, presumably,
+            the same applies to realloc */
     return realloc(ptr, size);
 #endif
 }