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

Really fix album replaygain calculation with gstreamer backend. #2846

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

sdrik
Copy link
Contributor

@sdrik sdrik commented Mar 18, 2018

Fixes #2845

@sampsyo
Copy link
Member

sampsyo commented Mar 19, 2018

Interesting; thanks!! Can you please describe a little bit about why this seems to be the right fix?

Also, please add a quick changelog entry if you have a moment.

@sdrik
Copy link
Contributor Author

sdrik commented Mar 19, 2018

As I'm not a gstreamer expert this was almost trial and error, but the existing comment about "ensuring that the filesrc and decodebin elements received the paused state of the pipeline in a blocking manner" led me to the right direction.

What I understand is that even if the paused state is set in a blocking manner on the pipeline in _set_next_file, it is propagated asynchronously to its child elements (filesrc and decodebin), thus the sync_state_with_parent/get_state calls in _set_file.
But those synchronization calls were made after the unlink/set_state/set_property calls, leading to a race condition.
So my fix is simply to move the synchronization calls before any calls affecting filesrc and decodebin.
I also removed the added set_state(PLAYING) because it was obviously redundant with the one in _set_next_file.

@sampsyo
Copy link
Member

sampsyo commented Mar 20, 2018

Got it! That makes a lot of sense. Thank you for looking into it and finding this fix—trial-and-error is usually the way I sort things out with GStreamer too. ✨

@sampsyo sampsyo merged commit fee4dd1 into beetbox:master Mar 20, 2018
@sdrik sdrik deleted the issue-2845 branch March 20, 2018 19:24
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

Successfully merging this pull request may close these issues.

2 participants