Allow player to retry read in case of read errors

[Karlson2k]

As discussed here: 5c7a19a#commitcomment-8220173
@topfs2 @FernetMenta for review

[topfs2]

This looks good to me, should make InputStreamFile work with the IFile changes.

[Karlson2k]

@FernetMenta Will this PR fix a problem with PVR and player?

[topfs2]

After some discussion on IRC, nope

We need to do if (ret == 0) m_eof = false;
return ret

That should work if all VFS have used the same trick for negative numbers.

[Karlson2k]

@topfs2 That's what this PR do. With additionally explicitly maps all negative values to "-1".

[topfs2]

m_eof = true I mean

[topfs2]

Yeah, just a bit unsure if we should map all negative to -1

[FernetMenta]

Will this PR fix a problem with PVR and player?

i think so though have not tried. i would expand this to the other file implementations as well.

[Karlson2k]

@FernetMenta I wouldn't do that because '-1' can be returned in case that file was closed or was not opened. Or in other cases when we should not attempt to retry. We need more precise mechanism.

[FernetMenta]

how about file->GetErrorNumber() or something like this? this would be a reuse of something we have here: https://github.com/opdenkamp/xbmc-pvr-addons/blob/master/lib/platform/sockets/socket.h#L61

[Karlson2k]

@FernetMenta It's good idea, but, as discussed on IRC, we decided to postpone function GetStatus() 😄 to I*.

[Karlson2k]

@FernetMenta Your "OK" is required before @topfs2 will approve this.

jenkins build this please

[FernetMenta]

we decided to postpone function GetStatus()

I don't agree here because I invested hours of work when I was doing AE and paplayer to get things working. pulling the network cable must not result in termination of playback. I was actually on my way to test this but then I hit this "exception".
If this behavior broke, I want it to be fixed again.

this is btw how most applications and browsers do behave as well.

[Karlson2k]

This fix will return old behavior with indefinitely waiting player for new data.
Only precise status was postponed.

Just a hint: when implementing something not very obvious, describe it in doxy, or at least in comments. Better use both. This will help reviewer to suggest correction if needed and will help other to use and update your code.

[FernetMenta]

I would like to test this but master is broken for me testing on Windows. I can't start kodi. does it work for you?

[Karlson2k]

Did you re-run DownloadBuildDeps.bat, DownloadMingwBuildEnv.bat and make-mingwlibs.bat ?

[FernetMenta]

yes I did. have you tried current master?

[Karlson2k]

Rebuilding mingw libs currently.

[Karlson2k]

Looks like master is currently broken for win32.

[Memphiz]

rebrand fallout?

[Karlson2k]

@Memphiz Seems to be so.
I suspect that BIN_DIR is incorrectly set / get.

[Memphiz]

@Karlson2k bool CApplication::InitDirectoriesWin32() in Application.cpp has XBMC_HOME set instead of APP_HOME

[Memphiz]

but that matches the VS2010Express/XBMC.defaults.props

[Montellese]

Setting APP_HOME instead of XBMC_HOME in my VS debugging configuration allows me to start XBMC again.

[Montellese]

See #5542 for a possible fix.

[FernetMenta]

I think this pr is fine though there are still some issues left. I did the test I described above: play some music from a smb share, pulling the network cable, and plugging it in again. This worked for approximately 50% of my attempts, means it continued playback from the last position. The other 50% I got eof and player moved to the next item. I don't know if this has ever worked reliable but some day it should.

[Karlson2k]

@FernetMenta I suspect same was before all VFS changes. :)
@topfs2 Get in?

[Memphiz]

@FernetMenta you are onto something here. I remember alot of debug logs where playback of dvdplayer was stopped because of premature eof. I think this behavior was there before for some VFS implementations and the fact that you are seeing this now too doesn't mean that it is related to the VFS changes. It looks more like this was never working in 100% of the use cases.

[topfs2]

Looking at the smb code there is a retry logic and if it fails it return 0.
So I guess it should be possible to return -1 but we need to check
throughout the code for -1 which I'm not sure we do. In paplayer we don't I
think. But there its messy and on a per codec base.
On 21 Oct 2014 10:06, "Memphiz" notifications@github.com wrote:

@FernetMenta https://github.com/FernetMenta you are onto something
here. I remember alot of debug logs where playback of dvdplayer was stopped
because of premature eof. I think this behavior was there before for some
VFS implementations and the fact that you are seeing this now too doesn't
mean that it is related to the VFS changes. It looks more like this was
never working in 100% of the use cases.

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

[FernetMenta]

talk is about the read function. I don't see a read function in Win32SMBFile.cpp

[topfs2]

I was thinking about the one used on the other platforms https://github.com/xbmc/xbmc/blob/master/xbmc/filesystem/SMBFile.cpp#L533 (man I really dislike that windows has its own implementation :P)

[topfs2]

Hmm, Which before returned 0 on the final If but I think thats changed too with the PR

[FernetMenta]

man I really dislike that windows has its own implementation :P

ah, that explains it. I configured a smb share on Windows and the method called was CWin32File::Read