search

google / sagetv

SageTV is a cross-platform networked DVR and media management system
http://forums.sagetv.com/
Apache License 2.0
267 stars 173 forks source link

Fix: ATSC Scanning Blank and Garbled Numbers #342

Closed CraziFuzzy closed 7 years ago

CraziFuzzy commented 7 years ago

Fix for scanning issues discussed in issue #338

Narflex commented 7 years ago

Have you had a few people test this to make sure it doesn't break existing working systems as well as fix the one who has issues? Your analysis sounded pretty thorough, but testing is always good. ☺️

CraziFuzzy commented 7 years ago

Only testing on my own system, where I was able to reproduce the OP's issue. Figured these were some pretty 'safe' changes.

CraziFuzzy commented 7 years ago

I never was able to determine the source of the odd characters getting into the channel name. What was odd, and reproducible, was that any channel name that used the full 7 characters had the same garbled response following the 7, when run in Docker, but NOT occurring with the same build on bare ubuntu. My only thoughts were perhaps the docker has a different default encoding set so that the non-initialized string was being interpreted differently there. In any case, nulling it out first takes away any question about how it needs to convert it.

Narflex commented 7 years ago

I'm on vacation right now, and I can review it in more detail when I get back to see if I come to the same conclusion...or if a couple other people test it I'm ok with merging it. The changes themselves aren't clearly safe because as individual lines they appear to be changing how things work....but I do understand that if I was to look at it overall I'd likey come to that conclusion.

Jeff Kardatzke Sent from my Android

On Jul 21, 2017 9:19 AM, "Christopher Piper" notifications@github.com wrote:

Only testing on my own system, where I was able to reproduce the OP's issue. Figured these were some pretty 'safe' changes.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/google/sagetv/pull/342#issuecomment-316924879, or mute the thread https://github.com/notifications/unsubscribe-auth/ANEIDCK23MEozgHLV99xBfPtZebwe0yhks5sQFDzgaJpZM4Oe2Gh .

CraziFuzzy commented 7 years ago

No worries. This is not a 'new' problem, so much as it's one that really didn't exist en masse until people started moving to Linux, and all it really causes is needing to massage the lineup a bit after the scan.

Not adding the line break to the scaninfo is probably the most significant change, though having spent a bit of time last year in the parsing code in the STV, I really have no idea why that was added in, as nothing indicates any reason for line breaks in the semi-colon delimited channel list.

In any case, enjoy the vacation.

texneus commented 7 years ago

I'd be more than happy to test, but I'm going on vacation myself in a week, so it will need to be quick! Let me know what I can do.

On a side note, I read today that Dallas-Ft. Worth is one of the first markets to go through a spectrum repack in preperation for ATSC 3. Channels begin moving around this fall, and allegedy some of the low power stations I was getting are already shutting down (sub channels turned off, others have just color bars, probably as existing contracts run out). The channel scanner will probably get a good workout the next few months...

CraziFuzzy commented 7 years ago

Thanks @texneus . I could send you the .so files zipped up and you could drop them into your sage folder to test, but frankly, I don't know if that's really necessary. I have pretty much your exact setup, and confirmed the bug before, and the fix after. What would be more helpful is users who do NOT have the same config (different tuners, different OS's, etc) to test it to make sure it didn't break them.

Narflex commented 7 years ago

Fuzzy,

I took a look at this again now that I'm back and I really would like to have somebody test this on a Windows setup and then also somebody else who has an already working setup on Linux to test it. As you're aware, this code is really ugly and complex and it's possible there's some other dependency somewhere else on how this works that could get broken by this change.

jfthibert commented 7 years ago

I took a quick look at this, according to

https://www.atsc.org/wp-content/uploads/2015/03/Program-System-Information-Protocol-for-Terrestrial-Broadcast-and-Cable.pdf

the VCT name is stored as 7 characters of UTF-16, the whole vct is already 0 initialized at the top of the loop and the name is defined as char name[9*2];. I'm not sure why it wasn't set as 8*2 but the proper fix in the loop is to simply copy the first 14 bytes. The current proposed fix seems right after removing the unnecessary memset.

CraziFuzzy commented 7 years ago

@jfthibert that's funny, i stared at this code so many times, and never even noticed that it is already nulled out. I was actually unfamiliar with what declaring with ={0} did in the case of struct's. I can certainly remove the unnecessary memset - as you said, the core of the issue was grabbing more than the 14 bytes of the name.