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 585956 - [mp3parse] indexing improvements
[mp3parse] indexing improvements
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-ugly
git master
Other Linux
: Normal enhancement
: 0.10.13
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-06-16 08:57 UTC by Tim-Philipp Müller
Modified: 2009-06-22 09:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mp3parse: assume seekability only if we know the upstream size (4.09 KB, patch)
2009-06-16 08:58 UTC, Tim-Philipp Müller
committed Details | Review
mp3parse: don't put every single frame into the index (4.46 KB, patch)
2009-06-16 08:59 UTC, Tim-Philipp Müller
committed Details | Review

Description Tim-Philipp Müller 2009-06-16 08:57:47 UTC
+++ This bug was initially created as a clone of Bug #585576 +++

There was this nice fix for mp3parse that it doesn't build an index table over time when it cannot seek anyways, which is supposed to stop the unbounded memory growth when listening to internet radio (for embedded devices, extra CPU load is also a factor).

Problem is that souphttpsrc initially assumes that servers support seeking. Try:
GST_DEBUG=mp3parse:4 gst-launch-0.10 playbin2 uri=http://gffstream.ic.llnwd.net/stream/gffstream_stream_wdr_einslive_a 2>&1|fgrep index

The server sends no Content-Length header, so there is no stream duration (which makes it questionable to assume seekability(?)).

Either souphttpsrc should have better prediction of seekability or maybe mp3parse should suppress index table building when the upstream duration query fails.
Comment 1 Tim-Philipp Müller 2009-06-16 08:58:34 UTC
Created attachment 136703 [details] [review]
mp3parse: assume seekability only if we know the upstream size
Comment 2 Tim-Philipp Müller 2009-06-16 08:59:12 UTC
Created attachment 136704 [details] [review]
mp3parse: don't put every single frame into the index
Comment 3 Michael Smith 2009-06-16 17:21:22 UTC
I think you're leaking the seeking query here (in the first patch), otherwise looks good.

Second patch is probably ok too, but I find that code fairly impenetrable already, so I'm not 100% sure.
Comment 4 Tim-Philipp Müller 2009-06-22 09:48:09 UTC
> I think you're leaking the seeking query here

Fixed, thanks.


 commit af3ab2ae945325976cb247e819a348850b81cd8f
 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
 Date:   Tue Jun 16 09:45:59 2009 +0100

    mp3parse: don't put every single frame into the index
    
    Let's not put every single mp3 frame in our index, a few frames per
    second should be more than enough. For now use an index interval
    of 100ms-500ms depending on the upstream size, to keep the index at
    a reasonable size. Factor out the code that adds the index entry
    into a separate function for better code readability.


 commit 1db592839e1cb07fca7bbd3f2878e76bfb154ac2
 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
 Date:   Tue Jun 16 01:40:42 2009 +0100

    mp3parse: assume seekability only if we know the upstream size
    
    While technically upstream may be seekable even if it doesn't know
    the exact size, I can't think of a use case where this distincation
    is relevant in practice, so for now just assume we're not seekable
    if upstream doesn't provide us with a size. Makes sure we don't
    build a seek index when streaming internet radio with sources that
    pretend to be seekable until you try to actually seek.