Fix reading metadata for some file extensions #13218
Open
+181
−110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ough the call stack to avoid guessing it from the file extension.
No longer guess the fileType from the file extension.
acolombier
reviewed
May 8, 2024
Co-authored-by: Antoine Colombier <7086688+acolombier@users.noreply.github.com>
acolombier
reviewed
May 24, 2024
src/test/soundproxy_test.cpp
Outdated
| @@ -1086,3 +1087,14 @@ TEST_F(SoundSourceProxyTest, freeModeGarbage) { | |||
| break; | |||
| } | |||
| } | |||
|
|
|||
| TEST_F(SoundSourceProxyTest, taglibStringToEnumFileType) { | |||
| for (const auto& fileType : SoundSourceProxy::getSupportedFileTypes()) { | |||
|
All green now. |
Swiftb0y
reviewed
May 25, 2024
| @@ -82,7 +83,7 @@ double SeratoTags::guessTimingOffsetMillis( | |||
| // PR for more detailed information: | |||
| // https://github.com/mixxxdj/mixxx/pull/2119 | |||
| double timingOffset = 0; | |||
| if (taglib::getFileTypeFromFileName(filePath) == taglib::FileType::MP3) { | |||
| if (fileType == "mp3") { | |||
There was a problem hiding this comment.
QStringLiteral, otherwise implicitly converts
Comment on lines
+66
to
+68
| // This table is aligned with detectByExtension() in fileref.cpp | ||
| constexpr std::array lookupTable = { | ||
| TypePair{"mp3"_L1, FileType::MPEG}, |
There was a problem hiding this comment.
one day we should think about introducing a "minimal perfect hash function"-based approach for LUTs like these.
There was a problem hiding this comment.
you'll also need to make it static, otherwise the compiler is more likely to choose to create it at runtime.
Suggested change
| // This table is aligned with detectByExtension() in fileref.cpp | |
| constexpr std::array lookupTable = { | |
| TypePair{"mp3"_L1, FileType::MPEG}, | |
| // This table is aligned with detectByExtension() in fileref.cpp | |
| constexpr static std::array lookupTable = { | |
| TypePair{"mp3"_L1, FileType::MPEG}, |
Comment on lines
-17
to
+14
| taglib::FileType fileType) | ||
| const QString& fileType) | ||
| : m_fileName(fileName), | ||
| m_fileType(fileType) { | ||
| m_fileType(taglib::stringToEnumFileType(fileType)) { |
There was a problem hiding this comment.
whats the advantage of taking a string here instead of shifting the conversion duty to the caller? This smells stringly-typed
There was a problem hiding this comment.
fyi, two of the other three comments were redundant if this would avoid strings better.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This replaces all file type guessing code form the file extension, by using the normalized file type stored in the Track object.
This makes sure all parts are using the same file type, even if the extension is wrong.
This fixes #13205