Skip to content
This repository has been archived by the owner on Apr 15, 2023. It is now read-only.

[pvr.hts] fill the VFS buffer on a separate thread #404

Merged
merged 3 commits into from
Jan 21, 2015

Conversation

Jalle19
Copy link
Collaborator

@Jalle19 Jalle19 commented Jan 14, 2015

@adamsutton can you review this? I'll start testing it at home tonight, I have to upgrade my HTPC anyway.
@opdenkamp you can also take a look if you have time

@Jalle19 Jalle19 changed the title Vfs thread buffer [pvr.hts] fill the VFS buffer on a separate thread Jan 14, 2015
@opdenkamp
Copy link
Owner

just one question: do we really need a setting for this?

@Jalle19
Copy link
Collaborator Author

Jalle19 commented Jan 14, 2015

@opdenkamp personally I'd say we don't, it was Adam's idea to add it. I'm afraid some users will see it and think "wtf I don't want any extra buffering", turn it to zero and watch their recordings stutter. If @adamsutton is fine with removing it then I'll remove it.

@adamsutton
Copy link
Contributor

@opdenkamp @Jalle19 it's probably overkill, drop it.

@adamsutton
Copy link
Contributor

Something feels a bit wrong about the condition handling, I'm not convinced about it being used in both directions (though I'm not really sure why). And I'm not sure we need to stall in the read() call unless there is NO data available.

I'm feeling like shit today though, so I really can't focus on it properly, I'll try and take a look in a couple of days.

@Jalle19
Copy link
Collaborator Author

Jalle19 commented Jan 14, 2015

If we return -1 from the read method (which we could if we don't wait for data), Kodi may decide the stream died and stop playback. At least that's how I figured it.

@adamsutton
Copy link
Contributor

@Jalle19 I wasn't suggesting that, I was suggesting that you return as much data as you can while asking for more. However I think that's also probably wrong, Kodi will probably assume that if we return less than it asked for that that indicates the end of the file. So probably fine as is.

Clearly my brain is seriously addled today!

@opdenkamp
Copy link
Owner

not so sure about that, i haven't checked, but i think len is just the maximum read size (size of the buffer) and returning less is probably fine. though it'll probably just lead to more hammering on this method :)

@Jalle19
Copy link
Collaborator Author

Jalle19 commented Jan 15, 2015

I say we play it safe and continue with the old behavior (only return if we can return the requested length).

@Jalle19
Copy link
Collaborator Author

Jalle19 commented Jan 15, 2015

I dropped the setting.

@opdenkamp
Copy link
Owner

squash and merge away, before we copy stuff over to a new repos ;-)

@Jalle19
Copy link
Collaborator Author

Jalle19 commented Jan 20, 2015

I haven't had time to test it extensively yet, I can try to do that tonight.

@Jalle19
Copy link
Collaborator Author

Jalle19 commented Jan 21, 2015

Tried to backport this to a Helix compatible addon but only got lots of missed packets and crap like that. Don't know if I did something wrong yet or if this just doesn't work. Will need to try on an Isengard Kodi to be sure.

@Jalle19
Copy link
Collaborator Author

Jalle19 commented Jan 21, 2015

Found the bug, it was something I introduced last minute. This works fine now on Windows.

gradually increase the read chunk size (allowing for more jitter in the
latency between client and server) gradually without making the reader
wait for the full chunk to be received.

The read length is reset during seeking and if the buffer happens to
run dry (ffmpeg does around 10 short seeks for each seek request so it
helps to keep the read size small).
Jalle19 added a commit that referenced this pull request Jan 21, 2015
[pvr.hts] fill the VFS buffer on a separate thread
@Jalle19 Jalle19 merged commit 67ae0d5 into opdenkamp:master Jan 21, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants