Yeah, Spider needs some love or maybe even to be replaced.

Isn't this the same as in 104854?

Yes it's the same as 104854, but that is marked as fixed for some
reason ;)

However, I have just fixed this bug in HEAD, attaching patch for 0.6.1
and changing fields to represent that.

Created attachment 15607 [details] [review]
Patch that backports 0.7 mp3 typefinding to 0.6 branch

Got it - applied to 0.6.1.

I'm still having trouble with this actually with 0.6.1.  Here's the
commands I'm using to test:

 (/build/gstreamer-0.6/bin/gst-launch -v gnomevfssrc iradio-mode=1
location=http://205.188.209.193:80/stream/1003 ! spider ! esdsink 2>&1)

The pipeline does work if I replace "spider" with "mad".  And spider
*does* work with 0.7.x. 

Spider is much better at least for local files, so I'm just going to
hardcode mad for iradio in netRhythmbox when we're using GStreamer
0.6.x for now.

Would be "really" great to get this fixed in the 0.6.x
series as it is causing problems getting net-rhythmbox
unforked.

I'm going to look into this tonight, see if I can come
up with anything useful.

I can confirm Colin's test case...

bug 
gst-launch gnomevfssrc
location="http://205.188.209.193:80/stream/1003" ! spider ! esdsink
does work. It seems that what was breaking Colin's test case was
the iradio-mode=1 1 part.

I'm not sure what exactly this means but hopefully it will
help the bug get fixed.

above: /r/bug/but

Colin, did the fix we made to gnomevfssrc's iradio mode make it into
0.6.1 or is that fix only in HEAD?
This one:
http://cvs.sourceforge.net/cgi-bin/viewcvs.cgi/gstreamer/gst-plugins/ext/gnomevfs/gstgnomevfssrc.c.diff?r1=1.30&r2=1.31

Created attachment 15880 [details] [review]
patch to fix iradio code path

May I apply this patch to the 0.6.x CVS?

Yes, go ahead.

*** Bug 111428 has been marked as a duplicate of this bug. ***

Ok, applied.

Ideally though we wouldn't have to use this hack to make typefinding
work.  I think people were discussing having spider try pulling until
it had "enough" data to make a decision.

So we could keep this bug around as a wishlist to implement that.  Or
we could open a new one and close this one.  Up to you.

Problem is that nobody knows how much data we need for typefinding.
If somebody puts everything he can (complete with images of the 
cover) into an IDv2 tag at the beginning of the file, we could easily 
need 50k of data before we know what kind of data (mp3, flac, 
something else?) we have.

Another thing is that lots of mp3s have garbage at the beginning of 
the file, for example loads of zeroed bytes. If we wanted to support 
these (invalid) mp3s we would have to look quite far into the file 
(no idea how far).

However proper typefinding requires a redesign of the API for 
typefinding functions, the current API doesn't support this. (It 
should allow for some other things like ranks for sorting which 
typefind function to use first, confidence values, ...)
I did a bit of design on that one in Oslo but lost it somwhere.

Could somebody please clarify the status of this bug. I'm a bit lost
following all the comments. Is it true that all 0.6.x versions are
broken? Does the above patch fix the problem? Is there a fix anywhere
for this bug?

a) Yes. Older 0.6 releases are more broken, newer are less broken. We 
still find cases where it doesn't work, but they've become less.
b) Both patches are applied in 0.6.2 I think, both fix some issues, 
buit not all.
c) No. But I'm aware of what needs to be done.

*** Bug 119272 has been marked as a duplicate of this bug. ***

Created attachment 19187 [details] [review]
bla

One of my mp3s refused to play, so I fixed spider to properly use
gst_buffer_merge(). It needed 4,2kB data and only fetched 4,0kB, so it
fetches 8kB typefind buffers now. This doesn't fix anything except my
specific mp3s.  We can probably extend this to 50k to make
99,99999999999999% of all songs work if anyone cares.

Patch is against 0.6.x, btw.

*** Bug 119911 has been marked as a duplicate of this bug. ***

*** Bug 120086 has been marked as a duplicate of this bug. ***

Patch applied to 0.6.x. Benjamin wants it *not* in HEAD so he'll fix
it "the right way" (I'm guessing for a new typefind system here? ;) ).

*** Bug 120306 has been marked as a duplicate of this bug. ***

MMM, a lot of my mp3's still break this. That patch only applied 
to that guy's mp3s. And since this still exists on HEAD I'm going
to re-open...

Mark, we'll need examples of each and every mp3 file that breaks, I'm
affraid...

I've got entire albums that don't work. I think it might be because
they have extra data in them like album covers. I'll upload a sample
somewhere...

Trying to play http://sisob.tuxfamily.org/screenshots/broken.mp3

[sisob@localhost sisob]$ gst-launch filesrc location="broken.mp3" !
spider ! osssink
INFO (31660: 0) Initializing GStreamer Core Library version 0.6.3.1
INFO (31660: 0) CPU features: (c1c3f9ff) MMX SSE 3DNOW MMXEXT
INFO (31660: 0) registry: loaded user_registry in 0.000172 seconds
          (/home/sisob/.gstreamer/registry.xml)
INFO (31660: 0) registry: loaded global_registry in 0.155349 seconds
         
(/home/sisob/Software/gnome24/var/cache/gstreamer-0.6/registry.xml)
GStreamer-INFO: 0 live buffer(s)
GStreamer-INFO: 0 live bufferpool(s)
GStreamer-INFO: 0 live event(s)
RUNNING pipeline
ERROR: /pipeline0/spider0/sink_ident: Could not find media type
execution ended after 1 iterations (sum 96701000 ns, average 96701000
ns, min 96701000 ns, max 96701000 ns)
GStreamer-INFO: 0 live buffer(s)
GStreamer-INFO: 0 live bufferpool(s)
GStreamer-INFO: 0 live event(s)


Replacing spider with mad in the pipeline makes the mp3 play.

> Mark, we'll need examples of each and every mp3 file that breaks, 

I'm pretty sure that they are all breaking for the same reason as they
were all ripped within a few weeks of eachother using the same piece
of software.

Oh, but this is allright. I simply meant that when you find one that
breaks, I'd love to get a sample of it so I can reproduce it locally. :).

Thanks for the file, I'll look into it.

As I guessed, the typefind buffer is (again) too small. It has a JPG
image attached at the beginning of the file. The actual data only
starts around position 0x11CE0. The proper way to fix this is to let
the typefind function pull more data if needed. That's work for HEAD
though. Short-term solution (for 0.6.x) is - again (...) - to increase
the typefind buffer to something above 0x12000 (say, ~75kB) in spider.

And no, I don't like this either. :).

Oh, we could also consider to make 49 44 33 (ID3) be enough for
typefinding in 0.6.x, though that has some issues too because we'll
have one mismatch for every 2^24 (16M) files.

There's an mp3 that fails to play at http://www.gnome.org/~kmaraas -
the massive attack one.

*** Bug 121439 has been marked as a duplicate of this bug. ***

*** Bug 122924 has been marked as a duplicate of this bug. ***

I'm flagging this as our general "typefinding sucks and needs a 
rewrite - especially because of mp3" bug now.
Dear Rhythmbox guys, please mark all "mp3 could not be detected bugs 
as duplicates of this one.

*** Bug 119159 has been marked as a duplicate of this bug. ***

*** Bug 115124 has been marked as a duplicate of this bug. ***

I've got a new typefind system! :).

General explanation: instead of providing the typefind buffer with a
buffer, we let it pull buffers itself, so it always has enough data.
However, it doesn't read. Instead, it peeks, so we don't actually lose
data (that'd kill playback after typefinding).

typedef (GstCaps *) (* GstTypeFindFunc) (GstByteStream *bs, gpointer
data);

This has several effects:
* bytestream is now part of the core
* all plugins have been modified to use this new typefind system
* asf typefinding added
* mpeg video stream typefiding removed because it's broken
* duplicate typefind entries removed
* extra id3 typefinding added, because we've seen 4 types of files
(riff/wav, flac, vorbis, mp3) with id3 headers and each of these needs
to work. Instead, I've added an id3 element and let it redo typefiding
after the id3 header. this needs a hack because spider only typefinds
once. We can remove this hack once spider supports multiple typefinds.
* with all this, mp3 typefinding is semi-rewritten
* id3 typefinding in flac/vorbis is removed, it's no longer needed
* fixed spider and gst-typefind to use this, too.
* Other general cleanups

Patches follow. The resulting system works for me locally. I'd love
some people to test this. I want to commit this to HEAD asap.

Apart from the patches below, gstreamer/libs/gst/bytestream/*,
gst-plugins/gst/mpegaudioparse/gstmp3types.c,
gst-plugins/gst/mpegtypes/gstmpeg[12]types.c can be removed.

Created attachment 20353 [details] [review]
gstreamer core patch for new typefinding

Created attachment 20354 [details] [review]
gstreamer plugins patch for new typefinding

Created attachment 20385 [details] [review]
New patch for plugins

The new patch above differs in two respects:

* id3 plugin moved to its own directory. Dependency on libid3tag (mad)
is optional, though without it, metadata won't be read from the id3 tag.
* added a query() function to the id3types elements, this should make
it totally transparent to the pipeline.

I'd like to commit this asap, please make comments if things aren't
ok, else I'll commit it.

New stuff committed. If any problems arise, please open a new bug. I
consider this bug fixed for now, this is all we can do for both HEAD
and 0.6.x...