sengsational / cwhelper

The code here is the background job for CW_EPG, an electronic program guide / DVR for Windows
1 stars 1 forks source link

Channels in lineup.xml not properly populated in /channels list #10

Closed sengsational closed 9 months ago

sengsational commented 9 months ago

The most important thing that I’ve seen is that we need somehow to give higher priority to the lineup.xml?tuning data, as it’s not finding its way into the /channels list (based on not seeing valid “frequency” or “protocol” values there when hdhomerun_config.exe is not available).

Edit: See my comment below about using the http query data.

sengsational commented 9 months ago

It makes a big difference in processing depending on how "userHdhrCommandLine" and "allTraditionalHdhr" are set, so when looking through the code, there's too many moving parts. So it might be helpful to just have the logs when you bring-up CwHelper and this problem happens. Along with the grab of what the tuner is responding with on lineup.xml.

TPeterson94070 commented 9 months ago

Sorry, it’s not that channels are missing. It’s just that the channels are clearly not enjoying the full data available in the lineup.xml?tuning response. I.e., they are missing correct “frequency” and “protocol” when the HTTP API is used when hdhomerun_config.exe is not installed.

EDIT: After further review.... :) I finally remembered why we needed to use the false "frequency" (and PID) values to force VC-only tuning. But that doesn't affect the protocol values, which should reflect what's in the xml?tuning (or hdhomerun_config.exe scan) reports.

TPeterson94070 commented 9 months ago

It makes a big difference in processing depending on how "userHdhrCommandLine" and "allTraditionalHdhr" are set, so when looking through the code, there's too many moving parts. So it might be helpful to just have the logs when you bring-up CwHelper and this problem happens. Along with the grab of what the tuner is responding with on lineup.xml.

stdout204133940520822.txt is an example of startup with two HDHR units, one 4k (108xxxxx) and one not (107xxxxx) with hdhomerun_config.exe not available. It shows that the lineup.xml?tuning request is actually performed:

Sun Feb 04 13:23:07 PST 2024 Executing request http://192.168.1.18/lineup.xml?tuning
finished executing request with 42957 characters received.
Sun Feb 04 13:23:07 PST 2024 Clearing channels for tuner: 1080F19F-0
Sun Feb 04 13:23:07 PST 2024 133 channels added to tuner: 1080F19F-0
Sun Feb 04 13:23:07 PST 2024 Adding new or changed: 1080F19F-1

and here is a snip from the http response:

<Program>
<GuideNumber>102.1</GuideNumber>
<GuideName>KTVU-HD</GuideName>
<Modulation>atsc3</Modulation>
<Frequency>177000000</Frequency>
<PLPConfig>0</PLPConfig>
<ProgramNumber>1377</ProgramNumber>
<VideoCodec>HEVC</VideoCodec>
<AudioCodec>AC4</AudioCodec>
<HD>1</HD>
<Favorite>1</Favorite>
<URL>http://192.168.1.18:5004/auto/v102.1</URL>
</Program>

But, despite the fact that the response includes valid frequency and protocol data, the stdout.txt channel listing shows that these are not used:

channelName="102.1" alphaDescription="KTVU-HD" input="1" **protocol="8vsb"** favorite="false" sourceId="" tuner="1080F19F-0" deviceId="276885919" tunerType="2" channelDescription="102.1 KTVU-HD" channelVirtual="102.1" **frequency="102"**
sengsational commented 9 months ago

Because we have different flavors of XML that produce a "ChannelDigital" object, we run a risk of breaking things if we force the current example to work, irrespective of other XML flavors. For this reason, we should understand under what conditions these other XML flavors appear, and make sure we preserve that functionality, while fixing the current example.

You can look at this code [here] for what goes on to create the channel from the xml.

This code looks for a "Name" entry in the XML. If it doesn't find "Name" it sets "virtualHandlingRequired" to true. (<<<TRUE for this case)

It also looks at "GuideNumber", and and if it doesn't have a "dot", it sets "isVirtualCable" to true. (<<<FALSE for this case)

If "virtualHandlingRequired" is false, it uses the contents of to populate the protocol, defaulting to "8vsb" if the value is not populated (a comment in the code says this is the pre-October 2018 traditional processing).

