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

Make sure CallbackStream doesn't get destroyed before the Music #163

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

crumblingstatue
Copy link
Contributor

Music's destructor call's stop(), which in turn calls onSeek.
For CSFML, this requires the CallbackStream to stay alive until that point.

In C++, destructors for fields are called in reverse declaration order.
We swap the order of the fields, putting the Music after the
CallbackStream, so it gets destroyed before it.

Music's destructor call's stop(), which in turn calls onSeek.
For CSFML, this requires the CallbackStream to stay alive until that point.

In C++, destructors for fields are called in reverse declaration order.
We swap the order of the fields, putting the Music after the
CallbackStream, so it gets destroyed before it.
@crumblingstatue
Copy link
Contributor Author

See jeremyletang/rust-sfml#239 (comment) for context.

@crumblingstatue
Copy link
Contributor Author

Tomorrow I'll check other usages of CallbackStream to see if they satisfy this invariant.

@eXpl0it3r eXpl0it3r added the Bug label Jul 22, 2021
@crumblingstatue
Copy link
Contributor Author

I checked the other occurrences of CallbackStream. They all seem good, with one possible exception.
https://github.com/SFML/CSFML/blob/master/src/SFML/Graphics/FontStruct.h#L40
Why does sfFont even hold a CallbackStream?
createFromStream calls loadFromStream, not openFromStream, so why does it need to hold onto the CallbackStream?

@Kolsky
Copy link

Kolsky commented Jul 25, 2021

It's likely due to somewhat misleading naming. C++ API requires the stream to remain accessible until new font is loaded or is destroyed.
https://www.sfml-dev.org/documentation/2.5.1/classsf_1_1Font.php#abc3f37a354ce8b9a21f8eb93bd9fdafb

@eXpl0it3r
Copy link
Member

eXpl0it3r commented Aug 2, 2021

It's a bit misleading indeed, but the file/stream is kept open in order to reload new characters/sizes from the font.

@crumblingstatue do you think the FontStruct needs adjusting as well?

@eXpl0it3r eXpl0it3r merged commit 317f58f into SFML:master Aug 2, 2021
@crumblingstatue
Copy link
Contributor Author

@crumblingstatue do you think the FontStruct needs adjusting as well?

I'm not sure, I'll have to look into it tomorrow

@charleyah
Copy link

charleyah commented Jan 2, 2022

Just noticed this merge and wondered if its the cause of an issue I raised for SFML.Net some time ago. Disposing a Music instance would crash with STATUS_STACK_BUFFER_OVERRUN error and it was concluded to be a bug in CSFML?

@eXpl0it3r
Copy link
Member

Might be, can you reproduce it with and without this change?

@charleyah
Copy link

Just to cross my comments over. This has infact fixed the Music dispose bug in SFML.Net and is also reproducible on the current 2.5 Nuget version.

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

Successfully merging this pull request may close these issues.

4 participants