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

audio: Do not handle buffering message. (Fixes #1722) #1734

Closed
wants to merge 1 commit into
base: develop
from

Conversation

3 participants
@kingosticks
Copy link
Member

kingosticks commented Jan 16, 2019

The on_buffering() handler was changing us out of GST_STATE_READY
and into GST_STATE_PAUSED before the pipeline had changed track.
This prevented the change from occuring.
According to https://bugzilla.gnome.org/show_bug.cgi?id=720836 GST
playbin should be handling the buffering itself.

audio: Do not handle buffering message. (Fixes #1722)
The on_buffering() handler was changing us out of GST_STATE_READY
and into GST_STATE_PAUSED before the pipeline had changed track.
This prevented the change from occuring.
According to https://bugzilla.gnome.org/show_bug.cgi?id=720836 GST
playbin should be handling the buffering itself.

@jodal jodal added this to the v2.2.3 milestone Jan 16, 2019

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Jan 29, 2019

Anyone have any issues/concerns with this going in?

@jodal

This comment has been minimized.

Copy link
Member

jodal commented Jan 29, 2019

👍 from me, but @adamcik used to be the GStreamer expert.

Are we confident enough in this to let it into 2.2.x instead of it having to wait for develop/3.x to be released?

@jodal

jodal approved these changes Jan 29, 2019

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Jan 29, 2019

I don't like how it seems to go against the gstreamer docs, it'd be nice to get the current official position. But it did fix the issues I had reproduced from users on discourse, and for @MHoyer12345678 too.

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Jan 29, 2019

OK, the word on the street (#gstreamer) is that we should still be handling our playbin buffering as per the docs. So I think we need a more stressful test case - a slow connection and a slow machine.

@kingosticks kingosticks assigned kingosticks and unassigned jodal Jan 29, 2019

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Jan 30, 2019

I've got an alternate version at kingosticks@6241201 which also seems to fix the particular radio streams I can intermittently reproduce the issue with. Unlike this one, the alt version does handle buffering messages. And it always has to do so just after we change track so it's much slower to change (like the current Mopidy behaviour). But maybe it's more robust. More testing on slow connections is needed I think.

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Jan 30, 2019

There's an example of Gstreamer's GstPlay bin handling this properly. And Rhythmbox. Considering these and what I was told in #gstreamer, I don't think we want this PR.

@kingosticks

This comment has been minimized.

Copy link
Member Author

kingosticks commented Jan 31, 2019

Closed in favour of #1740

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment