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

Workaround MSVC fseek/fread bug #276

Merged
merged 1 commit into from Aug 19, 2018

Conversation

@Thealexbarney
Copy link
Contributor

commented Aug 4, 2018

The MSVC runtime has a bug that results in data being returned from the wrong part of the file when using fread after an fseek.

The bug can be reproduced by decoding most HPS files with the CLI program. In the resulting output the right channel will cut out and the left channel will begin playing alternating audio from both channels.

Doing an fseek(FILE, ftell(FILE)); works around the issue in the cases I've seen.

@bnnm

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2018

W.o.w.

But wouldn't it affect like, the whole world? Maybe the trigger is something more complex? That sounds like a lot of seeking to fix it.

Also, shouldn't it be calling "fseeko" instead (like it does a bit above).

@Thealexbarney

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

I'm not completely sure what the trigger is. More testing or investigation is probably a good idea before applying this. I'm not sure why it pops up here and not other places. Maybe because HPS does more seeking compared to most formats?

What I do know is that there are whisperings of this sort of bug online, it doesn't appear to be documented that well, and that workarounds like this tend to get around it.

@kode54

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2018

Are we still using buffered files? I recall a file interface that would only seek within its own buffer, or seek in the file when reading.

@bnnm

This comment has been minimized.

Copy link
Collaborator

commented Aug 4, 2018

@Thealexbarney - I'll try to reproduce this and maybe compare with GCC, do you have some HPS that always triggers it? Doesn't look like HPS is doing more seeking that any other blocked files tho. Maybe related to decoding too fast/SSD drive/Windows version? Also foobar is doing its own read/seek, maybe could try to compare its wav writer.

@kode54 - I think they are buffered ok but it would always happen as it's reading/decoding the whole file I guess. Some blocked layouts or codecs can trash the buffers a lot, I'll see how many reads are being done.

@Thealexbarney

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2018

@bnnm I don't know that I've seen any HPS files that don't trigger it. 1p_qk.hps from SSBM triggers at ~5 seconds.

@bnnm

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2018

I'm having some building troubles but I get the bug in the appveyor MSVC build, and not in GCC (my own build). foobar also works, but there is one different byte vs GCC, towards the end. :S

I also get some byte differences w/ MSVC in other formats, but not as drastic as the hps. I'm not sure why some files aren't affected... Most curious, I'll try some more.

@bnnm

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2018

Ok, I compiled test.exe with VC2015 (not sure which sub-version) and it outputs the same exact file that GCC does. I take appveyor is using VC2018? Maybe some update could fix the bug?

WTF Microsoft, first this, now basic IO crap.

@kode54

This comment has been minimized.

Copy link
Collaborator

commented Aug 12, 2018

foobar2000 also does not use stdio, it uses its own IO replacement, so it can use its IO services for everything, which internally use Win32 IO directly.

@bnnm

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2018

Did some searching but not much info around (link, link).

Anyway, if it fixes MSVC's test/winamp builds I think it should be accepted. Reads are buffered so any performance penalty will be minimal.

It saddens me a bit the fix is black magic tho. Maybe the ifdef could be restricted later to _MSC_VER known to fail, as my MSVC builds work fine.

@kode54 kode54 merged commit 1294f51 into losnoco:master Aug 19, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.