Fixed pvr.hts ReadMessage data timeout to user settings timeout. #181
Conversation
|
@opdenkamp: Please take a moment time to look at this. It took me one week's time to debug this. I am running XBMC on RaspPi, focusing on TVHeadEnd with PVR.HTS add on. Every few minutes the PVR.HTS would drop the connection to tvheadend, reporting a timeout problem. Which it is not. The reason for timeout is, that a bogus packet size was requested. I found out, that the 5 millisecond timeout leads to a situation, where in the first size read, the timeout of 5 ms occurs after 1 to 2 bytes have actually already been consumed from the HTSP stream. The timeout stuff obscures, that a few bytes could actually be read. In the next size read, one or two remaining bytes from the packet size are read plus the first bytes from the packet as the size of the coming packet, which gives a bogus packet size, and possibly cannot be malloced. The stream in this case is out of sync and needs to be reopened. I have set this to 50 ms, which makes it work reliable on my Pi. Thanks |
|
Fixed and rebased this to work reliable on RaspPi. Unfortunately, i also had to patch the tcpsockets parts of it. I cannot test the impact of this on other PVR implementations. |
| // read error, close the connection | ||
| XBMC->Log(LOG_ERROR, "%s - failed to read packet size (%s)", __FUNCTION__, m_socket->GetError().c_str()); | ||
| TriggerReconnect(); | ||
| return NULL; | ||
| } | ||
|
|
||
| l = ntohl(l); | ||
| l = (lb[0] << 24) + (lb[1] << 16) + (lb[2] << 8) + lb[3]; |
There was a problem hiding this comment.
why? this breaks the code for other platforms.
There was a problem hiding this comment.
As far as I understood, network byte order is always Big-Endian. Please explain to me.
There was a problem hiding this comment.
but the byte order on the host differs, that's why such functions like ntohl exists.
There was a problem hiding this comment.
ok, you add byte by byte. that's really ugly, isn't it?
There was a problem hiding this comment.
Better than an union or memcpy i thought...
There was a problem hiding this comment.
To me this looks right. The manual combination of bytes is correct imho. This is an "invented" byte order that tvheadend writes to the stream. It has nothing to do with ntohl.
There was a problem hiding this comment.
This is an "invented" byte
I don't think so. hts works pretty much the same way as vnsi and vnsi writes the length with htonl.
There was a problem hiding this comment.
I would not do a non-block read in TcpSocketRead if something partial times out. Just do not cloak the bytes actually read before timeout with the error code.
In my point of view a call to
Read(buf,256,1000 /* timeout */)
means, read at most 256 bytes and return number of bytes read and NOT read all or some and return -ETIMEOUT as the result if it finally timed out.
There was a problem hiding this comment.
If poll times out, verify by doing a non-blocking read. This does not cloak any bytes read earlier. You can even do in addition to what you have already added.
The higher level code can decide how it would like to handle the results. In this case the changes to HTSPConnection.cpp would be obsolete.
There was a problem hiding this comment.
https://github.com/tvheadend/tvheadend/blob/master/src/htsmsg_binary.c#L224 is how it's written to the stream. No htonl there.
|
ping @dezi any progress? |
|
I see more posts in the forum where connection terminates which points to this issue. @opdenkamp are you taking this if @dezi does not respond? |
|
i'll have a look at it after nuking the ffmpeg header stuff we got. let's hope @dezi responds before i got time to fix this up ;-) |
|
I dunno if its related but actually when I'm using tvheadend protocol over WiFi with various equipment I keep having disconnect/reconnect up to a point its impossible to watch anything. Sometimes, it works. All equipment do not change so WiFi link quality is supposed to be constant as neighbors are unlikely to interfere... And using WiFi to download using DLNA or when upgrading system I have good throughput. Just my 0.02$ |
|
While trying to hunt the problem for WiFi, I am a bit puzzled by the way g_iConnectTimeout is initialized. ./temp/xbmc.log:14:26:05 T:139989492954880 DEBUG: CAddonCallbacksAddon - GetAddonSetting - add-on 'Tvheadend HTSP Client' requests setting 'connect_timeout' However the file pvr.hts/settings.xml still contains the 10 value after exiting while the GUI after restart indeed show 11 in addon settings. So its stored elsewhere but the file is still rewritten. I did a grep but found nothing in xbmc directory. So where is it stored? ls -l ./userdata/addon_data/pvr.hts/settings.xml => Jul 5 14:26 Edited : this was due to an old version of the setting file |
|
use the forums for questions like this. github is to discuss code |
|
As I'm hunting a bug in this area and this PR itself may solve it, I borrowed the thread. Now if you prefer me to bug you to fix it via a vocal forum post... |
|
yes, do not borrow threads. you're spamming everyone listed in this thread and everyone who watches this repos by doing that. |
|
I used a slightly modified version of the patch and indeed it fixes my annoying WiFi connectivity errors. I still have freeze but it recovers as when bit rate is not high enough compared to media bit rate. Kudos to @dezi. On the proposed patch code itself, I would like to say that os-socket.h should be more heavily modified so that API stay consistent between read and write and also of UDP vs TCP sockets. |
|
where is your slightly modified patch, what does it fix that this patch does not. |
|
Any technical reason (performance may be?) to use poll for TCP and select for UDP in os-socket.h? The modification is that I check in line 295 if (lread > 0)of HTSPConnection.cpp highlighted above if the short read is due to timeout and fail otherwise instead of checking it later on in the else part like in the proposed patch. The patch will be in my github fork fork of your tree but I will not push it because I have no time and no way to test effects on API semantic change on all pluggins. I may find time send you a proposal for a new os-socket.h. |
|
uhm, that was what i said what should be done indeed. i'll fix this up myself but i don't know whether i'll have time before the end of the merge window. |
|
No pressure from my side now that I have a kludgey/partial/hugly fix : my children can now really use the second TV via a WiFi connection. |
|
@EricV - do you have that on your branch so I can test, a quick looks seems that it hasn't any changes |
|
If you llok at the last message I said I will push... Just done it. |
…on't discard it but wait for the rest to come in within iDatapacketTimeout. issue #181
|
see #209 |
…on't discard it but wait for the rest to come in within iDatapacketTimeout. issue opdenkamp#181
Hi, I use Your great tools TVHeadend + XBMC PVR.HTS add-on. Thumbs up.
I have two RPIs running, one is my TVHeadend-Server, works like a charm. I use this with XBMC + PVR.HTS on Mac and Windows. Works perfect.
The second RPI runs a self compiled HEAD XBMC, with self compiled HEAD PVR-Addons. PVR playback of recordings worked sometimes, it kept dropping the TVHeadend HTSP connection, so I had a closer look into source code.
When You do the ReadMessage in the thread-loop, You use the default timeout for data to be read which is one second. When my TVHeadend Raspberry is under heavy load, which it always is ;-), it will not respond within time, so i patched this and now it works great.
I wonder, why the OSX and Windows versions (FRODO, not self compiled) do not have this problem.
I also have problems playing back the live streams. I will do some more research here. One symptom is, when Live-Stream plays, it has 1:1 aspect ratio. Most stream cannot be played at all.
Cheers
Dezi