TVRec::CheckChannelPrefix erroneously detecting channels with underscore spacer

[Tom Dexter <digitalaudiorock@…>]

When CheckChannelPrefix? in libs/libmythtv/tv_rec.cpp checks for channels using the various spacers ("", "_", "-", "#", ".") it can erroneously find channels matching the underscore ("_") as it's a SQL wildcard character.

I noticed that, for example, when I enter '51' on my remote, it gets changes to 5_1 even though I'm using '.' (5.1 for example) in all my channel numbers.

I tried to address this by changing the following line:

QString qprefix = add_spacer(prefix, spacers[j]);

...to:

QString qprefix = add_spacer(prefix, spacers[j] == "_" ? "
_" : spacers[j]);

...to force the underscore to be escaped. This stopped it from adding the underscore separator, but for some reason, did not get it to use the '.' separator. I can't figure out why not, as it appears the SQL would find channels usging (for example) LIKE '5.%'.

[Tom Dexter <digitalaudiorock@…>]

Test 3966 patch for fixes

[Tom Dexter <digitalaudiorock@…>]

Test 3966 patch for trunk

[Tom Dexter <digitalaudiorock@…>]

Ah...I think I found it. In addition to the SQL wildcard fix mentioned in my description, the block of code in CheckChannelPrefix? that is intended to check for a matching channel without a prefix was actually checking with the prefix. This prevented it from properly telling the client that a spacer was needed. I've attached preliminary patches for fixes and trunk. I've only tested in fixes, and even then, only with my specific setup, which is three DVB tuners, all with just digital OTA channels (that is, all with spacers in the names).

[Tom Dexter <digitalaudiorock@…>]

Sorry...in the above post I meant to say 'spacer' not 'prefix'...sorry for the confusion.

[danielk]

(In [14499]) Refs #3966. Fixes two related bugs in ATSC channel matching, a wildcard character was not being escaped in one instance, and a spacer was being used in the no-spacer channel check.

[danielk]

Can you try the patch I just attached with the current svn trunk. When testing your patch I also noticed that if you use different separators such as "_" for one set of channels and "." for another set prevents smart channel changing from working, so you need to wait for the channel change OSD to fade out or hit the select key for the channel change to take effect.

[Tom Dexter <digitalaudiorock@…>]

Unfortunately I can't actually. All my mythtv machines are running -fixes (0.20.2 svn 14324). I could probably adapt it to my fixes version and test that if it'd help. Just to be clear...that patch is in addition to my changes (the ones in 14499) correct?

[Tom Dexter <digitalaudiorock@…>]

I was able to apply those changes to my -fixes install and everything seems to work great as far as I can tell. All my tuners use the same channel numbers, but I did try renaming several with different separators and it all seems to work fine.

I'm not sure how good of a test that is, but I guess this code hasn't changed significantly between fixes and trunk.

By the way...I'll attach a -fixes version of your patch.

[danielk]

(In [14506]) Refs #3966. Applies patch to allow smart channel changing to commit channel numbers of the form 51 where 51 maps to 5_1 on the current tuner.

I tested this with various channel aliasing situations, and Tom Dexter has tested with his setup.

[Tom Dexter <digitalaudiorock@…>]

Full patch for fixes version of tv_rec.cpp

[Tom Dexter <digitalaudiorock@…>]

I've added an svn diff patch (3966-backport-fixes.patch) that has all the above changes backported to the current svn 0.20-fixes. I noticed that the 14506 changeset was a bit different from the changes I tested (from 3966-v1.patch). This patch has all the changes as shown in the trunk changesets 14499 and 14506, and I've tested it with my fixes setup.

[danielk]

(In [14574]) Refs #3966. Backports [14499] from trunk to 0.20-fixes. Two related bugs prevented channel entry of ATSC channels with a numeric only remote in some cases. See ticket for details.

[danielk]

(In [14575]) Fixes #3966. Backports [14506] from trunk to 0.20-fixes. Fixes smart channel changing so that it commit's channel numbers of the form 51 where 51 maps to 5_1 or 5.1, etc. on the current tuner. Tested by myself and Tom Dexter.

[ismael@…]

after [14499] and [14506] smart channel changing doesnt work for non DVB/ATSC channels... doesnt change channel until OSD fades. the problem resides in the function: CheckChannelPrefix? returning spacer needed when there is not a spacer on the channel number. [14444] worked ok I'm using ivtv 1.0.0 (gentoo 2.6.22)

[Tom Dexter <digitalaudiorock@…>]

I've duplicated what ismael has described in my 20-fixes install. If I rename one of my DVB channels with no spacer (for example changing 4.2 to 42) entering 42 changes to it but not until the "42" has faded from the OSD. I'll try to get a chance to debug it a little further.

[Tom Dexter <digitalaudiorock@…>]

I think I found the problem. The is_extra_char_useful variable is inadvertently getting set to true when the entire channel is keyed in. This is even true with DVB channels. I have a remote button mapped to the '.' character, and if I actually enter the entire channel number '4.1' without letting the smart channel change fill in the spacer, it waits for the OSD timeout as well. The problem is in this block of code:

    // Is an extra characher useful for disambiguation?
    if (query.exec(basequery.arg(prefix)))
        is_extra_char_useful = query.next();
    for (uint i = 0; i < ((is_extra_char_useful) ? 0 : fchannum.size()); i++)
        is_extra_char_useful = (fchannum[i] != add_spacer(prefix, fspacer[i]));

The sql query (which didn't exist in the older code) is always setting is_extra_char_useful to true and preventing the following for loop from executing. In my case, even when prefix was 4.1 and all values in fchannum where set to '4.1' and all values of fspacer where "", the loop (which would never set is_extra_char_useful to true) was never executing. If I comment out the sql query everything seems to work fine.

[danielk]

Try to be more agressive in finding matching channels

[danielk]

Ismael & Tom, can you test the attached patch. It attempts to be more aggressive in finding a match.

The logic of the first part is that if fchannum[i] != add_spacer(prefix, fspacer[i]) then there are more characters after the prefix in the channel and more characters would be useful. Since we check with the "" (blank) spacer, this will encompass all channels with additional characters in the string.

The removal of the "if (fcardid[i] != cardid) break;" in the last commit loop means we will commit channels on other recorders even if we need to add a spacer. This I'm still a bit uncertain of, because we don't check if a recorder is actually available for recording before doing this. TV::ChangeChannel?() will do this, but we could actually choose needed_spacer so as to prefer an available recorder, which TV::ChangeChannel?() can not do. This is no worse than the current behavior, but we could do better without much more work.

[ismael@…]

danielk: the patch works perfectly for me. tnx!!!

[Tom Dexter <digitalaudiorock@…>]

That works fine for me. Note however that I have no way of really testing the more aggressive logic, as I have the same DTV channel lineup for all three tuners, so obviously I can never hit an exact match in that loop when i>0, as I'd always hit it for the current tuner first and return.

Your logic definitely looks sound to me however. Since you're only doing this when is_extra_char_useful is 0 I can't see any way that your change could ever cause an unintended channel change or anything.

[danielk]

(In [14728]) Refs #3966. Makes optional smart channel changing a little smarter.

This makes the code a little more aggressive in finding a match so that when there is only one possible channel that can be selected from the input the user has typed it will be committed without waiting for the OSD to fade or for the user to hit select. Before we would only autocommit completely unambiguous channel changes.

[danielk]

(In [14745]) Refs #3966. Accidentally left a debugging VERBOSE macro enabled in [14728]...

[danielk]

(In [14746]) Fixes #3966. Backports [14728] to 0.20-fixes.

This fixes an annoying problem with the optional smart channel changing where we wait for additional characters for a channel number entry which is ambiguous, but further characters will not disambiguate the entry.