Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

file_stream_transform not working properly (again) #105

Open
barbudreadmon opened this issue Feb 9, 2019 · 3 comments
Open

file_stream_transform not working properly (again) #105

barbudreadmon opened this issue Feb 9, 2019 · 3 comments

Comments

@barbudreadmon
Copy link
Contributor

In libretro/fbalpha@df22096 i had to disable file_stream_transform for src/burner/libretro/retro_cdemu.cpp, that's the file for neocd iso operation, it uses fopen/fseek/fclose/fread/ftell/fgets, i couldn't determine which one but one of those drop-in replacement from file_stream_transform is not working as it should because if i enable this stuff, cdda playback is totally messed up.

Steps to reproduce :

  1. remove the hack in makefile.libretro (see my commit)
  2. build the core
  3. try to play a neocd game (NB : some neocd games don't have cdda)
@ghost
Copy link

ghost commented Jul 10, 2019

@albertofustinoni I noticed that these functions don't handle the special pointers for stdin/stdout/stderr and hence cause weird memory errors with simple things like printf() due to filestream functions assuming these global FILE* pointers are really RFILE*, and then it starts casting and dereferencing and things explode... which may be related to the above issue.

@barbudreadmon
Copy link
Contributor Author

barbudreadmon commented Jul 15, 2019

I found the culprit for the fbneo cdda playback : filestream_read is returning the number of byte read while fread returns the number of element read (aka block of size determined by second arg).

Fixing rfread is pretty straight forward (142132e), but to leverage migration from standard file functions to filestream (without using the transform stuff, which is causing other issues thanks to the lack of stdin/stdout/stderr implementation), maybe we should change filestream_read instead with something like this :

int64_t filestream_read(RFILE *stream, void *s, int64_t elem_size, int64_t elem_count)
{
   int64_t output;
   int64_t len = elem_size*elem_count;

   if (filestream_read_cb != NULL)
      output = filestream_read_cb(stream->hfile, s, len);
   else
      output = retro_vfs_file_read_impl(
            (libretro_vfs_implementation_file*)stream->hfile, s, len);

   if (output == vfs_error_return_value)
      stream->error_flag = true;
   if (output < len)
      stream->eof_flag = true;

   return output/elem_size;
}

@ghost
Copy link

ghost commented Jul 15, 2019

I have not seen the element parameter used much in general, have you? Either way, adding it to the existing function would require changing not only RA but all 200 cores and their usage of filestream.

If you really think it's necessary (I don't, as we haven't needed it so far), I would suggest making a second function with a different name that supports this.

Also, I believe internally the element reading is actually done in a loop instead of multiplying the read size by the element count... this behavior can technically work differently depending on the medium, for example with CD-ROM you cannot simply read an arbitrarily high number of bytes at once, but a loop with smaller chunks would work.

@twinaphex Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant