Backend won't change inputs with PVR-150 - regression from 27.6

[ltskinol@…]

I have mythtv 29.1 installed on Arch Linux with a PVR-150 card. That card has multiple inputs and I'm using the Tuner and Composite 1 inputs for two different sources of TV. When I upgraded from a Gentoo 28/fixes release to 29.1 on Arch, Myth has refused to consistently switch between the two inputs - always insisting on recording on the first-defined input. Neither live TV nor recordings worked.

For the PVR-150, you define two tuners (both with the same /dev/video0 device), then associate two listing sources, one per tuner. If the listing sources have different ranges of channels (with the tuner, say, being 2-99 and the composite being 100-199), Myth should (and did) switch between inputs based on changing the channel number.

After _much_ banging of head, I decided to dive into the code, and found a mistake/regression from the release of 27.6 to 28.0. It's in tv_rec.cpp, around line 3540. The change is:

if (!inputname.isEmpty()) {

• curCardID = channel->GetInputID();
• newCardID = channel->GetInputID();

+ curInputID = channel->GetInputID(); + newInputID = channel->GetInputID();

LOG(VB_GENERAL, LOG_INFO, LOC + QString("HW Tuner: %1->%2")

• .arg(curCardID).arg(newCardID));

+ .arg(curInputID).arg(newInputID));

}

if (curCardID != newCardID

!CardUtil::IsChannelReusable?(genOpt.cardtype))

This is 108d12d6507a9b6f61c81a7335e9c40fba96dce5 ( gigem committed on Jul 12, 2015 ) and it's obviously incorrect. curInputID and newInputID now grab the same value, and therefore are always the same. The last if() statement then doesn't execute, which causes the input change to not happen. This looks like a mistake in the refactoring.

I don't know what the proper fix is, but if I hack in "curInputID = 55;", thus ensuring that the last if() statement executes, the input changes as expected.

Please fix.

One other thing: this fixes the input switch when starting a recording, but doesn't address a (perhaps) related bug in that inputs also don't switch when changing channels with live TV. I don't know where to look for that bug.

Also, tv_rec.cpp is identical in this area from version 28.0 through the current head.

Thanks.

[ltskinol@…]

Oops - pasted in commit N+1. That commit was just a rename. Here's the real erroneous change:

if (!inputname.isEmpty()) {

• int current_input = channel->GetCurrentInputNum?();
• int new_input = channel->GetInputByName?(inputname);
• curCardID = channel->GetInputCardID(current_input);
• newCardID = channel->GetInputCardID(new_input);

+ curCardID = channel->GetInputID(); + newCardID = channel->GetInputID();

LOG(VB_GENERAL, LOG_INFO, LOC + QString("HW Tuner: %1->%2")

.arg(curCardID).arg(newCardID));

}

108d12d6507a9b6f61c81a7335e9c40fba96dce5, ( gigem committed on Jul 12, 2015 ).

[gigem]

The attached patch might work.

[ltskinol@…]

Didn't try the patch yet, but doesn't "look" quite right. TuningCheckForHWChange() still retrieves the same value twice (into curInputID and newInputID), prints a confusing debug message, and tests "if (curInputID != newInputID..." (which is never going to be true). Seems like maybe TuningCheckForHWChange() should just be retrieving the new value. Am I wrong?

Thanks.

[gigem]

Yes, you are wrong. The way MythTV works now, TVRec and ChannelBase? objects only ever deal with a single type of input (tuner, composite, etc.). Consequently, TuningCheckForHWChange() is completely superfluous because TVRec will never have to change inputs. Recordings on another input will be done by an entirely different TVRec with its own ChannelBase?. The patch removes TuningCheckForHWChange() and changes TuningShutdowns?() to always cleanup so the other TVRec can do what it needs to do.

[ltskinol@…]

Aha! Didn't understand the patch in the fancied-up trac window and still thought that TuningCheckForHWChange() was sticking around. I'll try the patch tonight and report back.

Do you expect that the patch will also correct changing inputs in live TV? And if not, do you want me to poke around tonight to try and find that bug?

[gigem]

It's hard to say if this will fix anything. Your configuration is very rare these days. I don't think it will hurt anything, though. If anything doesn't work, backend logs with -v channel,record will be needed, preferably with no other recording activity going on.

[ltskinol@…]

The patch didn't work for me (which didn't make sense), so I poked around some more. My theory was that the old "recreate the channel" logic was needed to be in, but that seemed like a hack, so I added a bunch of debug statements. What I found was that Myth created two TVRec's, and then worked with one or the other based on what input it's using - just as you mentioned. The issue seems to be in V4LChannel::SetFormat?(), in this code:

    if ((fmt == currentFormat) || SetInputAndFormat(inputNum, fmt))
    {
        currentFormat = fmt;
    }

That looks a little wonky to me because:

• Shouldn't SetFormat?() only change the format?
• It doesn't seem to consider what happens if the format is unchanged but the input is changed.

But let's say that somehow the class remembers the input and handles that elsewhere. If, as I'm using it, two instances of V4LChannel share the same physical hardware, then the input selection can change behind the back of class instance one by class instance two.

I changed the code to simply say:

    if (SetInputAndFormat(inputNum, fmt))
    {
        currentFormat = fmt;
    }

And things work as expected, with the input change only happening when what the class instance wants doesn't match how the actual hardware is set. I don't think this is less efficient, either, as various things are only commanded to change when the actual hardware is out of sync.

This also fixes live TV tuning, so I'm going to say win-win.

Let me know what you think.

[gigem]

When the card/input to simply input conversion was made, there was a lot of code that got [somewhat] mindlessly converted. I tried to fix up anything that no longer made sense as I noticed it, but some inevitably got through. This is another case. Please try the -2 patch to make sure it still works for you.

FWIW, your worry about the input selection being changed behind the back on one TVRec instance by another is unfounded. The way it is supposed to work now is that only one TVRec at a time should ever have the device open. Consequently, it should be sufficient to set the input accordingly at open time and then never have to change it. That still isn't the case in the code, however, as SetFormat? is called from two different places. I haven't changed that here yet for two reasons. One,I don't have a way to test it and two, the whole EncoderLink?, TVRec, Channel class structure is due for a major refactor.

[ltskinol@…]

I'll give the patch a try tonight - it's essentially what I had so it definitely should work. WRT the input selection changing behind the software's back - makes sense, and I think I noticed that there's a consistent channel->Close() thing now going on. What I wasn't able to prove (didn't spend too much time on it) was that opening a closed channel would always set the input correctly. Coming in cold to the code base, I'm still in the "quantifying what's going on" rather than the "actually understanding the logic" phase.

[gigem]

You've actually done quite well for being so cold to the code, especially since there's so much cruft still there.

[ltskinol@…]

Confirm fix with patch 2. Thanks very much!

[gigem]

Thanks for confirming. I'll get it committed shortly.

[gigem]

Fixed in commit [1f8cd4db].

[David Engel <dengel@…>]

In c0ac5283ab2a4af571837520a40cc718e9e842c8/mythtv:

Fix issues with inputs that support multiple, physical connections

There was some superflous and flat out wrong code left over from the
card/input to input conversion that prevented inputs from switching
among connections like tuner, s-video and composite. Thanks to
ltskinol@… for doing most of the leg work and testing the
fixes.

Refs #13247

(cherry picked from commit 1f8cd4dbf9196fd1c8f8e6096fd2ccee24514878)