Auto sized ringbuffers, changes since v1:
- increased maximum sizes for a few rb users. Most of the time just a small part will be used, but there will be more room for times when more is required. - a little smarter cRingBufferLinear::Read(), handles the TS buffer better. - faster buffer growth. Also makes it less likely that there won't be an opportunity to resize before an overflow. - a potentially sleeping cRecorder:Receive(). Mostly to find out how much more buffer/time would have been needed, ie debugging. - more verbose logging.
It still wouldn't surprise me if this version caused a few overflows, but hopefully these will be very rare.
artur
diff --git a/dvbdevice.c b/dvbdevice.c index 955483e..20dcf29 100644 --- a/dvbdevice.c +++ b/dvbdevice.c @@ -1184,7 +1184,7 @@ bool cDvbDevice::OpenDvr(void) CloseDvr(); fd_dvr = DvbOpen(DEV_DVB_DVR, CardIndex(), O_RDONLY | O_NONBLOCK, true); if (fd_dvr >= 0) - tsBuffer = new cTSBuffer(fd_dvr, MEGABYTE(2), CardIndex() + 1); + tsBuffer = new cTSBuffer(fd_dvr, MEGABYTE(8), CardIndex() + 1); return fd_dvr >= 0; }
diff --git a/recorder.c b/recorder.c index 8bb1621..3c0e002 100644 --- a/recorder.c +++ b/recorder.c @@ -157,8 +157,20 @@ void cRecorder::Receive(uchar *Data, int Length) { if (Running()) { int p = ringBuffer->Put(Data, Length); - if (p != Length && Running()) + if (p != Length && Running()) { + for (int ms=20; ms<1000; ms+=ms) { + cCondWait::SleepMs(ms); + if (!Running()) + return; + int r = ringBuffer->Put(Data+p, Length-p); + p += r; + if (r) + dsyslog("saved extra %d bytes in recorder ring buffer after %d ms delay", r, ms); + if (p == Length || !Running()) + return; + } ringBuffer->ReportOverflow(Length - p); + } } }
diff --git a/remux.c b/remux.c index da805b7..bd29b2f 100644 --- a/remux.c +++ b/remux.c @@ -1851,7 +1851,7 @@ void cTS2PES::ts_to_pes(const uint8_t *Buf) // don't need count (=188)
// --- cRemux ----------------------------------------------------------------
-#define RESULTBUFFERSIZE KILOBYTE(256) +#define RESULTBUFFERSIZE MEGABYTE(2)
cRemux::cRemux(int VPid, const int *APids, const int *DPids, const int *SPids, bool ExitOnFailure) { diff --git a/ringbuffer.c b/ringbuffer.c index 0633bd3..39cc02e 100644 --- a/ringbuffer.c +++ b/ringbuffer.c @@ -151,13 +151,30 @@ void cRingBufferLinear::PrintDebugRBL(void) } #endif
+// cRingBufferLinear are dynamically sized, or at least we can pretend they are ;) +// We treat 'Size' as the maximum size, but start with a small buffer, which can +// grow later as it fills up. Memory is always allocated for the full buffer, but +// the unused RAM portion remains untouched until (if at all) it is actually needed. +// Note that we can not start with a larger than requested buffer because there are +// ring buffer users that cause crashes then (eg softdevice mpegdecoder absolutely +// needs 32/64k). + +// Startup size. 64k still causes overflows before buffer starts to grow, 128k doesn't. +// The buffers grow to 200..500k anyway, so maybe increasing this a bit more would +// make sense, but let's first see if it's really needed. +// In fact 128k is on the low side, but let's try not going for 256k yet. +#define DEFRBSIZE KILOBYTE(128) + cRingBufferLinear::cRingBufferLinear(int Size, int Margin, bool Statistics, const char *Description) -:cRingBuffer(Size, Statistics) +:cRingBuffer(min(Size,DEFRBSIZE), Statistics) { description = Description ? strdup(Description) : NULL; tail = head = margin = Margin; gotten = 0; buffer = NULL; + maxsize = Size; + growbuf = 0; + dsyslog("New ring buffer "%s" size: %d margin: %d", description, Size, Margin ); if (Size > 1) { // 'Size - 1' must not be 0! if (Margin <= Size / 2) { buffer = MALLOC(uchar, Size); @@ -183,6 +200,8 @@ cRingBufferLinear::~cRingBufferLinear() #ifdef DEBUGRINGBUFFERS DelDebugRBL(this); #endif + dsyslog("Deleting ring buffer "%s" size: %d / %d (used %g%% of requested size)", description, + size, maxsize, size/(double)maxsize*100.0 ); free(buffer); free(description); } @@ -205,8 +224,21 @@ void cRingBufferLinear::Clear(void) EnablePut(); }
+// Must only be called by the producer (ie Read()/Put()), not the consumer (Get()). +void cRingBufferLinear::GrowBuffer(void) +{ + size = min(size+size, maxsize); + dsyslog("Enlarging ring buffer "%s": %d bytes (trigger %d)", + description, size, growbuf); + growbuf = 0; +} + int cRingBufferLinear::Read(int FileHandle, int Max) { + if (Available() >= size/2) + growbuf = 1; + if (growbuf && head>=tail && size<maxsize) + GrowBuffer(); int Tail = tail; int diff = Tail - head; int free = (diff > 0) ? diff - 1 : Size() - head; @@ -219,6 +251,10 @@ int cRingBufferLinear::Read(int FileHandle, int Max) Count = safe_read(FileHandle, buffer + head, free); if (Count > 0) { int Head = head + Count; + if (Available()+Count >= size/2) + growbuf = 2; + if (growbuf && head>=tail && size<maxsize) + GrowBuffer(); if (Head >= Size()) Head = margin; head = Head; @@ -245,6 +281,10 @@ int cRingBufferLinear::Read(int FileHandle, int Max) int cRingBufferLinear::Put(const uchar *Data, int Count) { if (Count > 0) { + if (Available()+Count >= size/2) + growbuf = 3; + if (growbuf && head>=tail && size<maxsize) + GrowBuffer(); int Tail = tail; int rest = Size() - head; int diff = Tail - head; diff --git a/ringbuffer.h b/ringbuffer.h index dba0e61..9e7268c 100644 --- a/ringbuffer.h +++ b/ringbuffer.h @@ -18,11 +18,11 @@ private: cCondWait readyForPut, readyForGet; int putTimeout; int getTimeout; - int size; time_t lastOverflowReport; int overflowCount; int overflowBytes; protected: + int size; tThreadId getThreadTid; int maxFill;//XXX int lastPercent; @@ -59,7 +59,10 @@ private: int margin, head, tail; int gotten; uchar *buffer; + int growbuf; + int maxsize; char *description; + void GrowBuffer(void); public: cRingBufferLinear(int Size, int Margin = 0, bool Statistics = false, const char *Description = NULL); ///< Creates a linear ring buffer.
On 5/10/07, Artur Skawina art_k@o2.pl wrote:
Auto sized ringbuffers, changes since v1:
- increased maximum sizes for a few rb users. Most of the time just a
small part will be used, but there will be more room for times when more is required.
- a little smarter cRingBufferLinear::Read(), handles the TS buffer
better.
- faster buffer growth. Also makes it less likely that there won't be
an opportunity to resize before an overflow.
- a potentially sleeping cRecorder:Receive(). Mostly to find out how
much more buffer/time would have been needed, ie debugging.
- more verbose logging.
It still wouldn't surprise me if this version caused a few overflows, but hopefully these will be very rare.
I'm curious how streamdev will function with these buffer changes.
Best Regards.
Stone writes:
It still wouldn't surprise me if this version caused a few overflows, but hopefully these will be very rare.
I'm curious how streamdev will function with these buffer changes.
And since I am not convinced that this memory footprint issue is significant, I am concerned if this patch is accepted before all the overflows and problems with live viewing have been solved first.
yours, Jouni
And since I am not convinced that this memory footprint issue is significant,
at a first glance, IMHO dynamic buffers are a good thing. we can get rid of small upper buffer size bounderies all together without wasting amounts of memory. this should result in even less buffer overflows when implemented correctly. it seems doable to me.
clemens
Jouni Karvo wrote:
Stone writes:
It still wouldn't surprise me if this version caused a few overflows, but hopefully these will be very rare.
I'm curious how streamdev will function with these buffer changes.
And since I am not convinced that this memory footprint issue is significant, I am concerned if this patch is accepted before all the overflows and problems with live viewing have been solved first.
It shouldn't. The whole point of posting it is so that other are able to test and review it. That is why I always mention the overflows. Right now it's stable enough for production for me, haven't seen any overflow since adjusting the growth rate, but it would be nice if somebody with a different vdr setup tested it too (eg a FF card based vdr). It will take weeks to verify that the code works as expected (the goal is to have _fewer_ overflows (compared to the static buffers) while at the same time a much smaller working set).
artur
Stone wrote:
It still wouldn't surprise me if this version caused a few overflows, but hopefully these will be very rare.
I'm curious how streamdev will function with these buffer changes.
it works fine -- i'm using a headless vdr server and streamdev+softdevice clients, so this actually gets tested fairly well.
Example buffer usage from server:
Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 524288 / 4194304 (used 12.5% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 524288 / 4194304 (used 12.5% of requested size) Deleting ring buffer "streamdev-streamer" size: 524288 / 4194304 (used 12.5% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 262144 / 4194304 (used 6.25% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size) Deleting ring buffer "streamdev-streamer" size: 131072 / 4194304 (used 3.125% of requested size)
and the streamdev client TS buffer usually grows to 12.5..25% (ie 256K/512K out of 4M max).
artur
On 05/10/07 01:55, Artur Skawina wrote:
Auto sized ringbuffers, changes since v1: ... diff --git a/recorder.c b/recorder.c index 8bb1621..3c0e002 100644 --- a/recorder.c +++ b/recorder.c @@ -157,8 +157,20 @@ void cRecorder::Receive(uchar *Data, int Length) { if (Running()) { int p = ringBuffer->Put(Data, Length);
if (p != Length && Running())
if (p != Length && Running()) {
for (int ms=20; ms<1000; ms+=ms) {
cCondWait::SleepMs(ms);
if (!Running())
return;
int r = ringBuffer->Put(Data+p, Length-p);
p += r;
if (r)
dsyslog("saved extra %d bytes in recorder ring buffer after %d ms delay", r, ms);
if (p == Length || !Running())
return;
} ringBuffer->ReportOverflow(Length - p);
} }
}
If this "auto sized ringbuffers" change (which, from what I can see so far - haven't tried it myself - looks like a good idea) is ever to make its way into the official VDR source, you'll need to get rid of the above waiting. It says in receiver.h:
...the call must return as soon as possible, without any unnecessary delay.
Maybe I should change this to
...the call must return immediately.
Klaus
Klaus Schmidinger wrote:
If this "auto sized ringbuffers" change (which, from what I can see so far - haven't tried it myself - looks like a good idea) is ever to make its way into the official VDR source, you'll need to get rid of the above waiting. It says in receiver.h:
...the call must return as soon as possible, without any unnecessary delay.
Maybe I should change this to
...the call must return immediately.
or "must not sleep".
I did mention in the patch description, which you snipped, that this was just a debugging tool. IOW that change can be dropped entirely, the condition that makes it sleep has never triggered since I made the buffers grow faster anyway.
I'd be great if somebody with a FF card could test the patch, the code has worked flawlessly on my vdr server, not a single overflow since v2. However on the streamdev/softdevice client I had two transfer buffer overflows (153 bytes each) just after switching to a new channel, while both machines were under significant (io) load, cutting aot. I don't think this is reason enough to introduce a minsize rb parameter, but if it also happens w/ a simple FF setup, that would be an option. I don't want to raise the default minimum, because 128k is often enough and i'd like to avoid shrinking, to keep things simple. The patch is for 1.4.6, but applies also, with a few harmless offsets, to 1.5.2.
artur
Artur Skawina art_k@o2.pl wrote:
I'd be great if somebody with a FF card could test the patch, the code has worked flawlessly on my vdr server, not a single overflow since v2.
i currently test your patch on vdr-1.5.2 with one FF card. i have some timers pending this afternoon, so stay tuned. ;-)
clemens