michaeltyson / TPCircularBuffer

A simple, fast circular buffer implementation
http://atastypixel.com/blog/a-simple-fast-circular-buffer-implementation-for-audio-processing/
839 stars 141 forks source link

Buffer does not use any circular features when calling TPCircularBufferProduceBytes #6

Closed fraggle222 closed 6 years ago

fraggle222 commented 9 years ago

Calls to TPCircularBufferProduceBytes will just return when the buffer does not have enough space left. So basically the buffer fills up and then when you call TPCircularBufferProduceBytes, the method just returns with no changes and the buffer stays exactly as it was. Offending line is: if ( space < len ) return false;

Instead it should be a circular buffer and if it is out of space, overwrite the older data.

My colleague found this issue a couple of years ago and I ran into it again when getting the code from source instead of using his fixed version (new method added). I will see if he wants to open a pull request with the changes. But basically if no space is left, call a Consume operation to free up that space, get a fresh pointer to the head, then write the data.

michaeltyson commented 9 years ago

Hello! Actually, this is not typical circular buffer functionality - doing this would violate thread safety, among other things. If you want to drain the buffer when out of space, you'll need to manage that yourself with the appropriate thread safety measures.

Sent from my iPhone

On 17 May 2015, at 12:54 pm, Paul G. notifications@github.com wrote:

Calls to TPCircularBufferProduceBytes will just return when the buffer does not have enough space left. So basically the buffer fills up and then when you call TPCircularBufferProduceBytes, the method just returns with no changes and the buffer stays exactly as it was. Offending line is: if ( space < len ) return false;

Instead it should be a circular buffer and if it is out of space, overwrite the older data.

My colleague found this issue a couple of years ago and I ran into it again when getting the code from source instead of using his fixed version (new method added). I will see if he wants to open a pull request with the changes. But basically if no space is left, call a Consume operation to free up that space, get a fresh pointer to the head, then write the data.

— Reply to this email directly or view it on GitHub.

fraggle222 commented 9 years ago

Hmm, I thought the whole idea was you could keep writing to it: http://en.wikipedia.org/wiki/Circular_buffer "A consequence of the circular buffer is that when it is full and a subsequent write is performed, then it starts overwriting the oldest data."

michaeltyson commented 9 years ago

That's one possible implementation, but it's not universal, and it's a limiting case. TPCB can be used that way, if you dequeue when full, but it will interfere with a separate dequeue thread. Care must be taken; the same is true of any circular buffer implementation.

On 17 May 2015, at 8:44 pm, Paul G. notifications@github.com wrote:

Hmm, I thought the whole idea was you could keep writing to it: http://en.wikipedia.org/wiki/Circular_buffer "A consequence of the circular buffer is that when it is full and a subsequent write is performed, then it starts overwriting the oldest data."

— Reply to this email directly or view it on GitHub.

fraggle222 commented 9 years ago

So what do people do when the buffer is full, erase some portion of it right? So why not a connivence method that automatically wraps around. We have a way to do this and my suggestion is to put it into a method that is part of your code. I will post the code here after I check with my colleague who wrote it that that's ok.

So if we have a buffer that can hold 4 bytes, and we write 4 bytes to it, it looks like this: byte1, byte2, byte3, byte4

Then we write 1 more byte to it and it looks like this: byte5, byte2, byte3, byte4

The way your code currently works, is that writing byte5 leaves the buffer like this: byte1, byte2, byte3, byte4

michaeltyson commented 9 years ago

If you want a system like that, I'd probably recommend just using a simple buffer with a head pointer, rather than using TPCB - it may be easier...

Sent from my iPhone

On 18 May 2015, at 4:15 pm, Paul G. notifications@github.com wrote:

So what do people do when the buffer is full, erase some portion of it right? So why not a connivence method that automatically wraps around. We have a way to do this and my suggestion is to put it into a method that is part of your code. I will post the code here after I check with my colleague who wrote it that that's ok.

So if we have a buffer that can hold 4 bytes, and we write 4 bytes to it, it looks like this: byte1, byte2, byte3, byte4

Then we write 1 more byte to it and it looks like this: byte5, byte2, byte3, byte4

The way the code currently works, is that writing byte5 leaves the buffer like this: byte1, byte2, byte3, byte4

— Reply to this email directly or view it on GitHub.

fraggle222 commented 9 years ago

Maybe we are misunderstanding one another. I was looking at what you wrote here: http://atastypixel.com/blog/a-simple-fast-circular-buffer-implementation-for-audio-processing/ and it seemed like you had 'auto-wrapping' in mind.

But perhaps in the example on that page, you are outputing audio as soon as you receive it, so then the buffer fill up issue never comes into play and you are just consuming right after you produce.

On the other hand what we are doing is continuously capturing the last 60 seconds of audio in your buffer and letting it wrap around to overwrite the previous 60 seconds as needed. Then at some later time we output whatever is in the buffer, which is the last 60 seconds of audio.

Anyway here is the method my colleague came up with. It works (at least in single threaded tests).

static __inline__ __attribute__((always_inline)) bool TPCircularBufferProduceBytesSafe(TPCircularBuffer *buffer, const void* src, int32_t len) {
    int32_t space;
    void *ptr = TPCircularBufferHead(buffer, &space);
    if ( space < len || ptr == NULL) {
        int32_t gap = len - space;

        TPCircularBufferConsume(buffer, gap);

        ptr = TPCircularBufferHead(buffer, &space);
    }
    memcpy(ptr, src, len);
    TPCircularBufferProduce(buffer, len);
    return true;
}

Let me know what you think. Also, I hope my tone isn't too combative here, your code is great and a key part of what we are doing at my company.

michaeltyson commented 9 years ago

This is indeed different to a buffer that automatically drains - the wrap around described here refers to the underlying segment of memory.

Your colleague’s method will indeed achieve the functionality you’re after, though - just be aware that with this, the utility’s no longer thread-safe.

On 19 May 2015, at 1:18 am, Paul G. notifications@github.com wrote:

Maybe we are misunderstanding one another. You wrote this right: "Circular buffers are pretty much what they sound like – arrays that wrap around. They’re fantastically useful as scratch space for audio processing, and generally passing audio around efficiently." and "When you produce data at the head, the head moves up the array, and wraps around at the end",

From http://atastypixel.com/blog/a-simple-fast-circular-buffer-implementation-for-audio-processing/ http://atastypixel.com/blog/a-simple-fast-circular-buffer-implementation-for-audio-processing/ But perhaps in the example on that page, you are outputing audio as soon as you receive it, so then the buffer fill up issue never comes into play and you are just consuming right after you produce.

On the other hand what we are doing is continuously capturing the last 60 seconds of audio in your buffer and letting it wrap around to overwrite the previous 60 seconds as needed. Then at some later time we output whatever is in the buffer, which is the last 60 seconds of audio.

Anyway here is the method my colleague came up with. It works (at least in single threaded tests).

static inline attribute((always_inline)) bool TPCircularBufferProduceBytesSafe(TPCircularBuffer buffer, const void src, int32_t len) { int32_t space; void *ptr = TPCircularBufferHead(buffer, &space); if ( space < len || ptr == NULL) { int32_t gap = len - space;

    TPCircularBufferConsume(buffer, gap);

    ptr = TPCircularBufferHead(buffer, &space);
}
memcpy(ptr, src, len);
TPCircularBufferProduce(buffer, len);
return true;

} — Reply to this email directly or view it on GitHub https://github.com/michaeltyson/TPCircularBuffer/issues/6#issuecomment-103279428.