GNOME Bugzilla – Bug 484881
DAAP seek support is poor
Last modified: 2018-05-24 12:55:56 UTC
Please describe the problem: The DAAP plugin currently operates in push mode; the most appropriate mode for its type. However, GStreamer's push mode suffers from poor seeking support in a number of plugins: - MPEG-4 demuxer will not seek in push mode. - AAC decoder will not seek in push mode. - OGG demuxer will not seek in push mode. This patch makes the DAAP plugin use pull mode instead of push mode. Aside from some extra code to buffer streamed data, I see no particular flaw in doing this. The pull mode plugin seeks correctly with all of the elements listed above Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
Created attachment 96909 [details] [review] Switch DAAP push mode source to pull mode Tested and working with .m4a and .ogg. Chunked stream support is untested - not sure how to obtain a stream of this type for testing.
Has any progress been made with this? I've managed to apply the rhythmbox-0.11.6-17.r6096 from Fedora 10 (with some adjustments) and it does the job (although the delay upon skipping is longer with Ogg Vorbis than MP3).
Created attachment 124435 [details] [review] Switch DAAP push mode source to pull mode I have been using this patch since I wrote it and still apply it to Rhythmbox on each Ubuntu install. I think it's mature enough for inclusion but that decision lies with the maintainers. Curious to hear that Ogg seeks less quickly than you would like, though. I use Firefly Media Server to stream Ogg files quite a bit and seeking is about as instant as one could expect. Are you connecting to a media server on a high latency connection? (Attached a slightly newer patch that still applies to trunk.)
Thanks for the new patch. This is between two Rhythmbox instances on a wireless LAN. It seems the latency increases with track length: on short tracks, seek is more-or-less instantaneous. But on longer tracks (e.g. one I've got is 3602 seconds long) the latency can be around five seconds.
Hmm, I'm unable to reproduce that on Ubuntu Intrepid with Rhythmbox 0.11.6 and GStreamer 0.10.21. I made a 3h (10000 second) Ogg file and streamed it from Firefly on one machine to Rhythmbox on another and seeking was instant. Are you sure your Firefly server isn't trying to transcode the Ogg to MP3 in real time? That could explain long seek delays. You can switch off transcoding (it's unnecessary for Rhythmbox; debatably for iTunes but Firefly currently a bug that always reports MP3 even when streaming Ogg, which iTunes does not like) with: never_transcode = ogg In mt-daapd.conf.
I'd been using Rhythmbox to serve. Seeking is much more responsive using Firefly server (although for some reason RB doesn't like it when Ff has a password...).
Created attachment 149208 [details] [review] Make DAAP plugin use GStreamer pull mode Patch refresh for 0.12.5.
Review of attachment 149208 [details] [review]: As I said on the mailing list, it should be possible to use the GStreamer queue2 element to do this without implementing buffering ourselves.
Created attachment 150508 [details] [review] Replace 80% of DAAP GStreamer code with "souphttpsrc" OK, this is a tricky one so let's start with a cleanup patch. The vast majority of code in the DAAP plugin is for HTTP handling. Specifically, many of the complexities arise from trying to handle "chunked" HTTP - a method whereby the server can send multiple fixed-size responses to make up a whole file (or presumably stream endlessly). All of this code can be replaced by using the GStreamer "souphttpsrc" element. I have modelled a cleanup patch on rb-mtp-gst-src.c (which delegates much of its own work to a "filesrc" element) and it seems to work fine. The rb-daap-gst-src.c is much slimmer after applying it. I have no idea what server sends chunked HTTP so I am unable to test that case. I have tested with a Rhythmbox server and the Firefly Media Server (mt-daapd) and both work fine. Playing from an iTunes 9 source doesn't seem to work in 0.12.5 with or without this patch; it gets stuck trying to list the library elsewhere in the plugin. In any case, if we need chunked HTTP support and it doesn't work in "souphttpsrc" then that's where it should be fixed. I'm happy to do that if anyone relies on this feature and it doesn't work post-patch. I'd much rather it was fixed in "souphttpsrc" than botched in here. (None of this fixes the seek issue discussed in this bug report because "souphttpsrc" is also a GstPushSrc. This is a preliminary patch to help me explain more cleanly what needs to be done to fix that bug.) How's this patch look for a start? :)
Review of attachment 150508 [details] [review]: This generally looks like a good idea. ::: plugins/daap/rb-daap-src.c @@ -29,3 @@ */ -#include "config.h" generally all c files should include config.h @@ +159,3 @@ + src->souphttpsrc = gst_element_factory_make ("souphttpsrc", NULL); + if (src->souphttpsrc == NULL) { + g_warning ("couldn't create souphttpsrc element"); indent should be 8 spaces/1 tab, not 2 spaces @@ -515,3 @@ - - /* The following can fail if the source is no longer connected */ - headers = rb_daap_source_get_headers (source, src->daap_uri, src->seek_bytes); This needs to be reimplemented using the souphttpsrc user-id, user-pw and extra-headers properties.
Created attachment 150553 [details] [review] Replace 80% of DAAP GStreamer code with "souphttpsrc" I think this patch addresses these issues. Well spotted on the password-protected point: I hadn't run into this during testing because Rhythmbox apparently doesn't require authentication tokens once the initial connection has been opened and authenticated! It was merrily letting me play without sending the right headers. Rhythmbox doesn't appear to like my password-protected Firefly server (it gets stuck elsewhere in the plugin) so I have been unable to test that this new patch interacts correctly. However, I have traced the connection and observed all the right headers now being sent. I backed off replacing the "Authentication:" header with the user/password properties of souphttpsrc because I didn't have a server to test with; this seems to be the safest way forwards for now.
Weird.. splinter doesn't seem to understand your patch file properly. How did you generate it?
Created attachment 150555 [details] [review] Replace 80% of DAAP GStreamer code with "souphttpsrc" Hmm. Same way I always generate patches: diff with manual editing to remove irrelevant files. The patch applied cleanly when I tested it. Anyway, here's the raw diff. Because it's recursive it'll need -p2 to apply.
I fixed up a couple of other things (the main thing is that rb-daap-src.c needs to include rb-daap-src.h) and committed it, since this seems to work no worse than what was there before. Generally it's easiest to use 'git diff' or 'git format-patch' to create patches.
Thanks John. Since we have a good reason for using a push mode source, I traced this problem back into GStreamer. Ogg turned out to be an inherently difficult format to seek and usually requires expensive bisection searches over the entire stream. Hence, in push mode there was intentionally no seek support. The Ogg Index [1] proposal (preliminary, the format is not yet frozen) rectifies this situation by inserting a metadata stream of seekable time:offset mappings. So, I set about writing a patch to include (preliminary) support in GStreamer and this is now being tracked in bug 607945 [2]. With this patch in place and index metadata added to my Ogg Vorbis files, I can happily seek over DAAP. :) [1] http://wiki.xiph.org/Ogg_Index [2] https://bugzilla.gnome.org/show_bug.cgi?id=607945
This looks interesting, and I hope it works out.. Is there a plan for getting Ogg Index data into people's ogg vorbis files so this will actually work? Is it possible to write the index data while transcoding or encoding from raw audio? If so, does it make sense to build this into oggmux so it happens automatically? If postprocessing is required, how practical would it be to automatically write index data into ogg vorbis files during playback or (maybe) importing?
The uptake of metadata streams (up to Skeleton 3.0, which is frozen) in audio content creation applications has been limited - those built upon liboggz are perhaps most prominent - although the information was of debatable value to non-video streams up until 3.1. Ripping and transcoding are good candidates for introducing metadata streams into a user's audio library. As you suspect, the information required for the skeleton header and index streams - which must appear before the content pages of other streams - necessitates a postprocessing phase. I am not sure how well this would fit into GStreamer's model. Modifying existing Ogg files without the user's consent would probably be a step too far. OTOH, I can see no easy way to inform users that their Ogg files are unindexed and thus unsuitable for sharing over DAAP. It may be the case that this approach will only be useful in newly ripped files constructed through an application with skeleton support (e.g. via liboggz) or those passed through an indexing tool by knowledgable users. (The alternative is to add support for GST_FORMAT_PERCENT seek to oggdemux and assume a rough mapping of time:file size. Rhythmbox knows the stream length from its own metadata database so it would be able to issue seek requests in that format.)
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/rhythmbox/issues/447.