Skip to content
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

Serato: Import Beatgrid from metadata #2958

Merged
merged 42 commits into from
Aug 16, 2020

Conversation

Holzhaus
Copy link
Member

@Holzhaus Holzhaus commented Jul 21, 2020

I finally found a few hours to implement parsing of the Serato Beatgrid. This already works, but nothing is imported yet, because we have the same problem as we had with cues, i.e. we can't determine the timing offset at the time when the metadata is read. We need to find a nice workaround for that.

  • Add more tests
  • Actually import the beatgrid while taking timing offsets into account

Support for tag types:

  • ID3v2
  • FLAC/Xiph
  • Ogg/Xiph Format differs significantly, no idea how to parse this.
  • MP4

EDIT: It works! here are two tracks with their beatgrids imported from Serato (for demo purposes I added some tempo changes to the upper track's beatgrid)
2020-07-22-172204_1231x378_scrot

@Be-ing
Copy link
Contributor

Be-ing commented Jul 21, 2020

I think this should go to 2.3, not master.

@Holzhaus
Copy link
Member Author

I think this should go to 2.3, not master.

This is based on 2.3, so you can simply change the target branch. I wan't sure about it, because we're already in feature freeze.

@Be-ing Be-ing changed the base branch from master to 2.3 July 21, 2020 16:42
@Be-ing
Copy link
Contributor

Be-ing commented Jul 21, 2020

I consider this finishing an incomplete feature.

@Holzhaus
Copy link
Member Author

Holzhaus commented Jul 21, 2020

Btw, I added the current state of Serato Metadata support in Mixxx here: https://github.com/mixxxdj/mixxx/wiki/Serato-Metadata-Format#mixxx-support-status

@Holzhaus Holzhaus added this to the 2.3.0 milestone Jul 21, 2020
@Holzhaus Holzhaus force-pushed the serato-beatgrid-import branch from 83517fa to dd325f2 Compare July 21, 2020 17:49
@Holzhaus Holzhaus marked this pull request as ready for review July 22, 2020 15:24
@Holzhaus Holzhaus requested a review from uklotzde July 22, 2020 16:29
@Holzhaus Holzhaus requested a review from daschuer July 23, 2020 15:41
@Holzhaus
Copy link
Member Author

Holzhaus commented Aug 2, 2020

Any comments?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR, I have left some comments.

src/sources/soundsourceproxy.cpp Show resolved Hide resolved
src/sources/soundsourceproxy.cpp Show resolved Hide resolved
src/track/beatsimporter.h Outdated Show resolved Hide resolved
src/track/serato/beatgrid.cpp Outdated Show resolved Hide resolved
src/track/serato/beatgrid.cpp Outdated Show resolved Hide resolved
src/track/serato/beatgrid.cpp Outdated Show resolved Hide resolved
// For now, we only read it to be able to dump the exact same byte sequence
// later on.
quint8 footer;
stream >> footer;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine Serato uses it as "dirty" flag. I am afraid we need to toggle it as well in case we alter the beat grid in Mixxx. Can you verify it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can check if it new values are only picked up by Serato if that value is different (if that's what you mean). However, Serato is also doing strange stuff elsewhere (e.g. they sometimes append a random byte for base64 encoded flac metadata). My guess was that they have a messed up length check and write a byte of uninitialized memory :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems also be likely, in particularly if it is not constantly increasing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not increasing.

src/track/serato/beatgrid.cpp Show resolved Hide resolved
src/track/taglib/trackmetadata_common.cpp Show resolved Hide resolved
src/track/track.cpp Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member Author

@daschuer Thanks for reviewing. As you're the only reviewer, you can hit merge :D

@daschuer
Copy link
Member

OK, I have not hit merge because not all Todos are checked.

@daschuer
Copy link
Member

CI is also not finished.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, for the delay. Just some minor issues.

src/track/beatsimporter.h Outdated Show resolved Hide resolved
src/track/beatsimporter.h Show resolved Hide resolved
src/track/serato/beatgrid.h Outdated Show resolved Hide resolved
src/track/serato/beatgrid.h Outdated Show resolved Hide resolved
src/track/serato/beatgrid.h Outdated Show resolved Hide resolved
src/track/serato/beatgrid.h Outdated Show resolved Hide resolved
src/track/serato/beatgrid.h Outdated Show resolved Hide resolved
src/track/track.cpp Outdated Show resolved Hide resolved
src/track/track.cpp Outdated Show resolved Hide resolved
@Holzhaus Holzhaus requested a review from uklotzde August 16, 2020 10:03
src/track/beatsimporter.h Outdated Show resolved Hide resolved
src/track/track.cpp Outdated Show resolved Hide resolved
@Holzhaus Holzhaus requested a review from uklotzde August 16, 2020 10:28
@uklotzde
Copy link
Contributor

Verified locally. LGTM

@uklotzde uklotzde merged commit de8cee1 into mixxxdj:2.3 Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants