Remove circular buffer from the VFS #100

Jalle19 · 2015-09-19T15:35:53Z

As mentioned by @adamsutton in #99, this drastically simplifies the code and indeed seems to fix #92. @ksooo @adamsutton for review (especially the first commit which is kinda unrelated), @FernetMenta for sanity check.

@ksooo we'll have to backport this to Isengard even though it's quite a drastic "fix".

ksooo · 2015-09-19T16:05:23Z

I have no objections re Isengard backport. This bug seems PITA for many people.

Despite from the multithreading question I raised, the change looks okay to me.

EDIT: @Jalle19 I wanted PR number 100! ;-)

Jalle19 · 2015-09-19T17:25:39Z

I removed the mutex because AFAIK the whole class is only accessed from one thread. m_fileId was never mutex protected anyway (except as a side effect where locking was used anyway).

ksooo · 2015-09-19T17:29:34Z

Ah, okay then.

Jalle19 · 2015-09-19T17:36:57Z

@ksooo do you mind runtime testing this? @Glenn-1990 you too perhaps?

ksooo · 2015-09-19T18:39:05Z

I never was able to reproduce this bug. Thus I could only test whether things still do work for me.

adamsutton · 2015-09-19T19:57:58Z

@Jalle19 @ksooo could you hold off merging this till I've had a chance to read it? Possibly tomorrow, Monday latest. I've had a very quick scan and I think a few changes are probably in order, but I need to read it through more carefully.

adamsutton · 2015-09-19T20:13:12Z

src/HTSPVFS.cpp

-  m_bHasData = false;
-  m_bSeekDone = true;
-  m_currentReadLength = INITAL_READ_LENGTH;
-  m_seekCondition.Signal();
 }

 int CHTSPVFS::Read ( unsigned char *buf, unsigned int len )


Did I really write a function using int's like this! urgh! really this should be something sensible like size_t

I was obviously having a bad day, or just putting up with how Kodi dev's had done things.

adamsutton · 2015-09-19T20:34:05Z

@Jalle19 @ksooo

Couple of mistakes in my comments, I can't remember the whole locking philosophy, but I think the lock removals are OK, I think these were due to addition of extra threads.

I've made some suggested (but untested) changes, adamsutton@ae43216

beralt · 2015-09-20T07:02:16Z

I would be happy to assist with some testing if needed.

Jalle19 · 2015-09-20T08:14:06Z

@adamsutton I added your patch and runtime tested it, seems to be working.

ksooo · 2015-09-20T08:37:38Z

src/HTSPVFS.cpp

  htsmsg_destroy(m);
-  return true;
+
+  return (int)read;


Could you please use a c++ style cast here.

