Attempt to fix #92

[beralt]

Issue #92 seems to be caused by data in the buffer left after a previous seek. This fix is by no means the most elegant solution, but clearing the whole buffer seems to fix the issue. All credits go to @Glenn-1990, I merely tested his ideas.

[ksooo]

@FernetMenta your opinion on this would be highly appreciated.

[Glenn-1990]

@beralt
The memset is not really an elegant solution but was just to pinpoint the problem,
you are really sure that this solves it for you?
What if you revert the 1st commit, does it still fix the issue?

[FernetMenta]

I am not familiar with hts and why this buffer on client side is needed. I don't see the network layer considered. What about packets already send by the server that have not been copied to the buffer? As mentioned already, for vnsi I tag the packets with an id that I change on seek. The client discards all packets that don't match the id.

[opdenkamp]

the buffer is @adamsutton 's work, part of the changes when he refactored the whole lot. no idea why it's needed, but @adamsutton or @Jalle19 should know

[adamsutton]

@FernetMenta There is no issue with data in transit. There is an application level request/response (over TCP), we wait for the response and either we get it (no loss) or we don't, and if the latter we assume something went wrong and close the VFS handle (like your index), and start a new one.

@opdenkamp This has nothing to do with the re-factoring, this is code that was originally in pvr.hts (though I did add it). Unfortunately my commit in which I added the buffer is very unhelpful and appears to relate to another feature, I'll slap my wrists for that!

I honestly don't remember why I buffered client side, probably because I was overly worried about latency caused by making unnecessarily small requests across the network. And assumed that if I provided Kodi more data than it asked for it would (rightly) get upset.

To be honest I was probably being overly worried about network latency, since Kodi obviously should be buffering itself anyway. So it's probably fair to say it's a redundant feature. Feel free to put it back to the original design without the intermediate buffer if you like.

[beralt]

@Glenn-1990 Just did another stress test with various combinations. Seems that beralt@5213ada is the only change required to get stable seeking. So beralt@4c3ed62 is not required (there already is a code path leading to reset of the buffer after a successful seek).

[Glenn-1990]

@beralt Yes, I kwow, but the reset was only called after the seek was finished.
It actually was to prevent wrong data getting it when the server is processing the seek ;-)

I could reproduce the issue once with the memset :s
Maybe related to my slow wifi, but we shouldn't say to quickly that all this is caused by the ring-buffer.

[adamsutton]

I've not looked at this issue, I've been out of the loop for a long time. But seriously, programming 101, memset() on a circular buffer is NOT required, no one in their right mind would waste cycles zero'ing a ring buffer in an embedded, low MIPs, time critical application (or any application, if they're sane, unless they're doing it for debugging purposes) when you only need set the in/out pointers (and really it should only set the one pointer, but in this context both is benign).

So I think whatever the problem is, this not the fix.

[opdenkamp]

@opdenkamp https://github.com/opdenkamp This has nothing to do with
the re-factoring, this is code that was originally in pvr.hts (though
I did add it). Unfortunately my commit in which I added the buffer is
very unhelpful and appears to relate to another feature, I'll slap my
wrists for that!

Well, then you've added it a bit earlier as part of some other
refactoring :) Just pinged you as you probably knew why you had added it ;-)

[beralt]

I do think that network latency is a factor in this issue, seeing how many people complain about the severity while I can only reproduce this by seeking repeatedly. My setup has TVHeadend and Kodi on the same machine, hence lower latency.

The memset() on the buffer in my case prevents the issue from occurring, so maybe someone could use that information to properly solve the issue.

Closing this PR and having this discussion in the issue seems like a good plan, as the proper solution to this issue is probably beyond my capabilities at this time.

[adamsutton]

@opdenkamp I added it because I added the feature ;) before this the addon only supported playback via plain HTTP rather than HTSP, which meant there were significant limitations.

Just wish I knew why I added the extra level of buffering. Personally if I'd say if it's causing issues and nothing else does it this way, drop it.

@beralt The memset() cannot be the solution, pin = pout (or in this case pin=0, pout=0) has the same basic effect. Unless we want to posit a race condition in updating said pointers and thus the content of the memory becomes more relevant.

[beralt]

@adamsutton I really appreciate the explanation. I'm pretty sure there is a race condition here, and I know that clearing the buffer is the wrong approach. There is a dependency on the latency between Kodi and TVHeadend and the rate at which the seeking issue occurs.

So the theory is that the pointers of the buffer somehow get moved during seeking due to a race condition. So either the race condition has to be solved, or the client-side buffer should be removed. Sorry for all the noise, I'm just trying to help.

[Jalle19]

This buffer wasn't used in a multi-thread environment before we refactored the VFS handler to read/write on separate threads. This could be why the issue hadn't shown up earlier.

[adamsutton]

To be honest, without having tried to remember why we/I bothered to add the buffer it the first place, I'd say it's caused a couple of problems in the past, it's not what the other addon's do, so it's just an extra complexity we don't need.

I'd completely remove the buffer and it would greatly simplify the code. Any concerns about race conditions "should" go away at this point.

To be honest, looking at the current code I can see a problem that's similar to what @FernetMenta described. And his idea of a per seek "key" is quite a good one. And possibly something we could add, but it needs changes in TVH.

[Jalle19]

If pvr.vdr.vnsi doesn't use buffering here then you're right, I guess we don't need it either. I just spent many hours fiddling with this and that and have concluded that if you add a very small delay in SendFileSeek (changing tvhtrace to tvhdebug is enough) the problem goes away. I have a feeling the issue is that m_buffer is accessed without locking (since it uses locking internally) when it shouldn't be (it's possible to read when you should actually be waiting for a reset).