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 647364 - VGM files loop indefinately
VGM files loop indefinately
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
0.10.21
Other Linux
: Normal blocker
: 0.10.22
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-04-10 14:48 UTC by mihai.draghicioiu
Modified: 2011-04-26 06:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fixes endless loop when playing VGM files (at least) with libgme (1.39 KB, patch)
2011-04-10 16:56 UTC, mihai.draghicioiu
needs-work Details | Review
Fix endless loop when playing files supported by libvgm, with 8 second fade at the end (1.41 KB, patch)
2011-04-16 19:42 UTC, mihai.draghicioiu
none Details | Review
Fix endless loop when playing files supported by libvgm, with 8 second fade at the end (git fornat-patch) (1.71 KB, patch)
2011-04-16 19:56 UTC, mihai.draghicioiu
none Details | Review
Fix endless loop when playing files supported by libvgm, with 8 second fade at the end (git fornat-patch) (1.70 KB, patch)
2011-04-16 20:25 UTC, mihai.draghicioiu
committed Details | Review

Description mihai.draghicioiu 2011-04-10 14:48:20 UTC
Hi guys. I'm a fan of old video game music, and I was happy to find that gstreamer supports playing back vgm files. Later I found that it also supports other formats, via libgme. However, when playing back most songs, they end up looping indefinately. The song length is reported correctly, but when the time slider reaches the far right (in Totem and Rhythmbox), it stays there and the song keeps looping. The structure of these songs is basically Intro+Loop.

My suggestion is to play the intro once and the loop twice. It seems that this is what in_vgm.dll does (winamp).

To reproduce the bug, download some sample vgm music ( http://project2612.org/details.php?id=599 for example - but be sure to ungzip the track from vgz to vgm), and play it in Totem or Rhythmbox.

Here is my suggested code fix. In ext/gme/gstgme.c, line 409, gme_track_ended() is called. This function returns false for looping songs. Whether this is correct or not, I don't know, but certainly, what can be done is:

instead of

if (gme_track_ended (gme->player)) {

use:

if (gme_track_ended (gme->player) || gme_tell(gme->player) * GST_MSECOND < gme->intro_length + gme->loop_length * 2) {

and to set intro_length and loop_length, add two GstClockTime members to the _GstGmeDec struct, and fill them in at gstgme.c:484 with:

  gme->intro_length =
      gst_util_uint64_scale_int (info->intro_length, GST_MSECOND, 1);
  gme->loop_length =
      gst_util_uint64_scale_int (info->loop_length, GST_MSECOND, 1);

I can provide a patch, if everyone agrees that looping the loop section twice is the best idea. Basically what this does is make the song not too short and not too long. It should work for most songs. More correct would be to have a setting somewhere, but I wouldn't know how to code that.
Comment 1 mihai.draghicioiu 2011-04-10 14:49:36 UTC
Perhaps I need to clarify that the looping bug occurs with VGM files (sega mega drive/genesis), and I haven't tested other file types.
Comment 2 mihai.draghicioiu 2011-04-10 16:56:48 UTC
Created attachment 185650 [details] [review]
Fixes endless loop when playing VGM files (at least) with libgme
Comment 3 Sebastian Dröge (slomo) 2011-04-13 14:39:29 UTC
I don't think this is correct. If the file specifies that it should be looped over and over again until the end of time there's no reason to only loop two times. Other files might specify that the loop should be done 3 times and you would break correct behaviour on those files with this patch.
Comment 4 mihai.draghicioiu 2011-04-13 18:04:17 UTC
The files themselves do not specify the number of loops to play, because they were designed to be played indefinately as the level background music, until the level ended. However, some files have a play_duration field in the header, which from my understanding is optional (see gme.h, i think there is something written in there about it). So the only thing that can be done to improve the patch would be to also test for gme_tell(gme->player) * GST_MSECOND > (gme->play_duration > 0 ? gme->play_duration : gme->intro_duration + gme->loop_duration * 2).

Think of it this way. Infinite looping has to be avoided, and from all the information we have, this seems the best we can do. Like I said, we don't have the number of loops from libgme, all we have is play_length and even that is optional. intro_duration and loop_duration seem to be always there. I suppose this would require extensive testing with several file types that libgme supports, to see how it behaves. But perhaps the libgme author himself has some more info on this subject.
Comment 5 mihai.draghicioiu 2011-04-13 18:07:19 UTC
alternatively, there could be a (registry?) setting for number of times to loop song

if it's 0, loop indefinately. if it's >0, loop that number of times. if it's below 0, loop until play_duration is reached, and if play_duration is 0 (unset), play negative that number of times.

but i wouldn't know how to implement that
Comment 6 Sebastian Dröge (slomo) 2011-04-14 07:50:00 UTC
Checking if there's a play_duration > 0 and if it isn't assuming that two loops sounds like a good plan.
Comment 7 Sebastian Dröge (slomo) 2011-04-14 07:50:30 UTC
Do you want to update your patch? If you want, please attach it in git format-patch format.
Comment 8 mihai.draghicioiu 2011-04-14 07:55:03 UTC
Sure. libgme also provides the ability to fade out, so I'll add 2 seconds fade out after the second loop run (first two seconds of the third loop)
Comment 9 mihai.draghicioiu 2011-04-16 19:40:27 UTC
Alright, after a bit of research and effort, I found a solution that should work fine. Apparently, the play_length member of the gme_info_t struct contains the value we need for song length. Here's an excerpt from gme.h:

struct gme_info_t
{
	/* times in milliseconds; -1 if unknown */
	int length;			/* total length, if file specifies it */
	int intro_length;	/* length of song up to looping section */
	int loop_length;	/* length of looping section */
	
	/* Length if available, otherwise intro_length+loop_length*2 if available,
	otherwise a default of 150000 (2.5 minutes). */
	int play_length;

So theoretically, it is correct to stop the song at play_length msecs. However, in gstgme.c, the condition for eos was if (gme_track_ended (gme->player)). This turns out to be either poorly implemented or just misunderstood in the gme library. For looping songs, it only returns true when fading  is enabled. To enable fading, one has to call gme_set_fade(gme->player, fade_time), where fade_time is the time from when to start the fade. The fade duration is 8000 ms, kind of hardcoded in the C api (the C++ gme api allows you to set the fade duration too). But the fading turned out to be unreliable as well. So my final solution was to add 8 seconds length to songs which have loop_length > 0 (they are looping songs), and instead of gme_track_ended() use gme_tell() and compare to total_duration. I also enabled fading, but this time it is not used for determining track end, it is simply there to have some fading out towards the end of the song. I need to mention that it doesn't always work, but at least the songs progress.

I suspect all this is due to a few bugs in libgme. But that's alright, I seem to have worked around it.

So I am attaching the new patch, hopefully this is what you meant by git format-patch format. If not, can you please instruct me how to create a proper patch? I have no idea how to do this without cloning the git repo.
Comment 10 mihai.draghicioiu 2011-04-16 19:42:16 UTC
Created attachment 186098 [details] [review]
Fix endless loop when playing files supported by libvgm, with 8 second fade at the end
Comment 11 mihai.draghicioiu 2011-04-16 19:45:06 UTC
I need to explain what I meant by "I need to mention that it doesn't always work, but at least the songs progress." - I meant that fading doesn't always kick in at the end of the song, especially when doing seeking manually. If you let the song play, it works fine. But in some cases, when seeking, it doesn't work, ie the song is played at full volume until it ends. But this is the best I could get out of the library.
Comment 12 mihai.draghicioiu 2011-04-16 19:56:51 UTC
Created attachment 186108 [details] [review]
 Fix endless loop when playing files supported by libvgm, with 8 second fade at the end (git fornat-patch)
Comment 13 mihai.draghicioiu 2011-04-16 20:25:33 UTC
Created attachment 186110 [details] [review]
   Fix endless loop when playing files supported by libvgm, with 8 second fade at the end (git fornat-patch)

After a bit of testing I made a small bugfix too.
Comment 14 Sebastian Dröge (slomo) 2011-04-17 10:09:17 UTC
The patch looks good, thanks. Could you also file a bug against gme for the gme bugs?
Comment 15 mihai.draghicioiu 2011-04-17 13:16:33 UTC
Thanks, I've found the libgme tracker at http://code.google.com/p/game-music-emu/

I have a few more issues to report though (sorry, just gathering my thoughts here):

1. VGZ files are not supported, and project2612.org has tons of zip files with vgz files inside them. Libgme does not support these directly, only if you ungzip them before you play them. I wouldn't know how to implement this, should I file it in this same section?

2. Support for NSF files is based on nosefart. It has the same looping issue, but this is different gst code - try playing back http://www.zophar.net/download_file/13252 for example. Thing is, libgme also supports nsf files, so the nosefart code might be unneeded. I'll file this as a bug.

3. Support for mime types and default actions in gnome. I guess I'll have to file against totem, rhythmbox and whichever package contains the mime database.

4. Make a set of test files for video game music - is there one already? Maybe for several other audio formats?
Comment 16 mihai.draghicioiu 2011-04-17 13:22:09 UTC
I've noticed that when the nsf plugin is disabled, the gme plugin plays nsf files fine. It reports the length to be 2:30 for the example nsf file I linked to earlier and it stops at about 0:26, after it detects silence. This silence detection is a feature of libgme, and in this way it is better than the nsf plugin. This is just one test, though, with one file and it would need to be tested further, but I assume it would work fine, and the nsf code in gst/nsf/ can be removed later.
Comment 17 Sebastian Dröge (slomo) 2011-04-18 07:33:13 UTC
(In reply to comment #15)
> Thanks, I've found the libgme tracker at
> http://code.google.com/p/game-music-emu/
> 
> I have a few more issues to report though (sorry, just gathering my thoughts
> here):
> 
> 1. VGZ files are not supported, and project2612.org has tons of zip files with
> vgz files inside them. Libgme does not support these directly, only if you
> ungzip them before you play them. I wouldn't know how to implement this, should
> I file it in this same section?

Please report to gme at code.google.com as explained in the other bug

> 2. Support for NSF files is based on nosefart. It has the same looping issue,
> but this is different gst code - try playing back
> http://www.zophar.net/download_file/13252 for example. Thing is, libgme also
> supports nsf files, so the nosefart code might be unneeded. I'll file this as a
> bug.

Is there a reason why the NSF plugin should be removed completely? It has no external dependencies and it might make sense to just lower the rank to secondary and have the gme rank at primary. Let's continue that in the bug you filed

> 3. Support for mime types and default actions in gnome. I guess I'll have to
> file against totem, rhythmbox and whichever package contains the mime database.

shared-mime-info contains the mimetype database but applications need to add the mimetypes to their .desktop file. For shared-mime-info, if some of the mimetypes are missing there, take a look at gst-plugins-base/gst/typefind, should be possible to convert that code into the shared-mime-info xml database.

> 4. Make a set of test files for video game music - is there one already? Maybe
> for several other audio formats?

samples.mplayerhq.hu has some samples and with the help of Google you find lots of game music files. But I don't know of a central place of test files just for video game music
Comment 18 mihai.draghicioiu 2011-04-18 08:58:25 UTC
> > 4. Make a set of test files for video game music - is there one already? Maybe
> > for several other audio formats?

> samples.mplayerhq.hu has some samples and with the help of Google you find lots
> of game music files. But I don't know of a central place of test files just for
> video game music

I was thinking more like a repository for testing all the formats supported by gstreamer, but more practical would be to just gather some video game music to test this part of gstreamer.

I took a look at gst-typefind-0.10 and it seems to do the same as the 'file' utility, but in its own way. So to build the mime-types for the video game music format, i would need this collection (or I could just peek at the source, but I'd rather have the files).

Perhaps a github project with a collection of files would be useful. Or maybe google code? Do you recommend git or svn?
Comment 19 mihai.draghicioiu 2011-04-22 07:31:27 UTC
I've started the chiptune collection project at https://github.com/vampirefrog/chiptunes#readme
Comment 20 mihai.draghicioiu 2011-04-26 06:32:17 UTC
Just as a note: after examining the in_vgm.dll winamp plugin, I saw that it has a plugin settings dialog, where you can choose how many extra times looped files loop (default is 1 - that means the looped section gets played twice). It also has a faulty fade-out that goes beyond the reported track time, so the winamp slider stays at 0 while the song fades out. It also gives the option to loop indefinately, in which case seeking is disabled.

So I should say our solution looks fine in comparison.