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

Fixed freezing while playing music from file objects #551

Merged
merged 4 commits into from Oct 9, 2018

Conversation

@dlon
Copy link
Member

@dlon dlon commented Oct 7, 2018

#548

The problem is primarily _pg_rw_read_th. Calling fileobj.read() (and possibly Py_DECREF) makes Python switch to the other thread and often causes the other thread to wait forever to acquire the mixer mutex.

My solution was to store the file descriptor and use this instead of the Python method, if it's available. Are there any drawbacks to this?

@@ -398,6 +402,17 @@ _pg_rw_seek(SDL_RWops *context, Sint64 offset, int whence)
Sint64 retval;
#endif

if (helper->fileno != -1) {
if (!(offset == 0 &&

This comment has been minimized.

@illume

illume Oct 9, 2018
Member

Is the logic here correct?

It seems lseek could be called twice here in the case where offset != 0 && whence != SEEK_CUR, and lseek doesn't return -1?

This comment has been minimized.

@dlon

dlon Oct 9, 2018
Author Member

True. It did do the right thing but that extra lseek was unnecessary.

@illume
Copy link
Member

@illume illume commented Oct 9, 2018

Thanks for this.

  • I wonder if we have a unit test for this already?
  • I guess this leave the bug there for file objects that don't have file descriptors.
if (retval == -1) {
return -1;
}
retval /= size;

This comment has been minimized.

@illume

illume Oct 9, 2018
Member

I guess it's possible that read could return less than a multiple of size. It's possible read could return 1 byte for example 4 times in a row. I'm not sure how to handle this case really, and it's also quite unlikely.

This comment has been minimized.

@dlon

dlon Oct 9, 2018
Author Member

Seems to be true of the other call as well, so it shouldn't be any worse at least.

@illume illume merged commit 13bd00f into pygame:master Oct 9, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@illume illume mentioned this pull request Oct 16, 2018
4 tasks done
@dlon dlon deleted the dlon:deadlock-fix branch Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants