audacious-media-player
Replies: 10 comments 18 replies
-
|
As a test-case, if adding "rad" extension to any Audacious input plugin other than AdPlug, the problem with "skychase.rad" can be reproduced. |
Beta Was this translation helpful? Give feedback.
-
|
Oh well, the code path in libaudcore/probe.cc that handles plugin selection by file content doesn't allow seeking to file end because of limited buffering. No surprise that Adplug plugin fails then. Any other plugin that also takes a look at the entire file during format detection/recognition would be affected, too, though. https://github.com/audacious-media-player/audacious/blob/master/src/libaudcore/probe.cc#L154 |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the quick analysis, Michael.
Confirmed, I adjusted the VTX plugin for example like this: diff --git a/src/vtx/vtx.cc b/src/vtx/vtx.cc
index c4655fed9..b3b00b65d 100644
--- a/src/vtx/vtx.cc
+++ b/src/vtx/vtx.cc
@@ -61,7 +61,7 @@ static int freq = 44100;
static int chans = 2;
static int bits = 16;
-const char *const VTXPlugin::exts[] = { "vtx", nullptr };
+const char *const VTXPlugin::exts[] = { "rad", "vtx", nullptr };
bool VTXPlugin::is_our_file(const char *filename, VFSFile &file)
{@jlindgren90: Could you share your expertise here please? Such low-level things are outside my state of knowledge, thanks in advance. What I also don't understand yet: The file size of the example is just 10 KB, way below the mentioned 256 KB. My assumption then would be that the limit would not matter at all here. audacious/src/libaudcore/vfs.h Lines 156 to 159 in bd66a08 |
Beta Was this translation helpful? Give feedback.
-
|
The probe-buffering scheme is intended to perform only sequential reads on the underlying FD which may not even be seekable (think stdin). Maybe we can loosen the restriction and allow some seeks if we know in advance that it's a small file. I need to think about it more. |
Beta Was this translation helpful? Give feedback.
-
|
See if this works: jlindgren90@fe866ca I don't have adplug dependencies installed currently and so ran out of time to test it (need to get to bed), sorry. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks John, it works for me here with the small Why is the ffaudio plugin not affected though? It claims to support @mschwendt: Do you think it makes sense to create an issue from this discussion? |
Beta Was this translation helpful? Give feedback.
-
|
Most modern audio formats (like .ogg and .opus) are designed to be able to be identified, and even start playback, without seeking to the end of the file. |
Beta Was this translation helpful? Give feedback.
-
|
(The probe buffer implementation has supported seeking in general for over a decade. Just not to the end of the file.) |
Beta Was this translation helpful? Give feedback.
-
|
Most old audio formats also feature some sort of header structure that contains a short ID and even a version field. Enough to identify a file. The culprit is the modern implementations of backend libraries that add safety measures during file identification already. They want to reject damaged files as well as files they cannot handle yet. Some of the libs don't offer file identification in a separate method that would only take a peek at the beginning of a file, but they evaluate the entire file in search of (1) abnormal or corrupted data that might cause a malfunction during playback, (2) documented deviations of a base file format that are not reflected in the file header, or (3) they run a file through a silent/dummy decoder in order to determine the playtime and to see what data loading/parsing errors might turn up. As such the problem isn't so bad and is limited to file format libs that seek to end as part of the is_our_file() step. Reality Adlib Tracker is one of them. Various other ".MOD"-like decoders likely do something similar, since particularly the ".MOD" format is about a family of formats that cannot be detected safely by just looking at the begining of a file. And MOD-like input plugins for Audacious there are multiple, so on the list of supported filename extensions there is more than one plugin for several file types. OpenMPT, ModPlug, XMP, AdPlug, FC, there's quite some overlap. Curiously, if there's only one input plugin for a filename extension, the is_our_file() call is skipped. Trust in file extension based recognition is high in that case, since the input plugin may reject the file during the attempt to play it. On the contrary, if it's two or more "competing" plugins that advertize handling a file ext, a plugin MUST explicitly confirm that it recognizes a file. That the XMP plugin also declared support for ".RAD" files was a mistake, possibly for years. Currently libxmp doesn't handle such files. https://github.com/libxmp/libxmp/blob/master/src/format.h#L72 Maybe probe.cc could be enhanced in some way to increase trust in the plugin with the highest priority, if multiple plugins claim to support the same file ext? |
Beta Was this translation helpful? Give feedback.
-
|
Makes a lot of sense. As far as increasing "trust" in one decoder if there are two or more - one issue is that we still have to be able to seek back to the start of the file between decoders, even if the underlying stream doesn't support this (e.g. stdin, or streaming). That's what the probe buffer accomplishes. If we allow the first/highest decoder to read farther than 256 KB (the current limit) into the file, then we have to buffer more so that the 2nd decoder can read that same data again. There are also over-aggressive decoders that will keep reading for a long time looking for their "magic bytes". These can introduce significant delays if we don't cut them off, especially on live broadcast streams. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, it's just academic. Sort of. Increasing trust could work like this:
The selected plugin may still fail when playback starts, but that can happen also if there's only one plugin to choose from. Example: A single input plugin claiming to handle ".WAV" containers but not actually handling all encoded data that may be found within them. Of course, plugin priorities are secretly hardcoded so far and cannot be adjusted via the GUI. But currently it's a hefty penalty for input plugins that they MUST pass the is_our_file() probing step (with its buffer seeking limits) only because there's one or more competitive plugins. Each of them MAY be able to handle the input during playback because buffer limits are lifted then, but possibly fails only during probing because of seeking limits. |
Beta Was this translation helpful? Give feedback.
-
That would break the feature where user can choose between multiple plugins via disabling/enabling them in the settings.
No. That method is skipped for the example I've given. A single ".WAV" input plugin is selected directly (i.e. without a call to is_our_file) any may or may not be successful during the play() step. |
Beta Was this translation helpful? Give feedback.
-
|
If sndfile isn't able to handle all .wav files, then we should add the extension to ffaudio as well. That's how it's supposed to work. |
Beta Was this translation helpful? Give feedback.
-
Okay - then just make the is_our_file method of the higher priority plugin always return true (if the extension matches). That increases "trust" in that plug-in without breaking content-based probing for unrelated plugins/formats. |
Beta Was this translation helpful? Give feedback.
-
If it's a "hefty" penalty then the plug-in is doing probing wrong. is_our_file is supposed to be a quick, simple check. Ideally no more than checking a few bytes at the beginning of the file. The probe limits were put in place partly to catch misbehaving plugins. A well written is_our_file method should not be hitting the 256 KB limit in the first place. |
Beta Was this translation helpful? Give feedback.
-
This is already the case, see https://github.com/audacious-media-player/audacious-plugins/blob/f98424c418203c5e6bc286a0fd0a232f34963175/src/ffaudio/ffaudio-core.cc#L687-L688.
So would you then recommend to remove the "ogg", "oga" and "opus" extensions from ffaudio? And how would you proceed with #1583, John? When adding "audio/aac" like Michael suggested, I would do the same for Opus and MP3 so all common codecs for online streams are covered. diff --git a/src/ffaudio/ffaudio-core.cc b/src/ffaudio/ffaudio-core.cc
index cfe2e60f7..d203f66d7 100644
--- a/src/ffaudio/ffaudio-core.cc
+++ b/src/ffaudio/ffaudio-core.cc
@@ -711,7 +711,10 @@ const char * const FFaudio::exts[] = {
const char * const FFaudio::mimes[] = {
"application/ogg",
+ "audio/aac",
"audio/mp4",
+ "audio/mpeg",
"audio/ogg",
+ "audio/opus",
nullptr
}; |
Beta Was this translation helpful? Give feedback.
-
|
#1583 is user error, the required plugin was disabled. I do not think we need to change anything for that. I would only add extensions to ffaudio if there is not a better plugin available that can handle all (reasonable) files using that extension. |
Beta Was this translation helpful? Give feedback.
-
|
Hmmm. Again, it boils down to a matter of trusting plugins. Just like with the file ext based choosing of plugins.
Assuming that both plugins do handle the content actually, which is why they advertize the MIME type(s), no harm would be caused if choosing either plugin. Or choosing the first match on the list. I could imagine sorting the list of plugins "Matched by MIME type" by priority (a different priority value than the more global one that is available already may not be needed). Giving the AAC high priority for audio/aac type, if it's the "required" (= preferred?) plugin for that content. |
Beta Was this translation helpful? Give feedback.
-
It is not really a matter of trust, it is a matter of different formats sharing the same file extension or MIME type. We need is_our_file to distinguish those. And ffaudio is a special case, it can handle most formats but we have better plugins for some that will handle them better. That is why ffaudio is the lowest priority - it is a fallback. |
Beta Was this translation helpful? Give feedback.
-
|
I would like to know what problem we are trying to solve here. #1583 is not an issue in a default configuration and was only created by a user disabling plugins indiscriminately. We can't foresee or prevent every user action that would cause a malfunction. |
Beta Was this translation helpful? Give feedback.
-
Potentially, Audacious would benefit from getting more hints on which plugins to try instead of performing a stab in the dark and trying all installed plugins. Such as if server says content-type is "audio/acc", Audacious would start probing those plugins that potentially handle "audio/aac" (like ffaudio), whether it's full support for all AAC can do or just a subset. Whether ffmpeg handles all related to AAC, I don't know. FAAD2 (as needed for the AAC plugin) has not been included in Linux distributions for years due to patent concerns. And then "AAC (Raw) Decoder" plugin isn't available by default. Yet ffmpeg-free would be available and can handle the stream in #1583 for example. Provided that Audacious selects ffaudio eventually after probing other plugins with priority, which takes time. In the worst case, it chooses madplug by mistake, because stream content probing isn't THAT safe apparently. Madplug then starts playing noise and running into lots of errors: Or to put it differently, if a plugin explicitly declares it supports a variety of file name extensions other than ".aac" and possibly some MIME types other than "audio/aac", why probe it nevertheless? |
Beta Was this translation helpful? Give feedback.
-
|
I assume you are talking about Fedora. Neither Debian nor Arch Linux seem to have any concerns with distributing FAAD2.
With no plugins available declaring support for handle audio/aac nor the .aac extension, we try all of them as a last-ditch effort. It is essentially the same case as a file with no extension nor MIME type at all. Since Fedora is not shipping the aac plugin, it would probably make sense to add the audio/aac MIME-type and .aac extension to ffaudio in that case, as the next-best alternative. I think it is fine to do that unconditionally. On systems which do have FAAD2, the aac plugin should remain higher priority and be preferred. |
Beta Was this translation helpful? Give feedback.
-
|
Yes, that makes sense. Added with audacious-media-player/audacious-plugins@6fcde6b. |
Beta Was this translation helpful? Give feedback.
-
|
Ubuntu, Red Hat via EPEL, openSUSE, likely more. openSUSE builds without faad by default, line 22: AAC patents will expire before 2030 or so, but I've not followed it closely. It's been like that with MP3 and portions of the codecs in ffmpeg, too. The split out plugins have always been available for Fedora (and EPEL I guess), but only in 3rd party repositories like RPM Fusion and its predecessors (run by individuals or as a community project). |
Beta Was this translation helpful? Give feedback.
-
|
According to Wikipedia this has changed recently and Fedora ships "ffmpeg-free" with AAC support by default now. |
Beta Was this translation helpful? Give feedback.
-
Yes, since February 2023. I've mentioned ffmpeg-free before, btw. It is used by default for building the ffaudio plugin, which also has returned from RPMFusion to Fedora and can be swapped with ffmpeg packages from RPMFusion and vice versa. It's not a bad solution, but requires special commands to permit the package manager to erase and replace packages. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
This is a followup to #1579 where an example file "skychase.rad" isn't recognized here.
My debugging had started with a look at the verbose output from Audacious, and I noticed that the underlying file format detection method failed doing an VFS_SEEK_END before getting the file size:
DEBUG ../src/libaudcore/vfs.cc:171 [fseek]: <0x7f6998001230> seek to 0 from endDEBUG ../src/libaudcore/vfs.cc:181 [fseek]: <0x7f6998001230> seek failed!Other seek types like VFS_SEEK_SET are not affected. Debugging it further, it turned out that this failed seek only happens if multiple input plugins advertize supporting a file extension:
DEBUG ../src/libaudcore/probe.cc:120 [aud_file_find_decoder]: Matched 2 plugins by extension.With the example file here it's the XMP plugin that also has "rad" on its list of file extensions. If disabling (or uninstalling) the XMP plugin, only the AdPlug plugin remains as one that supports "rad" files, and the VFS_SEEK_END then succeeds.
So, conclusively, when a plugin's is_our_file method is called with a VFSFile fd, an fseek to VFS_SEEK_END fails, if Audacious has found more than one plugin that potentially can handle the file.
Beta Was this translation helpful? Give feedback.
All reactions