lp1445885: Fix handling of files with wrong suffix #4615
Conversation
src/sources/soundsource.cpp
Outdated
| const QString fileSuffix = fileInfo.suffix().toLower().trimmed(); | ||
|
|
||
| if (fileSuffix == "opus") { | ||
| // opus has mime type "audio/ogg" which will be decoded with the SoundSourceOggVobis() |
There was a problem hiding this comment.
I think this is because "ogg" is a container and "vorbis" is the compression algorithm.
There was a problem hiding this comment.
Yes, the OGG container can include Speex, Vorbis, Opus, FLAC, OggPCM.
The Ogg magic numbers are:
OggS...........v.....v......vorbis is Vorbis
OggS.............l..........Opus is Opus
fLaC..."is the magic number for native FLAC. I think we do not support Ogg FLAC which will probably also start with OggS
There was a problem hiding this comment.
I think this is an improvement so I'm approving it. But I think much like the old code assumed "file extension == codec", this code assumes "mimetype == codec", which is not the case, which is why there is the hacks needed with vorbis vs opus. I would prefer a cleaner solution that first detects the mimetype, and then supports multiple compression algorithms for the ogg mimetype.
src/sources/soundsourceproxy.cpp
Outdated
| if (fileType == "opus") { | ||
| // opus has mime type "audio/ogg" which will be decoded with the SoundSourceOggVobis() | ||
| // but we want SoundSourceOpus() | ||
| return {}; |
There was a problem hiding this comment.
this seems a little hacky, is there a way we can detect files as audio/ogg and then later decide if they are vorbis or opus? It's not a bug that opus shows up as an ogg mimetype, it's just that mimetype is not sufficient to determine the codec, much like .avi can contain many codecs. (https://datatracker.ietf.org/doc/html/rfc7845)
There was a problem hiding this comment.
We may create a joint sound source for opus and vorbis files. Here we just work around the limitation of QMimeDatabase() because we currently need both, the mime type + the extension to select the soundsource.
|
Currently we rely on the correct file extension in case of "audio/ogg" files. I can imagine to open the file ourselves and check the stream content. Is this the type of cleaner solution you have in mind? While it would be correct to have a file type vorbis and a file type opus, I think this may confuse users, because ogg vorbis files are called "ogg" files. What do you think? |
|
Yeah I see. My preference would be to avoid the hack where we say that the opus filetype has no mimetype (in mimeTypesForFileType). This means that the function is broken by design, and code elsewhere needs to understand what to do because of that hack. I'd prefer mimeTypesForFileType return the correct values and the correction be done elsewhere (in registerProviders for instance) |
|
mimeTypesForFileType returns now "audio/x-opus+ogg" even though the lookup is bypassed. This would be a prerequisite if we decide to implement our own magic number lookup. |
|
How is this done for other container formats like .wav ? |
|
This is for example the case for RF46 wav files which are not in the magic number database |
|
There are conflicts now |
|
Conflicts resolved. |
There was a problem hiding this comment.
Thank you. Could be merged as is, only some minor remarks.
I just noticed that we could use QLatin1String instead of QStringLiteral at many places, especially for comparisons. Qt's string handling with their internal UTF-16 encoding adopted from Java is super inconvenient and confusing. It causes a lot of boilerplate code with the risk for inconsistencies due to the implicit overloads.
src/sources/soundsource.cpp
Outdated
| const QMimeType mimeType = QMimeDatabase().mimeTypeForFile( | ||
| const QString fileSuffix = fileInfo.suffix().toLower().trimmed(); | ||
|
|
||
| if (fileSuffix == "opus") { |
There was a problem hiding this comment.
Minor nit-pick: QLatin1String("opus")?
QString provides many QLatin1String overloads as I only recently noticed.
| } | ||
| static const QStringList& getSupportedFileNamePatterns() { | ||
| return s_supportedFileNamePatterns; | ||
| } | ||
| static const QRegularExpression& getSupportedFileNamesRegex() { | ||
| return s_supportedFileNamesRegex; | ||
| } | ||
| static QString getFileTypeByMimeType(const QMimeType& mimeType) { |
There was a problem hiding this comment.
The naming of the function and member variable is slightly inconsistent with getFileSuffixesForFileType(). It shouldn't matter that one returns a single item while the other returns multiple items.
src/sources/soundsourcesndfile.cpp
Outdated
| if (supportedFileExtension.startsWith(QStringLiteral("aif")) || | ||
| supportedFileExtension == QLatin1String("wav")) { | ||
| const QString& supportedFileType) const { | ||
| if (supportedFileType.startsWith(QStringLiteral("aif")) || |
There was a problem hiding this comment.
Already inconsistent in existing code: We should use either QStringLiteral or QLatin1String. Preferably the latter if it suffices.
Bypass mime type lookup from content for .flac files
|
Done! |
This continues the closed #4608 from @uklotzde
I have implemented now the solution with a s_fileTypeByMimeType QHash.
We have now a strong 1 to n relationship where one FileType is a replacement for n MimeTypes.
Unfortunately this did not work for opus files, because they are detected as audio/ogg I have added a spacial case for opus files.
I have also removed the space is the getTypeFromMissingFile test, because QT does not detect such files. I consider this case as to exotic to not use the QMemeTypeDatabase for this.
Existing files with spaces are working.