To make @adamsutton happy maybe we should return size_t all the way up the chain and perform the cast to int in client.cpp (since that's where the int requirement comes from)?

Jalle19 · 2015-09-20T09:08:48Z

@ksooo @adamsutton better?

ksooo · 2015-09-20T12:08:29Z

@Jalle19 👍

ksooo · 2015-09-20T12:46:47Z

Runtime-test was not a success. Crashes immediately when trying to play a recording.

Compile time warning:

..../pvr.hts-master/src/HTSPVFS.cpp: In member function 'CHTSPVFS::SendFileRead(unsigned char*, unsigned int)':
....pvr.hts-master/src/HTSPVFS.cpp:296:74: warning: 'buffer' may be used uninitialized in this function [-Wmaybe-uninitialized]
   if (htsmsg_get_bin(m, "data", buffer, reinterpret_cast(&read)))

=> https://github.com/kodi-pvr/pvr.hts/pull/100/files#diff-8c04d63d169ca5038cb71164664a467bR272

Crash:

0  0x00007fe74ff489dd in htsmsg_get_bin (msg=, name=, binp=0x0, lenp=0x7fe73a7f3a50) at /home/kai/src/openelec/OpenELEC.tv/build
.OpenELEC-Generic.x86_64-5.95.5/pvr.hts-master/lib/libhts/htsmsg.c:381
#1  0x00007fe74ff418ae in CHTSPVFS::SendFileRead(unsigned char*, unsigned int) (this=this@entry=0x7fe744002f40, buf=0x7fe704233100 "pnp-org:device-1-0\">\n    
\n        1\n        0\330\060#\004\347\177", len=) at /home/kai/src/openelec/OpenELEC.tv/build.OpenE
LEC-Generic.x86_64-5.95.5/pvr.hts-master/src/HTSPVFS.cpp:296
#2  0x00007fe74ff4191f in CHTSPVFS::Read(unsigned char*, unsigned int) (this=0x7fe744002f40, buf=, len=) at /home/kai/src/openele
c/OpenELEC.tv/build.OpenELEC-Generic.x86_64-5.95.5/pvr.hts-master/src/HTSPVFS.cpp:100
#3  0x00007fe74ff40aa0 in CTvheadend::VfsRead(unsigned char*, unsigned int) () at /home/kai/src/openelec/OpenELEC.tv/build.OpenELEC-Generic.x86_64-5.95.5/pvr.h
ts-master/src/Tvheadend.h:581
#4  ReadRecordedStream (pBuffer=, iBufferSize=) at /home/kai/src/openelec/OpenELEC.tv/build.OpenELEC-Generic.x86_64-5.95.5/pvr.ht
s-master/src/client.cpp:572
#5  0x0000000000ca988e in PVR::CPVRClient::ReadStream(void*, long) ()
...

bas-t · 2015-09-20T12:51:58Z

I cherry-picked this into Isengard
Runtime test in Isengard: success
(Debian Jessie)

bas-t · 2015-09-20T13:08:51Z

However, when trying to play a recording (directly, not in a nfs share), kodi crashes.
This may or may not be the result of just cherry-picking and not properly backporting.

See: http://pastebin.com/8Muu4j5h

puithove · 2015-09-20T13:11:11Z

Same crash here on Jarvis - LiveTV fine, immediate crash trying to play a recording. I'm a total git noob though. cherry-picked so I could build against 4.0.0 API.

Jalle19 · 2015-09-20T15:57:42Z

I probably screwed something up with the reinterpret cast, will look into it later.

Jalle19 · 2015-09-20T16:12:37Z

@ksooo mind testing again?

bas-t · 2015-09-20T16:21:52Z

@Jalle19 Your latest commit fixes the crash, but the recording will still not play.

ksooo · 2015-09-20T16:41:25Z

@Jalle19 the reinterpret_cast is okay; not related to the crash.

This should fix it:

diff --git a/src/HTSPVFS.cpp b/src/HTSPVFS.cpp
index 3067bc1..10961f2 100644
--- a/src/HTSPVFS.cpp
+++ b/src/HTSPVFS.cpp
@@ -269,7 +269,7 @@ long long CHTSPVFS::SendFileSeek ( int64_t pos, int whence, bool force )
 ssize_t CHTSPVFS::SendFileRead(unsigned char *buf, unsigned int len)
 {
   htsmsg_t   *m;
-  const void **buffer;
+  const void *buffer;
   ssize_t read;
 
   /* Build */
@@ -293,7 +293,7 @@ ssize_t CHTSPVFS::SendFileRead(unsigned char *buf, unsigned int len)
   }
 
   /* Get Data */
-  if (htsmsg_get_bin(m, "data", buffer, reinterpret_cast(&read)))
+  if (htsmsg_get_bin(m, "data", &buffer, reinterpret_cast(&read)))
   {
     tvherror("malformed fileRead response: 'data' missing");
     read = -1;
@@ -302,7 +302,7 @@ ssize_t CHTSPVFS::SendFileRead(unsigned char *buf, unsigned int len)
   }
   else
   {
-    memcpy(buf, *buffer, read);
+    memcpy(buf, buffer, read);
   }
 
   /* Cleanup */

bas-t · 2015-09-20T16:57:11Z

"The crash" is not right. I was obviously suffering from another crash.
So again: @Jalle19 Your latest commit fixes the crash that I'm experiencing (fullproof reproducable), but the recording will still not play. This is not related to the crash that @ksooo reported.

ksooo · 2015-09-20T17:20:17Z

@Jalle19 For me, things work just fine without your latest commit and my patch for the data type of "buffer".

@bas-t cannot see how latest commit from Jalle19 could fix your crash, which is according to the log you posted, exactly the crash I encountered, btw. Could you maybe test without Jalle19's latest commit and with my patch included instead?

bas-t · 2015-09-20T17:22:13Z

@ksooo will do.

bas-t · 2015-09-20T17:37:44Z

@ksooo it won't compile. Am I missing something ? I get this:

/data/kodi-addons/pvr.hts/src/HTSPVFS.cpp: In member function ‘ssize_t CHTSPVFS::SendFileRead(unsigned char_, unsigned int)’:
/data/kodi-addons/pvr.hts/src/HTSPVFS.cpp:296:58: error: expected ‘<’ before ‘(’ token
if (htsmsg_get_bin(m, "data", &buffer, reinterpret_cast(&read)))
^
/data/kodi-addons/pvr.hts/src/HTSPVFS.cpp:296:58: error: expected type-specifier before ‘(’ token
/data/kodi-addons/pvr.hts/src/HTSPVFS.cpp:296:58: error: expected ‘>’ before ‘(’ token
CMakeFiles/pvr.hts.dir/build.make:169: recept voor doel 'CMakeFiles/pvr.hts.dir/src/HTSPVFS.cpp.o' is mislukt
make[2]: *_* [CMakeFiles/pvr.hts.dir/src/HTSPVFS.cpp.o] Fout 1

ksooo · 2015-09-20T17:47:51Z

@bas-t you f****d up the patch file, I guess. Try this one: http://paste.ubuntu.com/12504317/

ksooo · 2015-09-20T17:54:13Z

@bas-t in fact it was me, who killed the patch file while pasting it into gh comment.... Always forget about problems with > and < and html. haha.

bas-t · 2015-09-20T17:54:43Z

@ksoo no, you did. look at the difference!
Anyhow, with the corrected patch, it compiles just fine.
Going to test it now

Op 20-09-15 om 19:47 schreef Kai Sommerfeld:

@bas-t https://github.com/bas-t you f****d up the patch file, I
guess. Try this one: http://paste.ubuntu.com/12504317/

—
Reply to this email directly or view it on GitHub
#100 (comment).

bas-t · 2015-09-20T17:55:44Z

I missed your post, and reacted too soon.

Op 20-09-15 om 19:54 schreef Kai Sommerfeld:

@bas-t https://github.com/bas-t in fact it was me, who killed the
patch file while pasting it into gh comment.... Always forget about
problems with > and < and html. haha.

—
Reply to this email directly or view it on GitHub
#100 (comment).

ksooo · 2015-09-20T17:56:02Z

all fine

bas-t · 2015-09-20T18:15:07Z

Runtime test without latest @Jalle19 patch and with @ksooo patch (http://paste.ubuntu.com/12504317/).

SUCCESS! Way to go, both of you (and @adamsutton).

Only remaining thing on my system seems to be that in case I jump to the beginning, or mayby past that, given the size of the jump, I sometimes don't get audio. Skipping a bit forward again restores audio.

Mind you, I'm still testing in Isengard only.
Thank you all so much for this breakthrough, this is exactly what kept me from using Kodi at all.

ksooo · 2015-09-20T18:30:41Z

@bas-t cool. Thanks for your support.

As you've tested on Isengard and I did on Jarvis, we already know that the changes of this PR will work on both versions. Cool.

@Jalle19 mind adding the fix to the addon changelog?

@adamsutton thank you for jumping by and giving the advices needed to find the fix. Especially like the reduction in complexity (less, sometimes is more).

Jalle19 · 2015-09-20T19:16:35Z

Thanks for testing. Should we keep it the way it is now or should I change it back to using reinterpret_cast? IMO a static cast on the return value is "safer" (reinterpret_cast is considered the most dangerous type of cast).

ksooo · 2015-09-20T20:15:44Z

As the reinterpret_cast is semantically correct here and we tested with that cast in place, I'd say let's stick with it.

Jalle19 · 2015-09-20T20:40:57Z

@ksooo done

Jalle19 · 2015-09-20T20:44:22Z

@ksooo added changelog update too, should be good to go now.

ksooo · 2015-09-21T08:25:00Z

@Jalle19 mind squasing commit 3 and 4 into commit 2 (or at least 4 into 2)?

EIDT: or at least the "buffer" fix part of commit 4 into commit 2? ;-)

circular buffer and threading. Fixes kodi-pvr#92, fixes kodi-pvr#99.

Jalle19 · 2015-09-21T09:28:00Z

@ksooo done (took the easiest route)

ksooo · 2015-09-21T10:23:10Z

Thank you, Sam.

Remove circular buffer from the VFS

Jalle19 · 2015-09-21T16:29:38Z

As if by magic this also fixes the ancient bug where the picture is basically frozen if you attempt to fast-forward or rewind in recordings. Now it's just as smooth as for local content.

adamsutton · 2015-09-21T19:19:28Z

@Jalle19 that's great news, and shows why I shouldn't be allowed to code this stuff!

Jalle19 added Improvement Fix Cleanup labels Sep 19, 2015

adamsutton reviewed Sep 19, 2015
View reviewed changes

adamsutton added a commit to adamsutton/pvr.hts that referenced this pull request Sep 19, 2015

vfs: minor corrections to PR kodi-pvr#100

ae43216

Jalle19 pushed a commit to Jalle19/pvr.hts that referenced this pull request Sep 20, 2015

vfs: minor corrections to PR kodi-pvr#100

9c24be0

ksooo reviewed Sep 20, 2015
View reviewed changes

Jalle19 force-pushed the try-fix-seek branch from 44aea98 to e144861 Compare September 20, 2015 20:40

reduce connection mutex lock scope in CHTSPVFS

fef3f5e

Jalle19 force-pushed the try-fix-seek branch from e144861 to b2ae7e3 Compare September 20, 2015 20:43

Jalle19 mentioned this pull request Sep 20, 2015

Isengard backports #103

Merged

Sam Stenvall added 2 commits September 21, 2015 12:27

simplify CHTSPVFS drastically by removing the intermediate

241d8ac

circular buffer and threading. Fixes kodi-pvr#92, fixes kodi-pvr#99.

remove unused includes and using statements

8800657

Jalle19 force-pushed the try-fix-seek branch from b2ae7e3 to 8800657 Compare September 21, 2015 09:27

ksooo added a commit that referenced this pull request Sep 21, 2015

Merge pull request #100 from Jalle19/try-fix-seek

21f76fb

Remove circular buffer from the VFS

ksooo merged commit 21f76fb into kodi-pvr:master Sep 21, 2015

Remove circular buffer from the VFS #100

Remove circular buffer from the VFS #100

Conversation

Jalle19 commented Sep 19, 2015

ksooo commented Sep 19, 2015

Jalle19 commented Sep 19, 2015

ksooo commented Sep 19, 2015

Jalle19 commented Sep 19, 2015

ksooo commented Sep 19, 2015

adamsutton commented Sep 19, 2015

adamsutton Sep 19, 2015

Choose a reason for hiding this comment

adamsutton commented Sep 19, 2015

beralt commented Sep 20, 2015

Jalle19 commented Sep 20, 2015

ksooo Sep 20, 2015

Choose a reason for hiding this comment

Jalle19 Sep 20, 2015

Choose a reason for hiding this comment

Jalle19 commented Sep 20, 2015

ksooo commented Sep 20, 2015

ksooo commented Sep 20, 2015

bas-t commented Sep 20, 2015

bas-t commented Sep 20, 2015

puithove commented Sep 20, 2015

Jalle19 commented Sep 20, 2015

Jalle19 commented Sep 20, 2015

bas-t commented Sep 20, 2015

ksooo commented Sep 20, 2015

bas-t commented Sep 20, 2015

ksooo commented Sep 20, 2015

bas-t commented Sep 20, 2015

bas-t commented Sep 20, 2015

ksooo commented Sep 20, 2015

ksooo commented Sep 20, 2015

bas-t commented Sep 20, 2015

bas-t commented Sep 20, 2015

ksooo commented Sep 20, 2015

bas-t commented Sep 20, 2015

ksooo commented Sep 20, 2015

Jalle19 commented Sep 20, 2015

ksooo commented Sep 20, 2015

Jalle19 commented Sep 20, 2015

Jalle19 commented Sep 20, 2015

ksooo commented Sep 21, 2015

Jalle19 commented Sep 21, 2015

ksooo commented Sep 21, 2015

Jalle19 commented Sep 21, 2015

adamsutton commented Sep 21, 2015