After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 440952 - Support gapless playback
Support gapless playback
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: GStreamer
git master
Other Linux
: Normal enhancement
: 2.x
Assigned To: Banshee Maintainers
Banshee Maintainers
: 443518 596940 (view as bug list)
Depends on: 579394 584987 585002 602000 602437 679544
Blocks: 577577 583933 587643
 
 
Reported: 2007-05-24 14:51 UTC by Sebastian Dröge (slomo)
Modified: 2012-07-07 07:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Rough, semi-working patch for the bzr challenged. (15.96 KB, patch)
2009-03-17 22:32 UTC, Christopher Halse Rogers
none Details | Review
Cleaner, better, still broken preliminary patch for the bzr challenged (18.17 KB, patch)
2009-03-19 07:49 UTC, Christopher Halse Rogers
none Details | Review
Another patch revision (29.43 KB, patch)
2009-04-07 09:10 UTC, Christopher Halse Rogers
none Details | Review
Patch that applies cleanly to trunk (28.29 KB, patch)
2009-04-07 20:44 UTC, Christopher Halse Rogers
none Details | Review
New patch revision (32.58 KB, patch)
2009-05-03 05:51 UTC, Christopher Halse Rogers
none Details | Review
Tabless patch (31.51 KB, patch)
2009-05-10 10:52 UTC, Christopher Halse Rogers
needs-work Details | Review
Updated patch (31.52 KB, patch)
2009-06-06 13:00 UTC, Christopher Halse Rogers
none Details | Review
Set buffer-length on audiosink (6.90 KB, patch)
2009-06-16 09:53 UTC, Christopher Halse Rogers
reviewed Details | Review
Cleaner gapless implementation (35.41 KB, patch)
2009-06-16 10:17 UTC, Christopher Halse Rogers
reviewed Details | Review
Vestigial documentation (2.60 KB, patch)
2009-06-16 10:23 UTC, Christopher Halse Rogers
none Details | Review
Fix playcount updating (2.37 KB, patch)
2009-07-01 09:05 UTC, Christopher Halse Rogers
none Details | Review
Gapless patch updated to apply to master. (39.49 KB, patch)
2009-09-29 07:57 UTC, Christopher Halse Rogers
none Details | Review
Gapless patch, really updated for trunk (39.69 KB, patch)
2009-09-29 09:14 UTC, Christopher Halse Rogers
none Details | Review
Revised patch (41.93 KB, patch)
2009-10-07 06:55 UTC, Christopher Halse Rogers
none Details | Review
Next patch iteration (42.36 KB, patch)
2009-11-03 06:54 UTC, Christopher Halse Rogers
needs-work Details | Review
Patch, r4830 (43.09 KB, patch)
2009-11-11 02:12 UTC, Christopher Halse Rogers
none Details | Review
Patch, r4981 (42.34 KB, patch)
2009-11-16 04:39 UTC, Christopher Halse Rogers
committed Details | Review
Total gapless diff (58.70 KB, patch)
2010-03-09 01:37 UTC, Christopher Halse Rogers
none Details | Review

Description Sebastian Dröge (slomo) 2007-05-24 14:51:39 UTC
Hi,
banshee currently does not support gapless playback. It's nearly gapless but still there is a small gap between song changes. To prevent this one could already prepare a "src ! decodebin" pipeline with the new song which then will be added immediately to the sink instead of the old one.

The gap currently present is caused by the time needed for getting enough data decoded to play it.

Rhythmbox implement such playback mode in 0.10.90 together with a cross-fading mode (which is maybe also a good idea?)

Bye
Comment 1 Aaron Bockover 2007-06-03 20:51:28 UTC
*** Bug 443518 has been marked as a duplicate of this bug. ***
Comment 2 Sebastian Dröge (slomo) 2008-02-11 11:21:14 UTC
gst-plugins-base 0.10.17 now has the playbin2 element. This allows (or will allow) this features automagically: simply set a new file uri to play while the element is in PLAYING or PAUSED state and it will continue to that file on EOS.
Switching immediately to a new file requires setting the state to READY, setting a new uri (or the other way around) and setting to PLAYING again.
Comment 3 Stanislas 2008-06-01 09:54:35 UTC
Hi,

I'm using Ubuntu Hardy and gst-plugins-base 0.10.18 is installed but there is still a gap between tracks in banshee and even if the gap is very reduced in rhythmbox, there is still one.

Stan
Comment 4 Sebastian Dröge (slomo) 2008-06-02 07:49:00 UTC
Banshee still needs to use playbin2 to get the gapless support.
Comment 5 Gabriel Burt 2008-07-25 15:03:40 UTC
One thing to note, some formats have gaps in the actual stream, so gapless playback is not doable.  Except of course there are hacks (by LAME and iTunes among others) to work around this.  Ideally we would read their tags/metadata to get real gapless playback.  See http://en.wikipedia.org/wiki/Gapless_playback for more info.
Comment 6 Andrew Conkling 2008-07-25 17:13:39 UTC
True. Could we at least get support for free formats? This is one of the functional reasons I avoid MP3.
Comment 7 owen-bugs 2008-11-06 16:47:59 UTC
is anyone working on this?  This is why I don't use Banshee -- all of my albums are mixed together.
Comment 8 Nicholas Doyle 2009-02-09 00:58:30 UTC
It would be nice to see banshee use this 'playbin2' element even if the support for files with a gap at the end is not there yet. 90%+ of my music is FLAC which doesn't have gaps due to the format; it seems using playbin2 would at least get this working for us FLAC users. Not having gapless playback is one of my biggest annoyances right now (second to the iPod artwork DB size problems) and this seems like such a trivial fix to at least get this functionality for some people.
Comment 9 Andrew Conkling 2009-03-02 18:18:07 UTC
(In reply to comment #8)
> this seems like such a trivial fix to at least get this functionality for
> some people.

Take a look at bug 130426 to see how very untrivial it was. It would certainly be easier with the playbin2 element, but I wouldn't be surprised if it were still a difficult change.
Comment 10 Christopher Halse Rogers 2009-03-08 23:58:21 UTC
I've had a bit of a look at playbin2, and it _seems_ to be as simple as hooking in to the about-to-change signal, setting the uri property to the next song in that callback, and not cycling-to-NULL on track change.

Preliminary testing suggests that playbin2 is a drop-in replacement for playbin - making bp_construct_pipeline use playbin2 rather than playbin appears to work as normal.

Actually hooking up gapless requires a bit more work than my initial "make about-to-change fire the eos callback"; the existing backend seems to try to set the pipeline to NULL in quite a number of places, and just commenting as many Close() calls out as I can find breaks playing the next track :).

I think it's necessary to expose the about-to-change callback to the managed code - overloading the eos callback implies strange behaviour on track-change: the UI will show that the track has changed before the audio for the new track kicks in.
Comment 11 Christopher Halse Rogers 2009-03-17 22:31:36 UTC
So, I've got a bit of a proof-of-concept branch up at 
https://code.edge.launchpad.net/~raof/banshee/banshee-gapless-work

It works, at least as far as gapless playback is concerned.  The last commit is full of hacks to try and hook up the infrastructure - playbin2 doesn't send EoS when doing a gapless transition, so there needs to be some work in either synthesising an EoS or handling song transitions a bit differently.  These hacks break track-advancing somewhat in PlaybackControllerService.

Monodevelop seems to have trouble understanding "replace tabs with spaces", so there's a lot of tabs in here at the moment; I'll strip them out once, at the end, before submitting a final patch.

For those who don't use bzr as their svn client of choice, I'll attach a patch against trunk.

I'd like some comments, and have some discussion about precisely how to handle track changing.
Comment 12 Christopher Halse Rogers 2009-03-17 22:32:23 UTC
Created attachment 130860 [details] [review]
Rough, semi-working patch for the bzr challenged.
Comment 13 Nicholas Doyle 2009-03-18 00:42:58 UTC
I tried out the patch and got puked on when I paused my music.


An unhandled exception was thrown: Object reference not set to an instance of an object

at Banshee.PlaybackController.PlaybackControllerService.OnPlayerEvent (Banshee.MediaEngine.PlayerEventArgs) <0x003be>
at Banshee.MediaEngine.PlayerEngineService.RaiseEvent (Banshee.MediaEngine.PlayerEventArgs) <0x0012f>
at Banshee.MediaEngine.PlayerEngineService.OnEngineEventChanged (Banshee.MediaEngine.PlayerEventArgs) <0x000cb>
at Banshee.MediaEngine.PlayerEngine.RaiseEventChanged (Banshee.MediaEngine.PlayerEventArgs) <0x0002f>
at Banshee.MediaEngine.PlayerEngine.OnEventChanged (Banshee.MediaEngine.PlayerEventArgs) <0x00047>
at Banshee.MediaEngine.PlayerEngine.OnStateChanged (Banshee.MediaEngine.PlayerState) <0x00148>
at Banshee.GStreamer.PlayerEngine.OnStateChange (intptr,Banshee.GStreamer.GstState,Banshee.GStreamer.GstState,Banshee.GStreamer.GstState) <0x000bb>
at (wrapper native-to-managed) Banshee.GStreamer.PlayerEngine.OnStateChange (intptr,Banshee.GStreamer.GstState,Banshee.GStreamer.GstState,Banshee.GStreamer.GstState) <0x00067>
at (wrapper managed-to-native) Gtk.Application.gtk_main () <0x0005c>
at Gtk.Application.Run () <0x0000b>
at Banshee.Gui.GtkBaseClient.Run () <0x00047>
at Banshee.Gui.GtkBaseClient.Startup () <0x00046>
at Hyena.Gui.CleanRoomStartup.Startup (Hyena.Gui.CleanRoomStartup/StartupInvocationHandler) <0x0009b>
Comment 14 Christopher Halse Rogers 2009-03-18 02:07:43 UTC
Yeah, it'll do that :).  As I say, the final set of hacks break the PlaybackControllerService.  You should get working gapless playback as long as you don't touch anything playback related!
Comment 15 Bertrand Lorentz 2009-03-18 20:26:56 UTC
As a side note : I see you've added xml comments in PlayerEngineService.cs.
Banshee currently doesn't have those comments in the source files.
Code documentation should go into the xml files under docs/, probably docs/Banshee/en/Banshee.MediaEngine/PlayerEngineService.xml in your case.
Comment 16 Christopher Halse Rogers 2009-03-19 07:49:32 UTC
Created attachment 130941 [details] [review]
Cleaner, better, still broken preliminary patch for the bzr challenged

Here's a somewhat better patch.  As long as you don't use the play queue, it works fine for me.  I'm now moderately interested in places where it crashes/doesn't work.

I think the general approach of synthesising StartOfStream messages on track change is reasonable (there's undoubtedly a better way to detect the track change than in this patch, though), but I'm still eager to hear from those better versed in Banshee internals as to their opinions!
Comment 17 Christopher Halse Rogers 2009-04-07 09:10:33 UTC
Created attachment 132249 [details] [review]
Another patch revision

Those following the bzr branch on Launchpad will know that there's been slow and somewhat unsteady work in there.  Here's another patch.  This one is pretty much feature-complete - everything works, mostly, barring some annoying heisenbugs.
Comment 18 Christopher Halse Rogers 2009-04-07 20:44:40 UTC
Created attachment 132294 [details] [review]
Patch that applies cleanly to trunk
Comment 19 Christopher Halse Rogers 2009-05-03 05:51:04 UTC
Created attachment 133845 [details] [review]
New patch revision

Here's a new revision.  This works pretty well, as long as you're using a pre-release of playbin2 - 0.10.22.2 is good, and as long as you don't want wavpack files to work correctly.

I think the core here is solid now - I think any playback bugs are as likely to be playbin2 bugs as bugs in this code.  Feel free to test this out.

This now has a preference to enable/disable gapless playback.  It's close to something trunk-worthy, I think, although I still need to go back and strip out the rest of the tabs.
Comment 20 Christopher Halse Rogers 2009-05-10 10:52:38 UTC
Created attachment 134350 [details] [review]
Tabless patch

In the interests of being more trunk-friendly, here's the patch updated to conform to Banshee coding standards.

It still needs a review, and will probably need some fixing after review, but I'm using this now.
Comment 21 Bertrand Lorentz 2009-05-10 15:45:28 UTC
Thanks for all your work on this, Christopher !

A few comments :
1. The .bzrignore file should not be in the patch

2. There are a few tabs remaining :
+		private uint last_position;
+		private bool next_track_set = false;

3. Compilation with mono 2.0.1 fails with the following error
Compiling Banshee.GStreamer.dll...
./Banshee.GStreamer/PlayerEngine.cs(85,30): error CS0169: The private field `Banshee.GStreamer.PlayerEngine.last_position' is never used
Compilation failed: 1 error(s), 0 warnings

After removing the offending line, I was able to compile and run with the patch. Gapless seems to work as expected, but I noticed a strange behavior of the position slider :
During the transition between two tracks, the slider displays the length of the track that is beginning and the position in the track that is ending. This only happens for a split second, and is visible when transitioning from a shorter track to a longer track.
This is with gstreamer 0.10.22.
Comment 22 Christopher Halse Rogers 2009-05-11 00:35:48 UTC
3) Hm.  I'm building with mono 2.0.1.  I guess I don't have Werror set.  That's a holdover from a previous experiment I thought I'd removed.

The track transition craziness is due to the way the patch handles next track detection.  When doing gapless playback, playbin2 is (currently[1]) unable to provide any signal for when the playing track changes - it doesn't fire an EoS or any other such bus message or signal.  The last_position variable was left over from a previous experiment where I compared last_position to the current position in the iterate callback, to detect timestamp discontinuities (which are visible on track change) and then fire the StartOfStream.

I thought that was quite messy, and instead hooked up the StartOfStream firing to playbin2's about-to-finish callback, which (on linux) fires ~200ms before the end of the current track, which gives the brief period where the positions come from the current track and the length/trackinfo comes from the next track.

This should be cleaned up, but I'm not sure what the best method is, or whether we can wait for playbin2 to make this easy.


[1] I asked in #gstreamer about this, and sparked a bit of discussion between some playbin2 hackers about how it could actually do this, the upshot of which was that it'd be technically possible get playbin2 to generate an appropriate message/signal.  I need to follow up this, either link to an existing bug or file an new enhancement bug.
Comment 23 Christopher Halse Rogers 2009-06-06 13:00:33 UTC
Created attachment 136053 [details] [review]
Updated patch

Here's a revision with the final tabs gone, no .bzrignore, and has the unused variable removed so builds with -Werror.

Bug 584987 submitted against playbin2 for a track-changed message or signal.

The strange position/duration behaviour on track change you describe is apparently a limitation of playbin2 - the duration is set to that of the new track is updated before the hardware has finished playing.  Thus we get the duration for the new song, but the position is still in the old.

So, things I know should be fixed would be:
a) Work around the position on track change problem.  This looks like it would be messy, and better fixed in playbin2.
b) Remove the awful polling code in PlayerEngine.OnIterate.  Rather than checking each iteration whether we're 5 seconds from the end, it would be nice to have the about-to-finish signal work.  Currently it doesn't - there's a very short deadline for setting the next uri after about-to-finish gets called, and in my testing hooking this up to managed code failed to respond in time, most of the time.  This deadline can apparently be extended by setting a bigger buffer on the audiosink.

I'm likely to continue to be busy over the next couple of weeks, so the time I have available to work on this will remain limited.  If someone else wants to pick up the patch and run with it, be my guest.
Comment 24 Sebastian Dröge (slomo) 2009-06-06 15:34:22 UTC
(In reply to comment #23)

> b) Remove the awful polling code in PlayerEngine.OnIterate.  Rather than
> checking each iteration whether we're 5 seconds from the end, it would be nice
> to have the about-to-finish signal work.  Currently it doesn't - there's a very
> short deadline for setting the next uri after about-to-finish gets called, and
> in my testing hooking this up to managed code failed to respond in time, most
> of the time.  This deadline can apparently be extended by setting a bigger
> buffer on the audiosink.

This should probably be fixed in playbin2 too by blocking the track switching until the about-to-finish callbacks have returned (if any). See bug #585002
Comment 25 Gabriel Burt 2009-06-09 22:04:41 UTC
Sebastian, do you know of any negative impacts Banshee switching to playbin2 might have?
Comment 26 owen-bugs 2009-06-09 22:56:56 UTC
I can't speak with absolute authority, but in my own experience with playbin2 I've had issues with occasional crashes on song changes.  It was happening often enough (10% of the time) that I decided not to use it.
Comment 27 Sebastian Dröge (slomo) 2009-06-10 05:19:13 UTC
(In reply to comment #25)
> Sebastian, do you know of any negative impacts Banshee switching to playbin2
> might have?

In my experience the switch to playbin2 in totem only had advantages... but requires the latest gstreamer core and plugins-base releases.

If you want I can give you a large list of improvements over playbin1 ;)
Comment 28 Christopher Halse Rogers 2009-06-16 09:53:44 UTC
Created attachment 136705 [details] [review]
Set buffer-length on audiosink

Here's the first of a series of patches which address (b) above.  This one sets adds a method to libbanshee to set the buffer length of the audiosink, which turns out to be non-trivial.
Comment 29 Christopher Halse Rogers 2009-06-16 10:17:16 UTC
Created attachment 136710 [details] [review]
Cleaner gapless implementation

Here's a cleaner implementation of gapless playback.  The obnoxious polling in OnIterate is replaced by actually using the about-to-finish signal correctly - the signal handler needs to block until the URI is set.  The managed OnAboutToFinish handler does this.
Comment 30 Christopher Halse Rogers 2009-06-16 10:23:45 UTC
Created attachment 136712 [details] [review]
Vestigial documentation

And here, because I wrote it earlier, is a tiny amount of documentation.
Comment 31 Christopher Halse Rogers 2009-06-16 11:13:42 UTC
Filed bug 585969 for the duration/position strangeness during track change.
Comment 32 Gabriel Burt 2009-06-16 16:54:05 UTC
(In reply to comment #28)
> Created an attachment (id=136705) [edit]
> Set buffer-length on audiosink
> 
> Here's the first of a series of patches which address (b) above.  This one sets
> adds a method to libbanshee to set the buffer length of the audiosink, which
> turns out to be non-trivial. 

In the patch you comment

// This increases the deadline for setting the URI after about-to-finish is triggered

But in bug #585002 wasn't it determined that there is no deadline; that the about-to-finish handlers run synchronously?
Comment 33 Gabriel Burt 2009-06-16 17:07:14 UTC
(In reply to comment #29)
> Created an attachment (id=136710) [edit]
> Cleaner gapless implementation
> 
> Here's a cleaner implementation of gapless playback.  The obnoxious polling in
> OnIterate is replaced by actually using the about-to-finish signal correctly -
> the signal handler needs to block until the URI is set.  The managed
> OnAboutToFinish handler does this.

This looks pretty good, I don't see anything obviously wrong.  Will require a lot of testing, though :)
Comment 34 Christopher Halse Rogers 2009-06-17 00:06:24 UTC
(In reply to comment #32)
> (In reply to comment #28)
> > Created an attachment (id=136705) [edit]
> > Set buffer-length on audiosink
> > 
> > Here's the first of a series of patches which address (b) above.  This one sets
> > adds a method to libbanshee to set the buffer length of the audiosink, which
> > turns out to be non-trivial. 
> 
> In the patch you comment
> 
> // This increases the deadline for setting the URI after about-to-finish is
> triggered
> 
> But in bug #585002 wasn't it determined that there is no deadline; that the
> about-to-finish handlers run synchronously?
> 

There is still a deadline - the length of the audio buffer + some time for the next pipeline to start decoding.  But you're right, we should be OK with the standard 200ms audio buffer.  This patch is included here partially because it was surprisingly difficult to do :).

If you'd prefer to not apply this patch, the only only part strictly necessary for the gapless implementation is the BUFFERLEN_IN_NANOSEC #define in banshee-private.h
Comment 35 Christopher Halse Rogers 2009-07-01 09:05:21 UTC
Created attachment 137673 [details] [review]
Fix playcount updating

Here's an extra patch to be applied on top of the others; this fixes the playcount/last-played updating that I noticed because "shuffle-by" wasn't working right.

That said, this has highlighted for me why I've found this line of development a bit unsatisfying.  I've tried to add gapless without really changing the non-gapless codepath, and this has resulted in me fighting the PlayerEvent system a bit.  I'll outline the current patch status, and how I think I'd like to fix it:
*****************
Currently...
A PlayerEngine has two different ways of signalling "I need a new track" - PlayerEvent.EndOfStream or PlayerEvent.BufferNextTrack.  The PlaybackControllerService handles these two requests differently, by calling either player.Open() or player.SetNextTrack().  In the case of PlayerEvent.BufferNextTrack, the PlaybackControllerService never sees a PlayerEvent.EndOfStream.

********************
What I'd like to see:
I'd like to separate the "I need a new track" signal from "I've finished this track" signal.  This would mean adding a PlayerEvent.RequestNextTrack signal to all PlayerEngines, and disconnecting the next track handling from EndOfStream handling.  
The gapless PlayerEngine would then synthesise EndOfStream (as well as the StartOfStream events it currently synthesises) events to signal to the PlaybackController that playback of a track has finished.
Currently, the BufferNextTrack signal isn't a terrible way of signalling that a track has finished.  However, it seems reasonable to assume that someone will want a crossfading backend at some point, and then BufferNextTrack will need to be raised significantly before the end of a track.
Comment 36 Gabriel Burt 2009-07-01 14:30:45 UTC
Makes sense to me.  Thanks for digging in on this!
Comment 37 Christopher Halse Rogers 2009-07-20 05:07:01 UTC
http://gitorious.org/~raof/banshee/gapless-work contains a git tree with gapless re-implemented in the cleaner way I outlined above.

I'm not totally wild about some of the variable naming, and it needs some docs (particularly that a PlayerEngine can expect SetNextTrack to be called once they've fired a RequestNextTrack, possibly with a null track), but I feel this is cleaner, and works for me.
Comment 38 Christopher Halse Rogers 2009-09-29 07:57:07 UTC
Created attachment 144244 [details] [review]
Gapless patch updated to apply to master.

Now that I've got a moment to think again, here's a patch updated for the changes to trunk over the last 5 weeks of mayhem.

It obviously needs lots of testing, but it seems to Work For Me(tm). :)
Comment 39 Christopher Halse Rogers 2009-09-29 09:14:05 UTC
Created attachment 144245 [details] [review]
Gapless patch, really updated for trunk

...and when I say "Works for me", I mean, of course, that *this* patch works for me.  URGH!
Comment 40 Michael Martin-Smucker 2009-10-01 12:59:37 UTC
*** Bug 596940 has been marked as a duplicate of this bug. ***
Comment 41 Aaron Bockover 2009-10-01 15:50:09 UTC
This is a sweet patch! I've given it the once-over review and it looks pretty solid. A few things stand out to me on this first pass:

a) I have yet to actually test or look in real depth at this code to get a deeper understanding, but I am curious as to why you are using an EventWaitHandle and not the main loop

b) In the GStreamer PlayerEngine.cs you have the following, which will break on Windows (should be libbanshee.dll):

+        [DllImport ("libbanshee")]
+        private static extern void bp_set_next_track_starting_callback ...

c) The GaplessEnabled field in the GStreamer PlayerEngine.cs should be gapless_enabled, since it's a field and not a property

d) PlayerEvent.RequestNexttrack should be PlayerEvent.RequestNextTrack

e) I'd prefer to remove the EXPERIMENTAL tag out of the preferences UI strings, otherwise translators will have to fix this later. If you prefer, I'm fine with doing something like Catalog.GetString ("Foo") + " " + Catalog.GetString ("(Experimental)")

f) Please use // for comments not /**/

Of course the player engine is a hugely sensitive and important area - hopefully this concern is unfounded now, but the last time I tried porting to playbin2, it worked well for audio, but fell completely on its face with video. 

This was back at GUADEC in 2008, so I'm hoping the issues here have just gone away by now. Have you done much testing with video? It's definitely time we port to playbin2, but I would like to be exceedingly careful and test, test, test.

Finally, as an aside, I have been personally wanting to drop the engine abstraction. It looks like you went through a bit of pain here with that - sorry :) However, it's not something that we can just up and drop right now, and as such it remains a critical path that also needs lots of testing (even just from a single-engine perspective).

Thanks for your hard work in this area, I am eager to get it into master soon provided minimal issues with playbin2 at heart. If there are issues, it's time to start a playbin2 branch and work through them.
Comment 42 Aaron Bockover 2009-10-01 15:50:47 UTC
Oh my, I just realized you have a patch with API documentation as well. You are a saint.
Comment 43 Wouter Bolsterlee (uws) 2009-10-01 18:48:01 UTC
(In reply to comment #41)
> e) I'd prefer to remove the EXPERIMENTAL tag out of the preferences UI strings,
> otherwise translators will have to fix this later. If you prefer, I'm fine with
> doing something like Catalog.GetString ("Foo") + " " + Catalog.GetString
> ("(Experimental)")

It's good to see that you even keep translators in mind when handling with experimental patches. However, your suggestion is a horribly broken approach from an i18n point of view. A much more acceptable way to handle this would be the C# equivalent of this code:

  sprintf(_("%s (experimental)"), _("Foo"));

... so that the strings "Foo" and "experimental" can be translated individually, while the translator still has some control over word order or word forms.
Comment 44 Christopher Halse Rogers 2009-10-06 11:35:20 UTC
I've fixed b), c), d) and f) locally.  I'll remove the "EXPERIMENTAL" warning from the preference, too.

For (a) - I'm using an EventWaitHandle because it was the first solution that sprang to mind.  The problem is: the about-to-finish callback gets called from the streaming thread of the playbin and this won't be Banshee's main thread.  Furthermore, the playbin's uri property *must* be set (via SetNextTrack) before the callback returns.  Since we're not on the main thread, and the event system delegates everything to the main thread, we need to use some form of thread synchronisation to block the callback until the event has propagated and SetNextTrack has been called.

Is there a solution I've overlooked here?

I'll also try to update that API documentation patch, too :).

Finally, I've now tried it with some video.  While I seem to remember it working, it certainly doesn't work now - gstreamer deadlocks in the first g_object_get call in bp_find_xoverlay.  I've had a look at how Totem uses playbin2; it obviously works fine with video, but it does things differently enough that it's not obvious what banshee does to trigger this.
Comment 45 Christopher Halse Rogers 2009-10-07 06:55:29 UTC
Created attachment 144929 [details] [review]
Revised patch

This patch should address the review comments.
Additionally, it works around the playbin2 deadlock when trying to play video; everything now works as far as I can see.

I'm not sure whether the deadlock is a playbin2 bug or not.  I need to ask in #gstreamer.
Comment 46 Christopher Halse Rogers 2009-10-07 06:58:07 UTC
Additionally, there's the gitorious branch http://gitorious.org/~raof/banshee/gapless-work if you'd like to check out some of the development history.
Comment 47 Christopher Halse Rogers 2009-10-08 08:13:09 UTC
Aha!  The playbin2 deadlock on video is almost certainly bug #583255, which is fixed in gst/gst-plugins-base 0.10.25.
Comment 48 mannheim89 2009-10-12 14:16:14 UTC
I've been using the gitorious gapless-work branch (for which many thanks). I experienced the following behavior: banshee behaves as if "Repeat" is on. For example:

1. Launch Banshee, with gapless playback enabled, and "Repeat Off".
2. Select an album.
3. Select and play the last track of the album.
4. Wait for the last track to finish.

Expected behavior: banshee stops at the end of the last track.
Actual behavior: banshee goes back to the first track of the album and plays it.

This behavior doesn't occur with the current git trunk (as checked out today), so perhaps it has been introduced by the patch. There are other similar situations where the patched version seems a bit over-zealous about finding and playing a "next track".
Comment 49 mannheim89 2009-10-12 17:28:25 UTC
Sorry to add to my previous comment, but I can confirm that the behavior described there is introduced by applying the current version of the patch directly to the trunk.
Comment 50 Christopher Halse Rogers 2009-10-14 10:30:53 UTC
Ah, yes.  I see where that is.

That's because the gapless engine fires *two* RequestNextTrack signals in this case - once when the about-to-finish callback is raised, and then a second one when the playbin signals EOS.  I made that a deliberate decision - if a playback controller can't supply a new track while the engine is playing & about-to-finish is called then it gets a second try once playback has finished.

I'm not sure whether there is any PlaybackController that would need this behaviour, but I think it's reasonable.  With a possible future cross-fading backend the first RequestNextTrack signal could be fired arbitrarily early; 30 seconds before the actual stream ends would not be tremendously unusual.  It wouldn't surprise me to find some PlaybackControllers couldn't handle submitting a new URI that far before the current one ends.

I'll need to work out how to make this work nicely.
Comment 51 Christopher Halse Rogers 2009-11-03 06:54:47 UTC
Created attachment 146798 [details] [review]
Next patch iteration

This update fixes the "continues playing after reaching the end of the album" problem by only firing RequestNextTrack once.  Perhaps this will need to be revisited, but I haven't found a PlaybackController so far that won't support this (I can't test with last.fm).

It also properly calls SetNextTrack(null) in response to RequestNextTrack when there isn't a next track to be set.  In turn, this makes it possible for the unmanaged code to not fire a next-track-starting event in this case, so the engine no longer fires an incorrect StartOfStream message.

I'll get around to finishing up the documentation at some point, too :)
Comment 52 Gabriel Burt 2009-11-10 07:46:28 UTC
Review of attachment 146798 [details] [review]:

::: libbanshee/banshee-player.c
@@ +202,3 @@
 {
+    g_return_if_fail (IS_BANSHEE_PLAYER (player));
+    if (player->timeout_id != 0) {

It might be good to rename timeout_id to something more specific and indicative of what the timeout is for.

::: src/Backends/Banshee.GStreamer/Banshee.GStreamer/PlayerEngine.cs
@@ +635,3 @@
             ));
+            gapless_preference = service["general"]["misc"].Add (new SchemaPreference<bool> (GaplessEnabledSchema,
+                String.Format (Catalog.GetString ("{0} (EXPERIMENTAL)"), Catalog.GetString ("Enable _gapless playback")),

Instead of creating your own SchemaPreference, you should be able to just pass the GapplessEnabledSchema, and the Section class will create the SchemaPreference from it - avoids duplicating these strigs with below.

@@ +663,3 @@
+        public static readonly SchemaEntry<bool> GaplessEnabledSchema = new SchemaEntry<bool> (
+            "player_engine", "gapless_playback_enabled",
+            false,

All other things (stability, mostly) equal, this should be enabled by default, no?

@@ +664,3 @@
+            "player_engine", "gapless_playback_enabled",
+            false,
+            "Enable gapless playback (EXPERIMENTAL)",

Add Catalog.GetString around this.

Let's remove this EXPERIMENTAL string unless you have large reservations about this feature's stability.

@@ +665,3 @@
+            false,
+            "Enable gapless playback (EXPERIMENTAL)",
+            "Eliminate the small playback gap on track change.  Useful for concept albums & classical music.  EXPERIMENTAL"

Catalog.Get String again.

& => and

::: src/Core/Banshee.Services/Banshee.MediaEngine/PlayerEngineService.cs
@@ +311,3 @@
+        public void SetNextTrack (TrackInfo track)
+        {
+            if (track != null && active_engine != FindSupportingEngine (track.Uri)) {

This method shared 90% of its body with the following SetNextTrack method; share that code.

::: src/Core/Banshee.Services/Banshee.PlaybackController/PlaybackControllerService.cs
@@ +230,3 @@
+        }
+        
+        public void Next (bool restart, bool userRequested)

I probably missed it, but is userRequested ever false with the current code base + this patch?
Comment 53 mannheim89 2009-11-10 15:58:10 UTC
I encountered another apparent glitch when using this patch.

I have one album which consists of AIFF files. (The sound data inside is uncompressed,  88.2 kHz sampled, 24 bit stereo.) When gapless is disabled, these files play fine (albeit with a gap between tracks). When gapless playback is enabled, there is a problem at the change of tracks:

The first track plays fine. At the end of the first track, the second track loads up, but doesn't play correctly. What appears to happen is that banshee silently fast-forwards through the track (the widget which shows elapsed time in the track moves along swiftly, with no sound output). Sometimes the fast-forwarding stops at a point somewhere around 50 seconds into the second track, at which point play resumes normally. Sometimes, the fast-forwarding continues right through the second track (and maybe the third) before settling down to normal play somewhere in the third or fourth track.

I understand that AIFF files are not first-class citizens in the gstreamer world (e.g. their tags don't show up in banshee). The same files play fine (gapless) when converted to FLAC files (still at 88.2 kHz times 24 bits per channel).
Comment 54 Gabriel Burt 2009-11-10 17:45:40 UTC
(In reply to comment #53)
> I understand that AIFF files are not first-class citizens in the gstreamer
> world (e.g. their tags don't show up in banshee).

Actually, Banshee mostly uses TagLib# for reading/writing metadata (exception: streamed media).  Feel free to file an enhancement request bug against it.
Comment 55 Christopher Halse Rogers 2009-11-10 22:00:11 UTC
(In reply to comment #53)
> I encountered another apparent glitch when using this patch.
> 
> I have one album which consists of AIFF files. (The sound data inside is
> uncompressed,  88.2 kHz sampled, 24 bit stereo.) When gapless is disabled,
> these files play fine (albeit with a gap between tracks). When gapless playback
> is enabled, there is a problem at the change of tracks:
> 

I'd guess this is a playbin2 problem.  If you want to check it out, "test7" in gst-plugins-base/gst/playback is a minimal gapless-playback testcase.  You'd call it with "./test7 file:///path/to/first/AIFF file:///path/to/second/AIFF".
Comment 56 Christopher Halse Rogers 2009-11-11 02:12:07 UTC
Created attachment 147439 [details] [review]
Patch, r4830

Thanks for the review.

1) I've changed player->timeout_id to player->next_track_starting_timer_id.  That should describe what it does, but may be a bit long :)

2) I don't think I *can* just use the GaplessEnabledSchema.  Firstly, the strings aren't the same - the gapless_preference short desc has a '_' in it for a keyboard shortcut, which the gconf schema doesn't want.  Secondly, passing in GaplessEnabledSchema wouldn't let me set the delegate.

3) Gapless playback is now enabled by default.  That's probably the right call.

4) I don't think I can or should wrap the GaplessEnabledSchema strings with Catalog.GetString.  Won't that break the schema translations?  It looks like the gconf schemas are generated by extracting SchemaEntry<>s by reflection, and then the schema strings are translated separately.

5) I've dropped the 'EXPERIMENTAL' from the preferences strings

6) & => and

7) I've factored out an "EnsureActiveEngineCanPlay" method from the two PlayerEngineService.SetNextTrack overloads.

8) Yes, you did miss it: userRequested is set to false in the PlaybackControllerService.RequestNextTrackHandler call.

I've also pushed these fixes to the gitorious repository.
Comment 57 mannheim89 2009-11-11 14:24:24 UTC
(In reply to comment #55)
> 
> I'd guess this is a playbin2 problem.  If you want to check it out, "test7" in
> gst-plugins-base/gst/playback is a minimal gapless-playback testcase.  You'd
> call it with "./test7 file:///path/to/first/AIFF file:///path/to/second/AIFF".

Thanks; and yes, indeed: test7 exhibited exactly the same problem with two or more AIFF files.
Comment 58 Christopher Halse Rogers 2009-11-16 04:39:53 UTC
Created attachment 147845 [details] [review]
Patch, r4981

Updated patch to apply to trunk again.  Now that I've got a more consistent internet connection, also pushing up to git://gitorious.org/~raof/banshee/gapless-work.git
Comment 59 Alexander Kojevnikov 2009-11-18 00:34:56 UTC
Review of attachment 147845 [details] [review]:

I committed the patch to the remote 'gapless' branch. I think it will be easier if further patches (if any) are diffed against the branch. Thanks!
Comment 60 Gabriel Burt 2009-11-18 18:59:50 UTC
A few problems while testing the gapless branch:

1) On track transition (or maybe right before), the old track's playback indicator flashes to paused then back to playing very briefly

2) I'm still hearing a tiny gap/pop on transition

and as you've already mentioned
3) the seek slider's label changes from "4:43 of 4:44" to "4:44" at the transition point
Comment 61 Sebastian Dröge (slomo) 2009-11-18 19:12:48 UTC
(In reply to comment #60)
> A few problems while testing the gapless branch:
> 
> 1) On track transition (or maybe right before), the old track's playback
> indicator flashes to paused then back to playing very briefly

That's bug #602000.

> 2) I'm still hearing a tiny gap/pop on transition

Also bug #602000, I'll work on that tomorrow.

> and as you've already mentioned
> 3) the seek slider's label changes from "4:43 of 4:44" to "4:44" at the
> transition point

That's bug #585969, fixed in GIT.
Comment 62 Gabriel Burt 2009-11-18 19:28:51 UTC
Awesome, thanks Sebastian!
Comment 63 Sebastian Dröge (slomo) 2009-11-25 07:25:00 UTC
You shouldn't really use the gapless feature until bug #602437 is fixed btw. See details there ;)

playbin2 should still be used ASAP, it brings enough other advantages over old playbin.
Comment 64 olivier dufour 2009-11-27 17:27:32 UTC
yes but playbin2 (and regular playbin too) do not support crossfading.
bug 524300 adress that for banshee. I have open a bug for this on gstreamer with bug 602286. Gapless and crossfading can be done in one way by creating a special pipeline with adder but best is support that in playbin2. Do you know if there is a plan for that in gstreamer?
Comment 65 Kevin Rotz 2009-11-28 00:35:59 UTC
Sebastian,

I just wanted to say that this works great for me on Arch Linux. There issues with seeking through files (although I'm not sure if this isn't a bug with Banshee itself), but I was overjoyed when Tool's 'Parabol' swelled up into 'Parabola' without any silence at all :) 

I've only tried with FLAC, WAV, OGG and Lame MP3's. This worked for FLAC, WAV and OGG, but didn't work for LAME MP3s with gapless metadata - in fact, I think that it cut off the last half-second or so of playback. What's the probability of getting it to work properly with Lame MP3s? Songbird (which is what I'm using now) does it fairly well, so it may be useful to check out how they're doing it.

How feasible is it to search the audio buffer for silence at the beginning/end of tracks like iTunes does? This would create at least artificial gapless for every format.

Gapless playback is the only thing keeping me from switching. If this would work well with Lame MP3's, I'd make the switch to Banshee for sure :)
Comment 66 Christopher Halse Rogers 2009-11-28 22:02:46 UTC
(In reply to comment #65)
> Sebastian,
> 
> I just wanted to say that this works great for me on Arch Linux. There issues
> with seeking through files (although I'm not sure if this isn't a bug with
> Banshee itself), but I was overjoyed when Tool's 'Parabol' swelled up into
> 'Parabola' without any silence at all :) 
> 
> I've only tried with FLAC, WAV, OGG and Lame MP3's. This worked for FLAC, WAV
> and OGG, but didn't work for LAME MP3s with gapless metadata - in fact, I think
> that it cut off the last half-second or so of playback. 
It shouldn't have done this.  I don't think that playbin2 looks at the mp3 metadata at all; it's a simple play until the stream finishes system.

> What's the probability
> of getting it to work properly with Lame MP3s? Songbird (which is what I'm
> using now) does it fairly well, so it may be useful to check out how they're
> doing it.
This wouldn't be done in Banshee; we don't do anything fancy with gapless, just hook up playbin2's gapless support.  It could conceivably be done in playbin2, though.

> How feasible is it to search the audio buffer for silence at the beginning/end
> of tracks like iTunes does? This would create at least artificial gapless for
> every format.
Would we actually want to do that?  Won't that mess up non-concept albums?
Comment 67 Kevin Rotz 2009-11-28 22:40:12 UTC
> Would we actually want to do that?  Won't that mess up non-concept albums?
Good point. Perhaps that could be a way around this - a user-settable flag in the database? Songbird uses Gstreamer (I believe they've changed over to playbin2) and achieve gapless for Lame somehow, so I'm guessing they're doing something similar.
Comment 68 mannheim89 2010-01-30 22:16:00 UTC
Perhaps related to comment #48 and the subsequent fix: I get the following with the gapless branch (including current git):

1. *Disable* gapless playback in the preferences.
2. Select an album in the browser.
3. Double-click the last track to play.
4. Wait for the last track to finish.

Expected behavior: banshee stops at the end of the last track.

Actual result: banshee plays the last track again (and again and again).
Comment 69 Christopher Halse Rogers 2010-02-05 06:37:30 UTC
(In reply to comment #67)
> > Would we actually want to do that?  Won't that mess up non-concept albums?
> Good point. Perhaps that could be a way around this - a user-settable flag in
> the database? Songbird uses Gstreamer (I believe they've changed over to
> playbin2) and achieve gapless for Lame somehow, so I'm guessing they're doing
> something similar.
They're probably doing roughly what we're doing, plus actually looking at the lame tags (lame adds extra metadata saying when the track actually finishes).

(In reply to comment #68)
> Perhaps related to comment #48 and the subsequent fix: I get the following with
> the gapless branch (including current git):
> 
> 1. *Disable* gapless playback in the preferences.
> 2. Select an album in the browser.
> 3. Double-click the last track to play.
> 4. Wait for the last track to finish.
> 
> Expected behavior: banshee stops at the end of the last track.
> 
> Actual result: banshee plays the last track again (and again and again).

Confirmed.  I'll look into it.

http://gitorious.org/~raof/banshee/gapless-work contains a gapless-ng branch.  This has a compile-time check for a new-enough playbin2 (currently > 0.10.25.2, which isn't satisfied by any release yet) and has removed many of the work-arounds necessary for earlier attempts.  I'd appreciate a review with a view to getting it merged.
Comment 70 Christopher Halse Rogers 2010-02-11 06:30:48 UTC
And fixed.  gapless-ng now doesn't repeat the last track in the playlist when gapless is not enabled.

gapless-ng should now be ready to merge to trunk.
Comment 71 Gabriel Burt 2010-02-11 19:46:22 UTC
Awesome work Christopher!  I will test/review later today.
Comment 72 Tim Hockin 2010-02-14 00:20:15 UTC
Yay.  waiting anxiously!
Comment 73 Gabriel Burt 2010-02-26 19:49:57 UTC
The gapless branch of git.gnome.org is obsolete and I'll be deleting it.  Christopher has commit access there now, and has pushed his gapless-ng branch.
Comment 74 Christopher Halse Rogers 2010-03-09 01:37:28 UTC
Created attachment 155606 [details] [review]
Total gapless diff

Here's the total diff against master for the gapless-ng branch for review purposes; this is what would be merged.
Comment 75 Gabriel Burt 2010-03-09 02:04:54 UTC
Not looking too closely at the libbanshee changes, some comments:

1.  I feel like the variable name userRequested is a bit confusing.  Would it make more sense to call it immediateChangeRequested/Expected or something?  It's possible at some point that a script would want to change the track immediately.

2.  Also, the delay in SaveTrackMetadataJob.cs makes each track-write take at least 11 hours to write-out or rename 20k songs.  The delay should be commented (or variables named contextually) to indicate why it's done.  As I recall, it should only need to be done once, not before every job iteration.

3.  "Useful for concept albums & classical music.  EXPERIMENTAL"  -- remove the EXPERIMENTAL part, replace & with 'and'
Comment 76 Gabriel Burt 2010-03-09 02:06:38 UTC
Heh, each track write takes 2s is what I meant to write;  20k track writes would take 11 hours.
Comment 77 Aaron Bockover 2010-03-09 02:25:31 UTC
I second Gabriel's comments, and add that there shouldn't be any compile-time conditional code in managed land. (ideally none in native land either, but we can deal with that in a second pass).

There are still a few style/whitespace issues as well. Mostly not having a space before an opening paren.

Also in a few places there you have some logging invocations like this:

Log.Warning ("Start of one thought");
Log.Warning ("Continuing the thought here.");
Log.Warning ("Closing the thought here.");

These should just be grouped into a single Log.Warning call.

I played a couple of albums on gapless last night on OS X and it worked really well. On the whole I am ok with this being merged, once the few remaining minor concerns we've outlined are addressed.

This is awesome.
Comment 78 Alexander Kojevnikov 2010-03-09 02:30:42 UTC
(In reply to comment #75)
> 2.  Also, the delay in SaveTrackMetadataJob.cs makes each track-write take at
> least 11 hours to write-out or rename 20k songs.  The delay should be commented
> (or variables named contextually) to indicate why it's done.  As I recall, it
> should only need to be done once, not before every job iteration.

If you update/rename 20k songs, they will all have the same or nearly the same LastUpdatedStamp. The job checks if (Now-LastUpdatedStamp) is less than 2 secs and delay for the remaining of the wait period only if it is. This means only the first (few) tracks will be actually delayed.
Comment 79 Gabriel Burt 2010-03-09 02:38:00 UTC
Ah cool, that works.
Comment 80 mannheim89 2010-03-10 12:51:52 UTC
Building this morning from git master (now that gapless is in master), I am finding the same problem I reported in Comment 48 above. That is, playback continues back on Track 1 after playing the last track of an album.

This was definitely fixed for me in gapless-ng. (I has previously been building banshee using git master with gapless-ng merged in. These builds didn't have that problem.)
Comment 81 mannheim89 2010-03-10 13:37:25 UTC
A correction to my last comment: I said above that my previous recent builds did not have the "unwanted repeat" problem. In fact, my most recent previous build, from a day or two ago (master, with gapless-ng merged), does have the same problem. Older builds of the same sort do not (for me).
Comment 82 Gabriel Burt 2010-03-11 18:54:26 UTC
gapless-ng has been merged and released in Banshee 1.5.5 - closing.  Please file new bug reports for any issues you find.