Hi,
I'm currently working on a tool that checks for timer conflicts and became aware of the following problem:
Suppose you have the following 3 timers on a system with two DVB cards:
nr. transponder - start - priority 1. A - 20:10 - 45 2. B - 20:10 - 5 3. C - 20:11 - 50
A,B,C are different transponders. VDR will start recording from transponder A at 20:10 with device 2. The same for transponder B with device 1. At 20:11 we have a conflict and IMHO VDR should stop timer 2 and start timer 3 on device 1. But when I check the logs the following occures: VDR first stops timer 1 on device 2 and starts timer 3 on device 2. Then it recognizes the pending timer 1 with higher priority than the running timer 2 and stops this one. So in the end all is fine, but there is a small break about 1-2 seconds in recording 1. The problem is, that cDevice::GetDevice handles the two devices in this case with the same priority and therefore returns the device with the highest number. I think one could fix this with a small change (patch against 1.4.0 is attached), but the whole thing is really complicated and I do not have a real deep insight in the side effects ;-) , so perhaps you could give a comment on this.
BR,
Christian
Am Samstag, 20. Mai 2006 08:40 schrieb Christian Wieninger:
The problem is, that cDevice::GetDevice handles the two devices in this case with the same priority and therefore returns the device with the highest number. I think one could fix this with a small change (patch against 1.4.0 is attached), but the whole thing is really complicated and I do not have a real deep insight in the side effects ;-) , so perhaps you could give a comment on this.
no comment on this? ;-)
Christian
Christian Wieninger schrieb:
Hi,
I'm currently working on a tool that checks for timer conflicts and became aware of the following problem:
Suppose you have the following 3 timers on a system with two DVB cards:
nr. transponder - start - priority
- A - 20:10 - 45
- B - 20:10 - 5
- C - 20:11 - 50
A,B,C are different transponders. VDR will start recording from transponder A at 20:10 with device 2. The same for transponder B with device 1. At 20:11 we have a conflict and IMHO VDR should stop timer 2 and start timer 3 on device 1. But when I check the logs the following occures: VDR first stops timer 1 on device 2 and starts timer 3 on device 2. Then it recognizes the pending timer 1 with higher priority than the running timer 2 and stops this one. So in the end all is fine, but there is a small break about 1-2 seconds in recording 1. The problem is, that cDevice::GetDevice handles the two devices in this case with the same priority and therefore returns the device with the highest number. I think one could fix this with a small change (patch against 1.4.0 is attached), but the whole thing is really complicated and I do not have a real deep insight in the side effects ;-) , so perhaps you could give a comment on this.
Was this the case also with older VDR-versions? I always thought that VDR behaves the way you also expected and avoids interrupting a recording ...
I don't have a in depth look into this code either, but shouldn't cDevice::GetDevice already only prefer device 1 (the one recording timer 2). As it loops through the devices it should give device 1 the priority 5 and as it comes to device 2 it should evaluate it to priority 7, as Priority of the device 2 is not smaller than the Priority of device 1 (which is one of the conditions for priority 5). Is that right, or am I missing something here? But why does VDR stop both timers?
Bye, Andreas Brugger
Am Montag, 22. Mai 2006 20:58 schrieb Andreas Brugger:
Was this the case also with older VDR-versions? I always thought that VDR behaves the way you also expected and avoids interrupting a recording ...
Atleast with 1.3.44 and above the problem exists.
I don't have a in depth look into this code either, but shouldn't cDevice::GetDevice already only prefer device 1 (the one recording timer 2). As it loops through the devices it should give device 1 the priority 5 and as it comes to device 2 it should evaluate it to priority 7, as Priority of the device 2 is not smaller than the Priority of device 1 (which is one of the conditions for priority 5).
With 'Priority' you probably mean the variable 'pri' used in GetDevice? In the case described before, GetDevice (1.4.0) sets 'pri' to 8 for both devices: The first loop for device 1 has no previously selected device ('d') and so 'pri' is set to 8, since device 1 is receiving. In the second loop device 2 has priority bigger than device 1 and so again 'pri' is set to 8. So device 2 will be returned resulting in a short break of recording 1 with priority 45.
The idea of my patch is to stretch the pri-value to MAXPRIORITY+1 and to add the current priority of the receiving device in this case. Perhaps it would even be a better approach to add the device priority in any case if a device is currently receiving.
Christian
Christian Wieninger schrieb:
I don't have a in depth look into this code either, but shouldn't cDevice::GetDevice already only prefer device 1 (the one recording timer 2). As it loops through the devices it should give device 1 the priority 5 and as it comes to device 2 it should evaluate it to priority 7, as Priority of the device 2 is not smaller than the Priority of device 1 (which is one of the conditions for priority 5).
With 'Priority' you probably mean the variable 'pri' used in GetDevice?
Yes thats right.
In the case described before, GetDevice (1.4.0) sets 'pri' to 8 for both devices: The first loop for device 1 has no previously selected device ('d') and so 'pri' is set to 8, since device 1 is receiving. In the second loop device 2 has priority bigger than device 1 and so again 'pri' is set to 8. So device 2 will be returned resulting in a short break of recording 1 with priority 45.
The idea of my patch is to stretch the pri-value to MAXPRIORITY+1 and to add the current priority of the receiving device in this case. Perhaps it would even be a better approach to add the device priority in any case if a device is currently receiving.
Oh, that was a stupid misinterpretation from me. You are right of course. I think there is the general problem of the cDevice::GetDevice-Routine that in the first run there is no selected device. This should effect all the conditions where there is a check for d != NULL, so maybe the easiest fix is to set the Device in the beginning of the routine to one of the devices (first or last):
- cDevice *d = NULL;
- cDevice *d = device[0]; // or device[numDevices]
So the two timers should evaluate to different priorities.
Any thoughts on that? Do I miss something again?
Am Dienstag, 23. Mai 2006 18:30 schrieb Andreas Brugger:
of the devices (first or last):
- cDevice *d = NULL;
- cDevice *d = device[0]; // or device[numDevices]
So the two timers should evaluate to different priorities.
Any thoughts on that? Do I miss something again?
I think so ;-) because if d is set to first device then
else if (d && device[i]->Priority() < d->Priority()) pri = 6; // receiving but priority is lower
the first loop does not match the if condition. It would work if d was initially set to the last device. But I don't know if this would make problems in any other cases.
Christian
Christian Wieninger schrieb:
Am Dienstag, 23. Mai 2006 18:30 schrieb Andreas Brugger:
of the devices (first or last):
- cDevice *d = NULL;
- cDevice *d = device[0]; // or device[numDevices]
So the two timers should evaluate to different priorities.
Any thoughts on that? Do I miss something again?
I think so ;-) because if d is set to first device then
else if (d && device[i]->Priority() < d->Priority()) pri = 6; // receiving but priority is lowerthe first loop does not match the if condition. It would work if d was initially set to the last device. But I don't know if this would make problems in any other cases.
Ohhh ... right! I haven't thought that through completely it seems ... there is also a problem that the first (or last) device would be returned even if the devices doesn't provide the channel.
This isn't as easy as I thought ...
Christian Wieninger wrote:
Am Dienstag, 23. Mai 2006 22:01 schrieb Andreas Brugger:
This isn't as easy as I thought ...
no it isn't ;-) But I think my patch should solve the problem. At least it works fine here. So let's see what Klaus is thinking of it (if he is reading this).
I believe the main problem with the current GetDevice() function is that it doesn't query *all* devices, but rather selects the "first best" device.
The attached patch implements a new way of selecting the device. It makes sure all devices are queried and selects the one with the least "impact".
The various criteria are represented as parts of an integer number, and in the end the device with the smallest value wins. The most important criteria are pushed into the integer first, resulting in the highest order bits, thus making the most difference.
In this patch the new calculation is done in parallel to the old one, and the result is logged in case the decision would be different. The final result is still taken from the old calculation, but once you find the new one stable enough, you can enable the 'return dd' line.
There is a lot of debug output in there, so that we can see the individual parts of the decision making. This will be removed in the final version.
Please give this a try, and let me know whether it fixes all known problems.
Klaus
Am Freitag, 26. Mai 2006 15:18 schrieb Klaus Schmidinger:
Please give this a try, and let me know whether it fixes all known problems.
A quick test of the problem described previously seems to work fine: (I'm listing only the relevant log entries, dd as return value was not active)
// Start of recording with prioritiy 45 at 00:27 on device 2 Mai 27 00:27:00 linux vdr: [5506] switching device 2 to channel 2 Mai 27 00:27:00 linux vdr: [5506] timer 65 (2 0027-0100 'Blond am Freitag') start Mai 27 00:27:00 linux vdr: [5506] record /video0/Blond_am_Freitag/2006-05-27.00.27.45.99.rec ... // Start of recording with prioritiy 5 at 00:27 on device 1 Mai 27 00:27:01 linux vdr: [5506] switching device 1 to channel 1 Mai 27 00:27:02 linux vdr: [5506] timer 66 (1 0027-0125 'Nachtmagazin') start Mai 27 00:27:02 linux vdr: [5506] record /video0/Nachtmagazin/2006-05-27.00.27.05.99.rec ... // Start of recording with priority 50 at 00:29 Mai 27 00:29:00 linux vdr: [5506] ******* new GetDevice would select device 1 in stead of 2 Mai 27 00:29:00 linux vdr: [5506] stopping recording on DVB device 2 due to higher priority Mai 27 00:29:03 linux vdr: [5506] switching device 2 to channel 6 Mai 27 00:29:03 linux vdr: [5506] timer 67 (6 0029-0220 'Nich' mit Leo') start Mai 27 00:29:03 linux vdr: [5506] record /video0/Nich'_mit_Leo/2006-05-27.00.29.50.99.rec ... Mai 27 00:29:03 linux vdr: [5506] stopping recording on DVB device 1 due to higher priority ... Mai 27 00:29:04 linux vdr: [5506] switching device 1 to channel 2 Mai 27 00:29:04 linux vdr: [5506] timer 65 (2 0027-0100 'Blond am Freitag') start Mai 27 00:29:04 linux vdr: [5506] record /video0/Blond_am_Freitag/2006-05-27.00.27.45.99.rec
//XXX what if Priority() is < -1? -> limit to -1..MAXPRIORITY???
how can one set a priority below -1 and what would this mean? As far as understood the cDevice class, this can only be done in a cDevice-derived class. So perhaps cDevice::Priority should limit its return value to -1 ... MAXPRIORITY?
Christian
Christian Wieninger wrote:
...
//XXX what if Priority() is < -1? -> limit to -1..MAXPRIORITY???
how can one set a priority below -1 and what would this mean? As far as understood the cDevice class, this can only be done in a cDevice-derived class. So perhaps cDevice::Priority should limit its return value to -1 ... MAXPRIORITY?
The priority comes from the cReveicers attached to the device:
cReceiver(int Ca, int Priority, int Pid, const int *Pids1 = NULL, const int *Pids2 = NULL, const int *Pids3 = NULL); ... ///< Priority may be any value in the range 0..99. Negative values indicate ///< that this cReceiver may be detached at any time (without blocking the ///< cDevice it is attached to).
So officially the value can't be greater than 99, but it may well be any negative value. However, VDR itself only uses '-1', and I guess it's fair to assume that no plugin will be using too large negative values. To be on the safe side I guess I'll do
imp <<= 8; imp |= min(max(device[i]->Priority() + MAXPRIORITY, 0), 0xFF);
which would allow values in the range -99..99, and in any case limit the result to 0..255.
Klaus
Klaus Schmidinger wrote:
Christian Wieninger wrote:
...
//XXX what if Priority() is < -1? -> limit to -1..MAXPRIORITY???
how can one set a priority below -1 and what would this mean? As far as understood the cDevice class, this can only be done in a cDevice-derived class. So perhaps cDevice::Priority should limit its return value to -1 ... MAXPRIORITY?
The priority comes from the cReveicers attached to the device:
cReceiver(int Ca, int Priority, int Pid, const int *Pids1 = NULL, const int *Pids2 = NULL, const int *Pids3 = NULL); ... ///< Priority may be any value in the range 0..99. Negative values indicate ///< that this cReceiver may be detached at any time (without blocking the ///< cDevice it is attached to).
So officially the value can't be greater than 99, but it may well be any negative value. However, VDR itself only uses '-1', and I guess it's fair to assume that no plugin will be using too large negative values. To be on the safe side I guess I'll do
imp <<= 8; imp |= min(max(device[i]->Priority() + MAXPRIORITY, 0),0xFF);
which would allow values in the range -99..99, and in any case limit the result to 0..255.
I just noticed that I have inadvertently removed assigning 'ndr' to the resulting 'NeedsDetachReceivers'. So attached is an updated version of the fix.
Klaus
I just noticed that I have inadvertently removed assigning 'ndr' to the resulting 'NeedsDetachReceivers'. So attached is an updated version of the fix.
Klaus, does this patch you just posted nullify both of Anssi Hannula's patches (vdr-1.4.0-transfermode-primary-limit.diff and vdr-1.4.0-transfermode-priority4.diff) or only the "priority4" patch.
Best Regards.
Stone wrote:
I just noticed that I have inadvertently removed assigning 'ndr' to the resulting 'NeedsDetachReceivers'. So attached is an updated version of the fix.Klaus, does this patch you just posted nullify both of Anssi Hannula's patches (vdr-1.4.0-transfermode-primary-limit.diff and vdr-1.4.0-transfermode-priority4.diff) or only the "priority4" patch.
Well, first of all it was meant to fix a problem with timer conflicts. Maybe it also helps for the problems Anssi has encountered - but that's a question he should answer.
Klaus
On 5/27/06, Stone syphyr@gmail.com wrote:
I just noticed that I have inadvertently removed assigning 'ndr' to the resulting 'NeedsDetachReceivers'. So attached is an updated version of the fix.
Klaus, does this patch you just posted nullify both of Anssi Hannula's patches (vdr-1.4.0-transfermode-primary-limit.diff and vdr-1.4.0-transfermode-priority4.diff) or only the "priority4" patch.
It does indeed nullify the "priority4" patch :)
The vdr-1.4.0-transfermode-primary-limit.diff is still valid. It changes the behaviour of the PrimaryLimit config option from "primary device protector" to "liveview protector". Note that Klaus has said he might be removing the PrimaryLimit alltogether in the near future as it's quite useless.
-
Andreas Brugger -
Anssi Hannula -
Christian Wieninger -
Klaus Schmidinger -
Stone