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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add simple support for STEM files #13044
Add simple support for STEM files #13044
Changes from 1 commit
bdfffb4
95155b3
c853c6b
ddbc69a
23d7866
79614a0
f7dbac4
76b12c7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we need apply the mime type logic. And probably the checking for easy "stem" properties in addition.
Alternative this may look for these properties only and we can call the function isMp4AStem() or such.
This raises the general question if this Class can be only used with MP4s or if we need to protect every invocation to be called with other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK, mime type isn't available in the Track object. Reading the file (to detect the MIME using
QMimeDatabase::mimeTypeForFile
will likely have effect on performance. of every single file being imported, which is likely not negligible when importing a large library.I'm not sure why we would want to support another file type. The STEM atom is part of the NI standard, meaning it needs to be in a mp4 file. If we want to implement other STEM standards, there is no guarantee that there will use a STEM atom, as part of the MPEG standard. My suggestion would be to keep the implantation to what we know and support currently (NI standard, mp4) and cross the bridge to support other standards when we decide to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... will likely have effect on performance. of every single file being imported, which is likely not negligible ...
We currently already support playing mp3s with wav extension for instance. I like to have the same for stems if that is reasonable.
I am not sure if the library scanner does also use mine types, need to check. At least we should treat all the files similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that that detection based on
stem.mp4
extension is overly restrictive. At the extreme any iso media container could be feature tested formoov.udta.stem
, but at the least we should be able to detect stems in arbitrary.mp4
containers.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I was simply implementing the specification (which I believe @dszakallas also had access to). Happy to go beyond what it defines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not expand this PR by general MP4 support. Scope of this PR is the STEMs format and nothing beyond!
Traktor Pro seems to have another detection logic than just checking for .stems.mp4 extension, because it seems to work also with the extension .stems.m4a (or they just check for one of both file extensions).
Performance for file type detection is important in my opinion. What would it cost to probe for moov.udta.stem ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already support mp4 some way, as I was able to drop mp4 tracks into mixxx and play them. And the specification says that the container should be an ISOBMFF with
.mp4
as extension..stem.mp4
is "preferred", not required. So, if we want to follow the spec we should try to detect stems in any mp4.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's hard to assess as the atom offset isn't static and require to traverse boxes in the file.
That's true but in the meantime, Mixxx aims to be vendor-agonistic IMO, so the flexibility that Tracktor Pro gives to arrange NI shouldn't drive Mixxx implementation. Anyway, last commit adds support for
.mp4
, but happy to remove depending where the consensus goes.