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 768576 - New base class for non-streaming audio formats such as module formats, SID music, MIDI files etc.
New base class for non-streaming audio formats such as module formats, SID mu...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-07-08 14:36 UTC by Carlos Rafael Giani
Modified: 2017-05-24 08:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
New nonstreamaudiodecoder base class (99.86 KB, patch)
2016-07-27 00:51 UTC, Carlos Rafael Giani
none Details | Review
Ported WildMidi plugin (58.95 KB, patch)
2016-07-27 00:53 UTC, Carlos Rafael Giani
none Details | Review
New OpenMPT decoder plugin for module files (mod,s3m,xm,it..) (35.31 KB, patch)
2016-07-27 00:56 UTC, Carlos Rafael Giani
none Details | Review
New nonstreamaudiodecoder base class, v2 (105.93 KB, patch)
2017-03-08 22:38 UTC, Carlos Rafael Giani
none Details | Review
Ported WildMidi plugin, v2 (55.67 KB, patch)
2017-03-08 22:39 UTC, Carlos Rafael Giani
none Details | Review
New OpenMPT decoder plugin for module files (mod,s3m,xm,it..), v2 (39.35 KB, patch)
2017-03-08 22:40 UTC, Carlos Rafael Giani
none Details | Review
audio: Add nonstreamaudiodecoder base class (106.55 KB, patch)
2017-05-22 14:33 UTC, Jan Schmidt
committed Details | Review
wildmidi: Port to 1.0 on top of the nonstreamaudiodecoder base class (56.17 KB, patch)
2017-05-22 14:33 UTC, Jan Schmidt
committed Details | Review
openmpt: Add openmptdec element (39.55 KB, patch)
2017-05-22 14:33 UTC, Jan Schmidt
committed Details | Review
wildmididec: explicitly cast buffer data to int8 in _decode() (1.25 KB, patch)
2017-05-22 23:13 UTC, Reynaldo H. Verdejo Pinochet
committed Details | Review

Description Carlos Rafael Giani 2016-07-08 14:36:51 UTC
I wish to propose the inclusion of the GstNonstreamAudioDecoder base class into gst-plugins-bad. It is a base class that was designed for non-streaming audio formats where the audio data needs to be fully loaded first before decoding can start. Examples are: module formats (MOD/S3M/XM/IT for example), C64 SID tunes, video console music files (GYM/VGM/etc.), .mid files.

The purpose of this bugzilla entry is to focus on reviewing the base class itself. Once it is approved for inclusion into -bad, I will also create separate Bugzilla entries with elements that are based on it. As part of this, the existing sidplay element in -ugly will become obsolete once a nonstream-audio based sidplayfp element is ready to be used. (The element in -ugly is based upon an ancient sidplay version, and it is not recommmended to use this old library.)

Future nonstream-based elements will also replace the modplug element in -bad with the libopenmpt-based openmptdec element (OpenMPT itself is based on the original ModPlug tracker and features many improvements over MPT). gmedec will also be replaced.
Comment 1 Carlos Rafael Giani 2016-07-08 14:38:21 UTC
Hmm, somehow the browser dropped parts of my text. Here are the missing bits:

The code can be found here: https://github.com/dv1/gst-nonstream-audio . The base class is in the gst-libs/gst/audio/ subdirectory (following GStreamer directory conventions).
Comment 2 Sebastian Dröge (slomo) 2016-07-08 14:41:04 UTC
Please attach it as patches for proper review, and ideally next to the base class also have patches that port existing elements or add new elements so that there's an example for the usage.
Comment 3 Carlos Rafael Giani 2016-07-08 14:42:49 UTC
Well okay, I thought that just looking it up in github would be a tad more convenient for now. But there is no problem in sending a patch :) I can also add the openmptdec here already.
Comment 4 Carlos Rafael Giani 2016-07-11 13:36:41 UTC
I forgot about one open question with this base class : how to communicate the list of subsongs to the user.

With old video game formats, one file (= one song) can have one or more subsongs. Subsongs are subsections within the song, which are fully contained; that is, you cannot just "play the big song", you always choose one subsong.

My original idea was to use GstToc to announce the list of available subsongs. Entry types would be either TITLE or TRACK.

However, there are a few problems with GstToc for subsongs:

1) Subsongs may not have a defined length. Some formats do not have length information, and may just end at some point. These would need support in GstTock for entry length -1 (= GST_CLOCK_TIME_NONE). But -1 is currently considered an invalid length.

2) As a variation of (1), they may loop internally without letting the outside world know that looping is going on. In this case, all that can be done is to either let it run indefinitely, or choose some arbitrary cutoff period (like, 5 minutes). Either way, the fact that this format loops internally would somehow have to be communicated to the application.

3) Neither TITLE nor TRACK fully fit the definition of a subsong.

4) There is currently no way to accurately control the looping via GstToc. GstNonstreamAudio not only allows for switching the looping on and off, it also allows to set the exact number of loops (or set infinite looping), and supports two looping modes: STEADY and LOOPING. In the STEADY mode, the playback position will continue beyond the song's duration. So, a 5 minute song will continue with positions 5:01, 5:02 ... after the first loop. LOOPING however will do non-flushing seeks to actually stay within the duration. And, not all formats support the LOOPING mode.


The fundamental problem I see is that GstToc just wasn't made for subsongs and their characteristics. It was made mostly for CDs and DVDs. So, the other option would be to create a new type of event, message, and type to communicate subsong information to the application. This comes with its own list of issues, most notably that applications would have to add explicit support for this "subsong info type". On the other hand, applications would have to be aware of the nature of subsongs anyway to be able to control them properly.

Opinions?
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2016-07-11 13:52:42 UTC
1) I think extending the TOC to allow duration = GST_CLOCK_TIME_NONE makes sense.

2) I am not sure if we need to communicate the fact that the song loops. If you run audiotestsrc it just beeps indefinitely and it could be considered that it loops the waveform cycle.

3) I'd would use separate titles for each subsong. I think we still need to update audio players to support those (e.g. totem only seems to support chapters).

4) the app will have to do the looping

Regarding GStToc. I think we should add a section with recommendations. E.g. document how to interpret a GstToc on videos and how to interpret it on music.
Also the GstTocEntryType need more guidance what they are.
Comment 6 Carlos Rafael Giani 2016-07-11 14:11:50 UTC
2) The number of loops do sound like something that advanced users might be interested in controlling. Same for a cutoff period. I'd like to at least offer applications the chance to control this.

3) Not even chapters showed up in my tests with totem, making me wonder how well GstToc is supported.

4) This is not an option. With several formats the looping has to be done precisely, otherwise small gaps and small errors in the current position can occur and accumulate. Some formats also allow internal looping but do not support seeking (some Amiga music rips have this problem).

I do agree that GstTocEntryType needs to more guidance.
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2016-07-11 18:59:09 UTC
Looping is normally done through segmented seek. If it is implicitly done by the element, then it just appears as a indefinite stream. And since for most formats even the element does not know much about the loops I don't see what we can practically do.

Re cutoff time, we could make this a property on the element, but it's kind of moot, since no generic media player will handle it :/

So one 'solution' that I mentioned before is to run auto-correlation (with a relative large window like 1 sec) to detect the loop and emit EOS. For this you'd need to introduce a large internal buffer and with that also 1 sec latency. You could even fade-out over the buffer when the loop is detected and emit EOS then.
Comment 8 Carlos Rafael Giani 2016-07-11 19:12:22 UTC
I think the key word here is "generic media player". The concept of subsongs is already far from generic. All in all, the idea of a custom type/event/message is starting to appear more favorable. Generic players would still play the first subsong, which usually is the "main" subsong.

There is a partial alternative: some players fill their playlists with N entries, one for each subsong. I do not know how this would work with autoplugging and typefinding, but it would make a TOC unnecessary.
Comment 9 Carlos Rafael Giani 2016-07-27 00:51:37 UTC
Created attachment 332182 [details] [review]
New nonstreamaudiodecoder base class

Here is a patch with the new GstNonstreamAudioDecoder base class.
Comment 10 Carlos Rafael Giani 2016-07-27 00:53:21 UTC
Created attachment 332183 [details] [review]
Ported WildMidi plugin

Here is a patch which reimplements the wildmidi plugin on top of the new base class. The old wildmidi plugin was dead 0.10 code and is also removed in this patch.
Comment 11 Carlos Rafael Giani 2016-07-27 00:56:50 UTC
Created attachment 332184 [details] [review]
New OpenMPT decoder plugin for module files (mod,s3m,xm,it..)

This introduces a new "openmpt" plugin with one "openmptdec" element. It uses the libopenmpt library (http://lib.openmpt.org/libopenmpt/) for decoding module music like MOD, XM, S3M, IT, MTM etc. files. OpenMPT is a base on the old ModPlug tracker, and features many improvements over the original.
Comment 12 Carlos Rafael Giani 2017-03-08 22:38:53 UTC
Created attachment 347510 [details] [review]
New nonstreamaudiodecoder base class, v2

Update to the base class for additional subsong mode property
Comment 13 Carlos Rafael Giani 2017-03-08 22:39:52 UTC
Created attachment 347511 [details] [review]
Ported WildMidi plugin, v2

Updated to apply to the latest git master
Comment 14 Carlos Rafael Giani 2017-03-08 22:40:44 UTC
Created attachment 347512 [details] [review]
New OpenMPT decoder plugin for module files (mod,s3m,xm,it..), v2
Comment 15 Jan Schmidt 2017-05-22 14:33:38 UTC
Created attachment 352359 [details] [review]
audio: Add nonstreamaudiodecoder base class
Comment 16 Jan Schmidt 2017-05-22 14:33:45 UTC
Created attachment 352360 [details] [review]
wildmidi: Port to 1.0 on top of the nonstreamaudiodecoder base class
Comment 17 Jan Schmidt 2017-05-22 14:33:52 UTC
Created attachment 352361 [details] [review]
openmpt: Add openmptdec element
Comment 18 Jan Schmidt 2017-05-22 14:37:40 UTC
Thanks! Committed with some minor changes (fixing typos, updating the meson build file, support for openmpt 0.3.0)
Comment 19 Reynaldo H. Verdejo Pinochet 2017-05-22 23:13:57 UTC
Created attachment 352384 [details] [review]
wildmididec: explicitly cast buffer data to int8 in _decode()

_decode() seems to use the wrong cast for the buffer's data.
At least my system's wildmidi-lib expect it explicitly as a
signed 8bit int (Debian's libwildmidi-dev 0.4.0-2+b2). Patch\
ttached.
Comment 20 Tim-Philipp Müller 2017-05-23 07:46:46 UTC
Comment on attachment 352384 [details] [review]
wildmididec: explicitly cast buffer data to int8 in _decode()

Got the same, so pushed this.

commit bbb89dd34c29b4ada9988a4913ef1d1c32161a1f
Author: Reynaldo H. Verdejo Pinochet <reynaldo@osg.samsung.com>
Date:   Mon May 22 15:52:33 2017 -0700

    wildmididec: explicitly cast buffer data to int8 in _decode()
    
    Fixes compiler warning introduced in commit ff32a4297:
    
    gstwildmididec.c:637:47: error: pointer targets in passing argument 2 of ‘WildMidi_GetOutput’ differ in signedness
           WildMidi_GetOutput (wildmidi_dec->song, (char *) (info.data), info.size);
                                                   ^
    wildmidi_lib.h:106:15: note: expected ‘int8_t * {aka signed char *}’ but argument is of type ‘char *’
     WM_SYMBOL int WildMidi_GetOutput (midi *handle, int8_t *buffer, uint32_t size);
    
    https://bugzilla.gnome.org/show_bug.cgi?id=768576
Comment 21 Tim-Philipp Müller 2017-05-23 07:47:37 UTC
On a side note, the namespace gst_nonstream_audio_*() will be awkward for bindings / g-i.
Comment 22 Jan Schmidt 2017-05-23 21:40:09 UTC
I'm not sure of a better name, but we have a little while to change it.

Fedora 25 only has wildmidi 0.3.9, so might put back support for that
Comment 23 Carlos Rafael Giani 2017-05-24 08:37:24 UTC
What is awkward about the name? The underscores?
Comment 24 Tim-Philipp Müller 2017-05-24 08:46:28 UTC
GstAudio module would expect a gst_audio_* prefix for the function names and then expose only the remainder.
Comment 25 Carlos Rafael Giani 2017-05-24 08:47:58 UTC
Ah, so, namespace-style conventions. Something like gst_audio_nonstream_*() then? Sounds like something that can be done with several sed commands.