if "virtualHandlingRequired" is true, it sets the protocol to qam256 or 8vsb, depending on whether "isVirtualCable" is true or not. (<<<This is what runs in this case, and is why we see "8vsb" and not the contents of "Modulation")

Unfortunately, there's no explanation in the code as to why not having a "Name" entry meant alternative processing was required. I suspect that XML that didn't have a "Name" entry did not have a "Modulation" entry.

So perhaps we can try not including this more modern XML from going though "virtualHandlingRequired" by narrowing the trigger for that? I'm at a disadvantage here because I've only had over-the-air functionality, and never have had hands-on cable-based experience.

TPeterson94070 commented 9 months ago

I think I know where the two xml “flavors” come from. One is the complete data from lineup.xml?tuning and the other is the shortened version generated when the ?tuning parameter is not set. (Just use that url in your browser to see the difference)

I’ve been further ruminating on how we got to where we are with this mess and I now remember that we were driven to it by the fact that neither of the xml responses include more than one entry for each virtual channel pair, while the hdhomerun_config.exe list has all channels, including the duplicate vc pairs for the LD stations. This is the key fact that I had forgotten when I thought that we could get the RF.PID information from the xml responses by using the ?tuning parameter. 😒

So I think we’re stuck with the “all traditional” and “vc-only” modes (darn it) and we’ll have to require hdhomerun_config.exe for the former. But we can fix that protocol guess anyway—and prune the ATSC 1.0 tuners’ channel list of ATSC 3.0 channels.

TPeterson94070 commented 9 months ago

The reason for using the absence of "Name" to force VC-only tuning is because that parameter only exists in the hdhomerun_config.exe scans and the xml scans have "GuideName" instead. The logic in forcing VC-only is still valid (because the xml lists lack any LD duplicate vc pairs) but we should retain the protocol value that is in the xml?tuning list.

TPeterson94070 commented 9 months ago

I just reviewed the xml from Steve's HDHR Prime and see that his cable channels have integer "GuideNumber" values, so that is the rationale for using the absence of a "." to choose between OTA and cable settings. (That xml also has "Modulation" tags, but they say "auto", which is not helpful here.

sengsational commented 9 months ago

If you understand this code [here] maybe you could propose how it could be updated.

It sounds like getting the contents of the Modulation entry into the protocol variable is one thing.

And I'm not sure if you want to abandon the "fake frequency" we've got in there and put a real frequency in there instead (here)

TPeterson94070 commented 9 months ago

Let's leave the fake frequency and fake pid as they are for now. Changing that would also require changes in CW_EPG and we still wouldn't have a true equivalent of "allTraditional=true" because we'd be missing the LD duplicate vc pairs which was the motive for creating that mode in the first place. So there's no upside to such a change.

I'll post a PR proposal for the ChannelDigital method later this evening.

TPeterson94070 commented 9 months ago

I have a working version of ChannelDigital. It creates HDHR lists with no change in the source selection rules. I'll put it in a PR now. It leaves 3 issues untouched:

  1. The ATSC 3.0 filtering for ATSC 1.0-only tuners.
  2. Improper use of the Silicondust Antenna Digital.xml file, which does not include protocol information. It should only be used for units that don't support http lineup queries, but it gets used for all units whenever allTraditional=true. So the logic (wherever it is) that disables the lineup query in this case needs to be fixed.
  3. The RfFromMegahertz method is still tied to us-bcast/us-cable definitions. We need to look at either broadening it to non-US standards or simply disabling RF.PID tuning for non-US lineups. (I think the latter is an OK solution)
sengsational commented 9 months ago

I think "1." is being addressed in https://github.com/sengsational/cwhelper/issues/11

And "3." needs it's own issue.

It seems to me "2." is this issue

... we need somehow to give higher priority to the lineup.xml?tuning data, as it’s not finding its way into the /channels list

You should probably describe the intention for the change in your PR here in this issue.

TPeterson94070 commented 9 months ago

Today's PR changed the ChannelDigitalmethod to use the actual protocol data, if any, passed to it and use "auto" if none passed (i.e., when the Antenna Digital.xml file is the source). It also changed the RfFromMegahertz method to depend on OTA vs Cable rather than 8vsb vs QAMxxx.

The remaining part of this issue is to locate and change the logic that is disabling the http lineup.xml fetch from "new" HDHR units when allTraditional is true so that they can use that instead of the Antenna Digital.xml file.

sengsational commented 9 months ago

Thanks for the descriptions. Those changes in the PR look good to me.

The remain part is happening in LineupHdhr.scan().

Here's a rough write-up of my notes from 1/4, slightly enhanced from then:

Set vChannelTuner to false if hdhomerun_config.exe returns some specific (old) tuner type names. This uses hdhomerun_config.exe, if available.

Set xmlFileName and airCatSource from the registry.

Then we do one of these 3 things:

1) if vChannelTuner, get lineup from http lineup.xml or, if useExistingFile is false, use lineup_status.json and lineup.post?scan=start. I think useExistingFile is false only if the user requests a scan.

2) if not vChannelTuner and not useExistingFile use hdhomerun_config.exe to scan (old/slow).

3) if not vChannelTuner and useExistingFile (this is designated 'default processing', is loaded with conditionals)

A) set the validXmlExists variable, starting with xmlFileName we found earlier (false if file not round, false if "CableCard", false if older than the scanFile.

B) set previousScanExists based on existence of file.

if not validXmlExists and not previousScanExists then use lineup_status.json and lineup.post?scan=start and set liveXmlAvailable and set liveXmlFileName

if validXmlExists use the data in the file

if not validXmlExists and previousScanExists use the scan file

if not validXmlExists and not previousScanExists user the live xmlFileName contents created above.

I'm not sure how much of this makes sense, but I think we need to see which path is followed through this code to determine the logic that is disabling the http lineup.xml fetch from "new" HDHR units when allTraditional is true.

TPeterson94070 commented 9 months ago

I'm not clear on the useExistingFile logic. It strikes me a a holdover from the original situation where we had scan...txt files that were created with some pain so we wanted to preserve them rather than needing to re-scan. Now, however, for the http-capable units getting the response to lineup.xml?tuning is as fast as reading a disk file and a whole lot more current, since the units do periodic rescans of their own. So, effectively, validXmlExists is always true for them. [EDIT: I believe that I had misinterpreted validXmlExists. If it refers to the HDD file, it should always be FALSE for new tuners]

We certainly want to avoid running the scan=start routine unless the user explicitly requests it from the Options menu.

It's really unfortunate that none of the xml files retains duplicate vc pairs, so our only alternative for accessing those is via the scan...txtfiles created with hdhomerun_config.exe.

sengsational commented 9 months ago

I think useExistingFile is only false when we really do want to do a painful/slow scan, so I'm less worried about that one. I think the problem is that we're going through "1)" above.

The way it works today, when HDHR tuners are created, if useHdhrCommandLine is false, all HDHR tuners are tagged as VCHANNEL devices (as opposed to ORIGINAL). That might have been true at an earlier point in the evolution of hardware, but I'm not sure it's true as things have changed.

As I mentioned above, we need to figure out what path is being followed, and I suspect it's path "1." One problem that is a bug is that "debugBuf" is not printed out unless it goes through path "3." or hit's an exception. You can fix that by moving this line down to just before the } catch (... line near 119, and run it.

I'm pretty sure we'll see vChannelTuner: true, and, as I understand it, that should not be true for over-the-air tuners?

So if the tuner should not be considered vChannel, we go back to where vchannel is assigned or maybe are smarter about setting the isVchannel instance variable in the constructor.

TPeterson94070 commented 9 months ago

If hdhomerun_config.exe is not present (and therefore useHdhrCommandLine is false) none of the ORIGINAL tuners is visible to us, so the visible tuners are all VCHANNEL and path 1 is fine (as long as we don't use Antenna Digital.xml).

TPeterson94070 commented 9 months ago

N.B.: CW_EPG checks for the Silicondust Windows package and insists that it be installed if no tuners are visible. Even if there are vChannel tuners, CW_EPG posts a notice on first run advising that additional capabilities would be available if the package were installed.

TPeterson94070 commented 9 months ago

Regarding Antenna Digital.xml: The file is created by scanning with hdhomerun_setup.exe, so it (and the Registry path to it) really shouldn't be there if the Windows package was not installed, but let's make sure that it's not sniffed out anyway.

TPeterson94070 commented 9 months ago

I've just re-checked the lineups produced with allTraditional true and false. It seems that path 1 above is indeed followed and the http lineup query is used when it is false (i.e., VC-only tuning on new tuners). However, when it is true, Antenna Digital.xml is used for all tuners that don't have a newer scan...txt file. That is the desired result for "old" tuners, but it is not desired for any http-capable tuners (i.e., "new" aka vChannel tuners). They should use the http query whether or not allTraditional is true if they don't have a scan...txt file.

Now...we can't use the "which file is newer" logic in this case because the http query will always win. But if we give preference to the scan...txt file, how to stop using a deprecated one? Maybe CW_EPG should have a menu option to delete a tuner's scan...txt file and ask the user upon opening Options if any that are older than a certain amount should be removed? Or (a bit more difficult) it could compare the scan and http lists and delete any file that is indistinguishable from the http query. There probably should be a menu item for deleting a scan file in any case.

sengsational commented 9 months ago

We spend the initial part of path 3 in the scan() code in making three booleans: validHdhrXmlExists, previousScanExists, and liveXmlAvailable. We set validHdhrXmlExist to false if it's older than the scan file (looks like we went back and forth on this strategy). The other two booleans are what they seem to be.

Then we take them in the order above. The first "true" wins.

Is the solution as simple as putting liveXmlAvailable first? On old tuners that need a scan file, this won't be true, as they don't answer http. But there must be a reason why we're not always putting liveXml first?

TPeterson94070 commented 9 months ago

Actually, I think that we want to use the scan file if the user has gone to the trouble of creating it, so liveXmlAvailable should be last, as is. But we never want to use the Antenna Digital.xml file for "new" units, so I think that means that validHdhrXmlExist should always be false for them. Then, as I suggested above, we need to have a new menu item in CW_EPG Options to (maybe alert the user to) delete antiquated scan files.

sengsational commented 9 months ago

I was thinking that liveXmlAvailable being true was equal to a "new" unit, but I'm often confused about these lineup sources, lol! You've always had a much better handle on this than I have. I'll defer to your approach.

TPeterson94070 commented 9 months ago

So--if I do understand this correctly--setting validHdhrXmlExist to false for new (i.e., vChannel) units and keeping the logic in Path 3 should work as desired.

Then I'll add the delete-scan-file option to CW_EPG's Options Tuners right-click menu.

sengsational commented 9 months ago

vChannelTuner should be true with non-old tuners, so we go down path #1 (paths #2 and #3 become irrelevant). The boolean validHdhrXmlExists is in path 3, so I'm not sure what you're going for there.

If the way it runs today is ok, except for the problem is having an unwanted scan file, then adding the ability to remove the scan file would solve the problem.

TPeterson94070 commented 9 months ago

The way it runs today is not ok. When allTraditionalHdhr is true, the old and new HDHR units run through path #3 and that causes the list to be generated from Antenna Digital.xml. (E.g., see this file that had the debug printing of skipAtsc3Channel enabled so it confirms that the list is coming from the source without protocol information, Antenna Digital.xml, and not the http query that does have it: [stdout208133424024086.txt] (https://github.com/sengsational/cwhelper/files/14225588/stdout208133424024086.txt))

Path #3 should have validHdhrXmlExists set to false for "new" tuners and true for "old" ones. That will cause the "new" tuners to fall through to the http query unless there is a scan file, in which case the file should be used irrespective of its date. The logic for "old" tuners of using the newer of the scan file or Antenna Digital.xml is fine.

TPeterson94070 commented 9 months ago

I just realized that I've not made an important esoteric point (AFAICR) about the scans on "new" tuners. We want those to be done using the http scan=start procedure if allTraditionalHdhr is false, but otherwise they should be done using hdhomerun_config.exe, just as for the "old" tuners. The reason for this is that the motive for creating allTraditionalHdhr mode was to access multiple transmitters using the same virtual channels and none of the xml lists retain those, so we need the txt file from the Silicondust Windows utility scan, which keeps all channels regardless of duplicated vc. OTOH, in VC-only mode, using the xml file ensures that the vc having the best signal quality is retained.

sengsational commented 9 months ago

You have made that point before, and it's one that I've not really got pre-loaded into my head properly. Too bad SiDust is not making it easy on us as we try to get the most out of that multiple transmitter, same virtual channel thing.

TPeterson94070 commented 9 months ago

Yeah, I think they believe that they're making it "easy" by selecting the best quality of the multiple sources. But here in multipath city that's a moving target and the user needs options.

TPeterson94070 commented 9 months ago

I just posted a new Sengsational CW_EPG with my latest CWHelper in the usual place. I need to fix the former's icon it seems, but it has somewhat revised handling of the channel scanning and the right-click Tuners menu has some added preambles for the "Scan for Channels" and (new) "Remove Scan Record" items to prevent unwitting execution of those. (Neither of them appear for tuners to which they don't apply--nonHDHR or lacking scan...txt file)

Also, the preamble for the Scan for Channels is customized for VC-only vs RF.PID setting of the tuner.

TPeterson94070 commented 9 months ago

Need some guidance. vChannelTuner is used to select the path here and it evidently is made false somehow when allTranditionalHdhr is true. How can I distinguish "new" and "old" tuners in this case? I thought I could use tuner.isHttpCapable() but that seems to have the undesirable side effect of causing a lineup scan of its own.

TPeterson94070 commented 9 months ago

I finally realized that I needed to use the qualified field name ((TunerHdhr)tuner).isHttpCapable() to get the correct result. I'm still not home free, as that stray scan=start is still present, but that's likely a logical error that will come to light eventually for me. Edit: I see where the scan is coming from, but haven't yet worked out the fix.

TPeterson94070 commented 9 months ago

I think I've got it! I've reposted the current Sengsational CW_EPG and my latest CWHelper in the usual spot. Also, I'll push the latter to feature_s in a bit.

sengsational commented 9 months ago

I haven't looked at the changes yet, but this, from above, might shed some light:

The way it works today, when HDHR tuners are created, if useHdhrCommandLine is false, all HDHR tuners are tagged as VCHANNEL devices (as opposed to ORIGINAL). That might have been true at an earlier point in the evolution of hardware, but I'm not sure it's true as things have changed.

And this:

So if the tuner should not be considered vChannel, we go back to where vchannel is assigned or maybe are smarter about setting the isVchannel instance variable in the constructor.

But if you've got something working that you're happy with and didn't need to mess with the setting of the tuner's isVchannel, that's great.

BTW, I didn't look at the isHttpCapable(), but it's not named right...typically the is...() methods just return a boolean and don't run anything. A better name might be getHttpCapable() if it goes out and does something.

TPeterson94070 commented 9 months ago

Um...isHttpCapable() was already there, I just used it to differentiate between "old" and "new" tuners. The "stray" scan wasn't really connected to that method, as I first thought, but was triggered by the new differentiation. I got rid of it by removing the call in Path #3 to

                    `loadChannelsFromXml(liveXmlFileName, airCatSource, tuner);` 

and replacing it with this pair of calls that mimic the lineup fetch for vChannel tuners.

sengsational commented 9 months ago

I wasn't indicating you added the method and named it wrong, I was indicating I probably could have named it better. The name is ok, but not great. I like how you made it call once and "remember" in subsequent calls.

The changes to scan() do have a couple of things that really could have improved names. I understand you worked with what was there already, but there are a couple of variable names that morphed in their purpose, so the names don't align. I don't want to step on your toes, but think the best way to manage this one, rather than me describe what I'm thinking, is for me to check in a change. My goal will be for zero change in logic, just to improve code readability (make the variable names align).

TPeterson94070 commented 9 months ago

I initially made some changes to that method (when I thought that it was the culprit behind the stray scans) but ended up removing them and leaving it as originally written. (I haven't yet read through your later comments, but will do so now.

sengsational commented 9 months ago

The change has been merged into master branch.