From patchwork Sat May 13 22:34:40 2006 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Anssi Hannula X-Patchwork-Id: 12306 Received: from pne-smtpout4-sn1.fre.skanova.net ([81.228.11.168]) by www.linuxtv.org with esmtp (Exim 4.50) id 1Ff2hc-0007Wf-Ez for vdr@linuxtv.org; Sun, 14 May 2006 00:35:12 +0200 Received: from mail.onse.fi (80.223.77.223) by pne-smtpout4-sn1.fre.skanova.net (7.2.070) id 444495DF001419A4 for vdr@linuxtv.org; Sun, 14 May 2006 00:34:41 +0200 Received: from [10.0.0.3] (kone [10.0.0.3]) by mail.onse.fi (Postfix) with ESMTP id 6D82846C4DD1 for ; Sun, 14 May 2006 01:34:40 +0300 (EEST) Message-ID: <44665F00.4090109@gmail.com> Date: Sun, 14 May 2006 01:34:40 +0300 From: Anssi Hannula User-Agent: Mozilla Thunderbird 1.0.6-7.5.20060mdk (X11/20050322) X-Accept-Language: en-us, en MIME-Version: 1.0 To: VDR Mailing List Subject: Re: [vdr] [PATCH] Priority of transfer-mode should not be -1 References: <4306F832.9030407@cadsoft.de> <4311D492.8040602@gmail.com> <43122CB3.30105@cadsoft.de> <446255A9.2010304@gmail.com> <44640281.3030702@gmail.com> <4465B23F.7080105@cadsoft.de> <4465D763.6030301@gmail.com> <44660148.5000107@cadsoft.de> <44664C12.4050200@gmail.com> <44664F45.8040806@cadsoft.de> In-Reply-To: <44664F45.8040806@cadsoft.de> X-Enigmail-Version: 0.92.0.0 X-BeenThere: vdr@linuxtv.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: VDR Mailing List List-Id: VDR Mailing List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 May 2006 22:35:12 -0000 Status: O X-Status: X-Keywords: X-UID: 9443 Klaus Schmidinger wrote: > Anssi Hannula wrote: > >> Klaus Schmidinger wrote: >> >>> Anssi Hannula wrote: >>> >>>> Klaus Schmidinger wrote: >>>> >>>>> I'm afraid this patch has a nasty side effect. >>>>> >>>>> Asssume the following scenario: >>>>> >>>>> - two devices, primary is 1 >>>>> - all channels are available on both devices, except for channels >>>>> 10, 11 and 12 which can only be received by device 2 >>>>> - channels 10 and 11 are on the same transponder, channel 12 is on a >>>>> different one >>>>> - switch directly to channel 10 -> transfer mode starts >>>>> - press "Up" to switch to channel 11, but that channel is skipped and >>>>> it switches to channel 12 >>>>> >>>>> So I guess I can't accept this patch. >>>> >>>> >>>> Thank you for taking a look. >>>> >>>> However, I don't see why there is such a problem in the above scenario: >>>> >>>> - let's consider PrimaryLimit is 0 (the default) >>>> - SwitchChannel() gets called (device.c line 581) >>>> - PrimaryDevice()->ProvidesChannel() returns false (line 591) >>>> - GetDevice() is called (line 281) >>>> - for the device 1 ProvidesChannel() returns false (line 288) >>>> - ProvidesChannel() is called for device 2 (dvbdevice.c line 767) >>>> - Priority = 0, this->Priority() = -1, so hasPriority = true >>>> - ProvidesChannel() result = true is set in line 785 >>>> - ProvidesChannel() return true and ndr = false (device.c line 288) >>>> - (device[i]->Receiving() && !ndr) evaluates true (line 290) >>>> - pri = 0 so device 2 is selected >>>> - SwitchChannel() calls another SwitchChannel (line 601) >>>> - SwitchChannel() calls SetChannel (line 568) >>>> - and so on >>>> >>>> Hopefully I'll have time to test this scenario later today. >>> >>> >>> Well, just try it - it did behave here as described. >> >> >> Well, I tried it in the following scenario: >> - two devices, primary is 1 >> - all channels available on device 2 only >> - channels 1 and 2 are on the same transponder, 3 and 4 on a different >> one >> - manually switch to channel 1 >> - press up -> vdr switches to 2 >> - up again -> vdr switches to 3 >> - up again -> vdr switches to 4 >> >> It works ok. >> Do you have PrimaryLimit != 0? As it turns out, there is indeed a bug in > > > Well, I normally have PrimaryLimit=0, but I've set it to 10 for testing. > Sorry, forgot to mention that. > >> the patch which appears when PrimaryLimit is something else than 0 and >> affects the channelup & channeldown. >> If you do, try one of the following: >> 1. The above patch with only the Receiving() change. >> 2. The above patch fully and additionally Priority of GetDevice() call >> in device.c line 591 changed from 0 to Setup.PrimaryLimit. >> 3. PrimaryLimit = 0 > > > Sorry, it's late today - but see below, maybe this is all no longer > a problem, anyway ;-) It is, but hopefully not for long :) >>> Looking back at your original posting, I believe the actual problem that >>> triggered all this was that streamdev would interrupt live viewing in >>> Transfer Mode, right? >> >> >> Yes. >> >>> Well, what if streamdev, when selecting the device >>> to use, could avoid the ActualDevice()? >>> >>> To test this (without modifying the cDevice API) you could introduce a >>> >>> bool AvoidActualDevice = false; >>> >>> right before >>> >>> cDevice *cDevice::GetDevice(const cChannel *Channel, int Priority, bool >>> *NeedsDetachReceivers) >>> >>> and inside that function check that variable, and if it is true and the >>> checked device is the ActualDevice(), continue the 'for' loop: >> >> >> I think it would be much better to fix this bug (VDR doesn't prefer free >> devices over transfer-moded) rather than add this kind of workaround. > > > VDR does prefer free devices over ones used for Transfer Mode: No, it doesn't, if the transfer-moded device is a budget card: > Device *cDevice::GetDevice(const cChannel *Channel, int Priority, bool > *NeedsDetachReceivers) > { > ... > else if (!device[i]->Receiving() && !device[i]->HasDecoder()) > pri = 2; // free and not a full featured card This causes the device to be selected no matter if it is in transfer mode or not. It seems to me that the above "2" case and the below "3" case should be swapped. However, Receiving() is supposed to be "Returns true if we are currently receiving." so I think it should still check for transfermode, as it is then definitely receiving. Then the "free and not the actual device" case could be dropped from GetDevice(). > else if (!device[i]->Receiving() && device[i] != ActualDevice()) > pri = 3; // free and not the actual device > else if (!device[i]->Receiving() && !device[i]->IsPrimaryDevice()) > pri = 4; // free and not the primary device > ... > } > > Hmm, come to think of it: could it be that your original problem > no longer exists since VDR 1.3.46? > > 2006-04-09: Version 1.3.46 > > ... > - Now avoiding the 'actual' device when starting a recording, so that a > Transfer > Mode for live tv isn't interrupted. Nope, I'm running 1.4.0 and I also confirmed the problem still exists with vanilla VDR. I've now attached two patches. One adds a check for transfer mode in Receiving() and drops the then-useless "free and not the actual device" case from GetDevice(). The other one is optional: it changes the effective transfer-mode receiver priority from fixed 0 to PrimaryLimit, so that the transfer-mode is also "protected" by PrimaryLimit. diff -Nurp -x '*~' vdr-1.4.0/device.c vdr-1.4.0-fix/device.c --- vdr-1.4.0/device.c 2006-04-14 17:34:43.000000000 +0300 +++ vdr-1.4.0-fix/device.c 2006-05-14 01:06:32.000000000 +0300 @@ -588,7 +586,7 @@ bool cDevice::SwitchChannel(int Directio cChannel *channel; while ((channel = Channels.GetByNumber(n, Direction)) != NULL) { // try only channels which are currently available - if (PrimaryDevice()->ProvidesChannel(channel, Setup.PrimaryLimit) || PrimaryDevice()->CanReplay() && GetDevice(channel, 0)) + if (PrimaryDevice()->ProvidesChannel(channel, Setup.PrimaryLimit) || PrimaryDevice()->CanReplay() && GetDevice(channel, Setup.PrimaryLimit)) break; n = channel->Number() + Direction; } @@ -627,7 +625,7 @@ eSetChannelResult cDevice::SetChannel(co // use the card that actually can receive it and transfer data from there: if (NeedsTransferMode) { - cDevice *CaDevice = GetDevice(Channel, 0, &NeedsDetachReceivers); + cDevice *CaDevice = GetDevice(Channel, Setup.PrimaryLimit, &NeedsDetachReceivers); if (CaDevice && CanReplay()) { cStatus::MsgChannelSwitch(this, 0); // only report status if we are actually going to switch the channel if (CaDevice->SetChannel(Channel, false) == scrOk) { // calling SetChannel() directly, not SwitchChannel()! @@ -1158,7 +1156,7 @@ int cDevice::Ca(void) const int cDevice::Priority(void) const { - int priority = IsPrimaryDevice() ? Setup.PrimaryLimit - 1 : DEFAULTPRIORITY; + int priority = ActualDevice() == this ? Setup.PrimaryLimit - 1 : DEFAULTPRIORITY; for (int i = 0; i < MAXRECEIVERS; i++) { if (receiver[i]) priority = max(receiver[i]->priority, priority);