Codec Engine / H.264 Encoder memory leak

Andreas,

The Memory_maxCbListSize check is there to keep performance of the Memory_getBufferPhysicalAddress() and Memory_getBufferVirtualAddress() at a reasonable level.  Before it was added, the list would grow unbounded due to automatic insertion during Memory_getBufferPhysicalAddress(), and search operations would take more and more time as the list grew, eventually eating up so many CPU cycles that the application started missing realtime.

This would not be a problem if the list was restricted to Memory-managed buffers - buffers that are allocated and freed by Memory_contigAlloc()/Memory_contigFree().  The problem creeps in because Codec Engine was doing a "favor" for users that passed non-Memory buffers to codec algorithms, which is a perfectly valid use-case.  To explain, a user app would pass a buffer allocated outside of Codec Engine (CE) to a VISA-class "process()" or "control()" function (e.g., VIDENC1_process()).  The VISA-class stub needs to pass the physical address to the codec algorithm, so it uses Memory_getBufferPhysicalAddress() since the CE function Memory_getBufferPhysicalAddress() is designed to work for non-Memory buffers.  On the way "back" from the process() function, the VISA-class stub needs to translate a physical address from the codec algorithm into a virtual address that can be used by the Linux app, so the Memory module's translation cache is used for this purpose.  Since the addition to the translation cache was automatic in this case, and since there is no corresponding automatic removal, the list would grow and grow with entries for these non-Memory buffers, and the list would retain translations for buffers that were no longer in use.

There is a bug that we filed that is related to this: SDOCM00076172 - "CE's OSAL API Memory_getBufferPhysicalAddress() should not add buffer to its physical/virtual address translation cache"

Our "fix" for this is to not automatically add a translation entry to the cache during Memory_getBufferPhysicalAddress() and also require the user to register/unregister the buffer using the existing functions Memory_registerContigBuf()/Memory_unregisterContigBuf() before calling the VISA-class process() function.  Memory_contigAlloc()/Memory_contigFree() will continue to add/remove translation entries, and Memory_getBufferPhysicalAddress() will still work for non-Memory, non-CMEM buffers but without adding them to the cache.  Basically, we need the application to tell CE when the buffer is active and when it's done, which is just basic good programming practice (CE was allowing the application to be lazy about buffer management).

With this new behaviour the list should stay reasonably small and not grow unbounded.

This "fix" also contains a big fat error "hint" when Memory_getBufferVirtualAddress() can't find an entry:

    if (virtualAddress == 0) {
        Log_print2(Diags_USER7, "[+7] Memory_getVirtualAddress> "
            "ERROR: buffer (physAddr=0x%x, size=0x%x) not found in translation"
            "cache\n\nEnsure that you have registered this buffer with "
            "Memory_registerContigBuf()\n", (IArg)physicalAddress, (IArg)sizeInBytes);
    }

We will probably be addressing this in a better fashion some time soon (maybe 2011 Q1), perhaps by using a hash table instead of a list, so we would appreciate any suggestions that you have regarding a better solution.

Regards,

- Rob