GNOME Bugzilla – Bug 601524
Slider jumps around at the end of songs
Last modified: 2012-07-07 07:19:59 UTC
Originally reported at: https://bugs.launchpad.net/ubuntu/+source/rhythmbox/+bug/474155 Steps to reproduce: 1) Start playing a file in a playlist with more than one song 2) Either wait until the end, or drag the slider to near the end of the song 3) Just before the next song begins, the slider jumps to 0, then back to the correct location, then back to 0 This is with default settings, and can be reproduced with multiple file types (.ogg and .flac). The sound is perfect however (my suspicion is that it is to do with queuing up the next song for gapless playback)
Created attachment 147467 [details] [review] Workaround until playbin2 is fixed The main issue is that the playbin2 gstreamer element doesn't actually tell you when it finishes one stream and begins the next. This patch works around that for the moment by assuming that when the stream position goes backwards, we have moved to the next stream. So, now instead of having the position slider jump around to different spots, it now either works correctly, or jumps to 0:00 of the next track in the time period where it would previously have been jumping around. The previous behavior had the position slider moving to the elapsed time for the current stream, but scaled according to the length of the next stream, so it was often somewhere in the middle. I've spoken to a gstreamer guy on irc, and there are plans to fix/enhance playbin2 in the future.
This is nice, but far too complicated. playbin2 will tell you when it stops one stream (you'll receive an EOS signal), and when it starts a new one (the pipeline state will change). Furthermore, there's no gapless playback in the rb-player-gst code, it's only in rb-player-gst-xfade.
The rb-player-gst code uses the playbin2 about-to-finish signal to do gapless playback, and when you do that, you don't get EOS or state change messages.
Wait, isn't the whole point of using playbin2 and setting the uri in the "about-to-finish" signal to get gapless playback? I know that playbin2 doesn't guarantee that there will be no gaps, but my understanding is that where possible it will do so (and on my computer, this seems to be possible everywhere as I don't get any gaps between files). You don't receive a EOS signal until playbin2 gets to the end of the uri it is currently playing AND the uri property is not set to something new. I agree that the patch is complicated, but it does remove the "fake_playing_position" hack and replace it with a hack which at least will be able to be removed cleanly once playbin2 can actually tell us when the new file starts. I don't believe the pipeline state does change, though I'm happy to be proved wrong. When I was trying to work out a decent fix, I was listening to signals all over the place and nothing happened when the stream finished - the only two useful times we get are when the stream is about to finish, and when playbin2 has stopped playing completely. At the moment, the UI goes to the next track when the "about-to-finish" signal is sent, which is also slightly broken. But in the long term, I believe you will need to track the stream data for both streams anyway (since at the end of a stream you do need to track both the currently playing stream and the stream which is cued up to play). Again, I'm happy to be proven wrong. Would a more acceptable solution be to remove the hack that checks whether the elapsed time has gone backwards? And use the previous behaviour of assuming that when the next tick_timeout() callback gets called after an "about-to-finish" signal that the current track has finished?
(In reply to comment #3) > The rb-player-gst code uses the playbin2 about-to-finish signal to do gapless > playback, and when you do that, you don't get EOS or state change messages. My mistake.
Could you file a GStreamer bug for playbin2 to get fixed, and let us know the number? I'd rather not we added support for this until it was fixed in GStreamer, though that's up to Jonathan whether or not it is accepted. I'd need the same time of support for Totem when we catch the about-to-finish signal.
Sounds like a decent plan. When I spoke to someone on irc (who I think was a gstreamer dev... now I think about it, I just assumed they were because they spoke authoritatively :)) they knew it was an issue and said it was non-trivial :). But I'll do the bug reporting, and I'll poke gstreamer a little to see if I can add it in myself. Though gstreamer has always seemed a bit impenetrable to me, so I don't know how that will work out.
Review of attachment 147467 [details] [review]: This looks like a reasonable short term approach to the problem (certainly better than the existing hack). Could do with a better commit message, though. The style I tend to use for rhythmbox is 'component: summary (bug #nnnnnn)' as a summary line, where component in this case would be 'playbin2', followed by an attempt at explaining what's being fixed and why. The existing comment message you've used doesn't specify which player backend it applies to, how it attempts to keep the time consistent, or what problem made it inconsistent. ::: backends/gstreamer/rb-player-gst.c @@ +93,3 @@ GDestroyNotify stream_data_destroy; + /* in the overlap between two streams, we need to track two stream_data objects */ don't really need this comment here @@ +147,3 @@ + * stream, the display will jump to the next track prematurely. + * working around this would be an uphill battle, and it would be + * better to just fix playbin2. */ this comment seems a little odd.. isn't what we're doing here working around the problem? what happens when playbin2 is fixed? @@ +689,3 @@ +static void +_get_next_stream_data (RBPlayerGst *mp) could just add this to _destroy_stream_data (and possibly rename it) @@ +792,3 @@ +static void +begin_stream_playback (RBPlayerGst *mp) This function doesn't actually begin stream playback, so this doesn't seem like a good name for it. @@ +799,3 @@ + mp->priv->buffering = FALSE; + mp->priv->playing = TRUE; + if (mp->priv->stream_data_next) { How are we going to get in here with stream_data_next == NULL? What does it mean if we do? I don't think this function should be dealing with that case. @@ +914,3 @@ } + else { + begin_stream_playback(mp); 'else' goes on the same line as the previous closing brace; one space between function name and arguments.
Thanks for your feedback: > this comment seems a little odd.. isn't what we're doing here working around the problem? Yes, it is just a workaround. Does the comment give any indication that that isn't the case? > what happens when playbin2 is fixed? Then there would be another patch to remove my workaround, which would involve deleting the sections of code with the big comments about them saying they are hacks, and listening for the theoretical "finish" signal, and calling the "begin_stream_playback" function (which will have been renamed). I'm happy to clean it all up according to your suggestions (thanks again), but if you would prefer to just wait for playbin2 then I'm ok with that too.
Without looking at this closer, a simple GstMessage posted on the bus by playbin2 at the time when it switches to the new file would be the correct solution, right? I.e. bug #584987. FYI I'm going to implement that in the next days, so if you have any additional requirements speak now :) For example, would you prefer a signal (emitted from the streaming thread that does the switching) instead of a message because you need to know that the track has changed immediately?
I don't think we need to know exactly when the track changes. If we were using a signal to do this, we'd probably end up just calling g_idle_add in the signal handler and handling the track change on the main thread anyway, so I don't think it would make any difference. Thanks for working on this.
The other thing which I believe is related is making sure the elapsed time and duration are always in sync with each other: https://bugzilla.gnome.org/show_bug.cgi?id=585969 I would imagine that doing this correctly will fix that bug too. (and thanks from me too)
commit c258166cef4aa61a59f78c142ea7140490126ce4 Author: Jonathan Matthew <jonathan@d14n.org> Date: Mon Dec 28 17:10:15 2009 +1000 playbin2: fix track changes (bug #601524, bug #602957) Rather than emitting the playing stream change signal immediately, and pretending the playback position is 0 until the next tick, we wait until either we get a playbin2-stream-changed signal or a position query returns a position inside the first second. This means the playback position should always be consistent, and the track change is reflected in the UI closer to when it actually occurs. This also means we don't emit the playing-stream signal on a streaming thread, which fixes some threading problems with a11y enabled.
*** Bug 607247 has been marked as a duplicate of this bug. ***