GNOME Bugzilla – Bug 600072
Improvement for volume control latency
Last modified: 2010-09-15 02:43:20 UTC
Created attachment 146528 [details] [review] Patch to improve replaygain and volume latency As described in https://bugzilla.gnome.org/show_bug.cgi?id=562046 there is a latency when using replaygain. I changed the current implementation. It now uses the gstreamer plugin rgvolume instead of the code from banshee-player-replaygain.c. Additionally this path introduces an additional pipeline element for volume control. This eliminates the normal volume control latency for me.
Thanks Christian, the patch works as advertised. It still needs to be reviewed by abock or gabaug, I know little about libbanshee.
It works for ReplayGain here.
Setting target for 1.6, this would be a good fix to have
It seems you missed out the ReplayGain debug logging though: - bp_debug ("scaled volume: %f (ReplayGain) * %f (User) = %f", scale, player->current_volume, - g_value_get_double (&value)); It should be trivial enough to re-add it.
(In reply to comment #4) 2 questions before i fix the patch: 1. The ReplayGain target volume is in dB, e.g. -20.0dB instead of 0.1. player->current_volume is not in dB. Should i convert the dB to get the same output as in the current version of banshee? 2. The resulting overall volume is not directly readable from any pipeline element. If it should still be printed, it must be calculated from the ReplayGain gain and the current_volume. If this is necessary, should it be in dB or as a value between 0.0 and 1.0?
I think the current behaviour would be fine, though I think a Banshee developer should comment on this?
Tangentially related: There's a bug over on launchpad (https://bugs.launchpad.net/banshee/+bug/377354) that shows that replaygain does not update after the first/second song played. The patch on this bug appears to fix it as a nice side-effect. :)
Is it just me, or does this patch break video playback?
(In reply to comment #8) > Is it just me, or does this patch break video playback? (sorry for the late response) I haven't been able to use video playback for quite some time - with and without the patch I get: ./Banshee.exe: Fatal IO error 11 (Resource temporarily unavailable) on X server :0.0. If video works, can anyone with working playback give the patch a test?
I haven't received any complaints about video playback issues in the Banshee builds in the daily PPA, and this patch has been enabled there for quite some time (since November). I personally use it as well and don't have any video playback issues.
(In reply to comment #10) > I haven't received any complaints about video playback issues in the Banshee > builds in the daily PPA, and this patch has been enabled there for quite some > time (since November). I personally use it as well and don't have any video > playback issues. Ah, then it's probably just the result of me applying the patch to an old snapshot (from 2009-12-26). I'll see if I can try it with a more recent snapshot and report back.
Oddly enough, compiling a new snapshot (2010-01-20) seems to have mitigated the video problem, but not eliminated it - Banshee no longer crashes consistently, but instead randomly crashes or hangs when starting a video. Most of the crashes leave the same debug entry as that in comment 9: /usr/lib/banshee-1/Banshee.exe: Fatal IO error 11 (Resource temporarily unavailable) on X server :0.0. I haven't observed any differences between a build with the patch and a build without it, so I suppose it's safe to say the video is a separate issue.
Review of attachment 146528 [details] [review]: Marking as needs-work as per comments 4-6. Christian, could you adapt your patch to include debug logging? It doesn't have to be in the same format if that's not straightforward to implement. Thanks!
Created attachment 153768 [details] [review] Reworked patch to improve replaygain and volume latency I added the requested debug output. It prints linear volume information, if replay gain is enabled. The patch is created against the current HEAD.
Before this is committed, I'd like to know a few things: Has it been tested it with audio and video files, switching to Now Playing, skipping songs, changing volume, turning rg off/on? I'd also like to know if we'll lose some functionality changing from abock's custom rg implementation to rgvolume - his version has this 10-track history thing, for example.
(In reply to comment #15) > Has it been tested it with audio and video files, I tested it only with audio files, yet. > switching to Now Playing, skipping songs, Pause, Play and Skip is tested. > changing volume, turning rg off/on? Changing volume works, turning rg on and off, too. But if you start a song without rg and then switch it on, the rg information is not available for the current song. The information is available for the next song. > I'd also like to know if we'll lose some functionality changing from abock's > custom rg implementation to rgvolume - his version has this 10-track history > thing, for example. The 10-Track history is lost in this patch.
I tested with both audio and video, with all scenarios I could imagine. Everything works fine.
Incidentally, the latency on volume-change will be fixed automatically by the gapless branch; this is one of the behaviours fixed in playbin2.
Cool, so when will it get merged? =)
Review of attachment 153768 [details] [review]: I'll be applying most of this patch to my gapless branch on gitorious; It's by-and-large correct, and much easier for me than fixing the existing RG implementation to work with gapless ;). It doesn't handle errors well, though - should a GStreamer error occur, the pipeline is destroyed. This causes the rgvolume element to lose its only reference (since gst_bin_add takes ownership of the element passed in), but the current code does not detect this; the player->rgvolume pointer remains non-null, and instead points to... something. In my testing, it often ended up pointing to another member of the pipeline (which then raised warnings about flacdec, for example, not having a target-gain property), but sometimes it just crashed Banshee. Finally, you've got some stray tabs in there; these should be converted to spaces. ::: libbanshee/banshee-player-pipeline.c @@ +235,3 @@ } player->preamp = gst_element_factory_make ("volume", "preamp"); + Unnecessary whitespace changes should be removed. @@ +331,3 @@ +pad_block_cb (GstPad *pad, gboolean blocked, gpointer user_data) { +static void + Why do you have this local variable? Why not simply rename the formal parameter to srcPad? @@ +375,3 @@ + pad_block_cb(srcPad, TRUE, player); + } +} I think that _bp_pipeline_rebuild and pad_block_cb should be in banshee-player-replaygain.c. They're only interesting for ReplayGain. Furthermore, I'd rename _bp_pipeline_rebuild to mention ReplayGain (again, as it's not rebuilding the whole pipeline, just the RG bit). ::: libbanshee/banshee-player-replaygain.c @@ +48,1 @@ +GstElement* _bp_rgvolume_new (BansheePlayer *player) You don't actually seem to use _bp_rgvolume_new anywhere? ::: libbanshee/banshee-player.c @@ +99,3 @@ +{ +bp_update_volume (BansheePlayer *player) +static void This should probably be g_return_if_fail, and use IS_BANSHEE_PLAYER and GST_IS_ELEMENT. It provides better logging - silently failing to set the volume would be annoying. That will unfortunately show you that Banshee is calling bp_set_volume before initialising the pipeline; I haven't tracked this down yet.
Created attachment 154626 [details] [review] Reworked patch (2nd time) to improve replaygain and volume latency Most of the changes are applied to this patch. The error handling, if the pipeline is destroyed, is still not implemented. As i am new to gstreamer: What would the best way to handle the errors?
Christian, Christopher: is there anything else left in this patch that haven't been incorporated into the gapless branch (which is now merged into master)? I'm still hearing delays when changing the volume. RG volume seems to be applied instantly though.
If your audiosink supports volume control, then the gapless branch fixes everything. (Sufficiently new?) Pulsesink supports volume control, alsasink does not. If your audiosink does not support volume control, then it seems the the gapless branch misses the volume latency; playbin2 still seems to stick its volume control too early in the pipeline. Without the sink's cooperation, though, there's no (easy) way we can completely eliminate the latency - there'll always be the audiosink's (and audio card's) buffer. The gapless branch does eliminate the latency with replaygain, because rgvolume sits in the pipeline and can set the appropriate volume on the samples as they pass through.
As Christopher said, the replaygain part of the patch is now active in the master. But additionally this patch contains the normal volume latency fix (for me). It is the additional volume element in the gstreamer pipeline. At the moment i cannot build banshee, maybe someone could check if adding the volume element to the pipeline fixes this issue.
Review of attachment 154626 [details] [review]: Marking as needs-work because of comment 26.
Created attachment 162292 [details] [review] Insert volume element into the pipeline This patch takes relevant bits from Christian's patch to insert a new volume element into the pipeline. The latency is still noticeable, but it's much much better than setting volume on the entire pipeline. I've noticed a few issues though: 1. The volume control is set to muted on start up, apparently this behaviour is also there in git master. However, unlike git master, the control is not updated to the right volume when playback starts. 2. The volume scaling is different from git master. Dragging volume control to 50% sets the real volume to 0.5 with the patch and to about 0.2 in git master. 3. When the song ends, the seeker shows times past the song's duration for a second or two.
(In reply to comment #28) > 1. The volume control is set to muted on start up, apparently this behaviour is > also there in git master. However, unlike git master, the control is not > updated to the right volume when playback starts. If this is just a cosmetic issue, then that's exactly what's happening on my copy of git master without this patch. Even after I've started playing music, the control isn't visually updated, unless I manually change it.
Review of attachment 162292 [details] [review]: Hey Alexander, Can you re-test your patch with latest stable/master? The cosmetic-not-initialized bug should be gone, and perhaps the scaling bug too?
Created attachment 169733 [details] [review] Updated to git master Attaching a slightly updated patch that cleanly applies to git master. (In reply to comment #30) > Can you re-test your patch with latest stable/master? The > cosmetic-not-initialized bug should be gone, and perhaps the scaling bug too? 1. Is no longer an issue. The volume control is set properly both on startup and after the playback begins. 2. Is still there, however I think the behaviour of this patch is more correct that what git master does. In git master the volume is not linearly proportional to the volume set, e.g. 50% slider setting results in 10x lower volume and 25% setting results in 100x lower value. 3. Is no longer reproducible. I'm for committing this patch after a bit of testing, would be nice to have it in 1.8
Review of attachment 169733 [details] [review]: Thanks Alexander, please commit
Committed, thanks to everyone involved!