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.

Sign up for GitHub

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

Jump to bottom

Add FIXMEs for use of bpm.value() without checking #4165

Merged
merged 1 commit into from
Aug 1, 2021
Merged

Add FIXMEs for use of bpm.value() without checking #4165

merged 1 commit into from
Aug 1, 2021

Conversation

mr-smidge
Copy link
Contributor

This PR does two things:

  • Makes the Engine Prime export more resilient to any occasion where cue points or the beat grid are not properly set up for a given track - this led to assertion violations during a whole-library export.
  • Marks with FIXME statements any instances in the code where I could see a call to Bpm::value() without an appropriate check or assertion on Bpm::isValid() first.

With regard to the FIXMEs, it may be the case that some parts of the code I highlighted in this PR are actually safe - but the point of this PR is simply to highlight where any such safety doesn't seem immediately "obvious".

Happy to review any of the FIXMEs (i.e. remove or fix them if trivial!) before this PR is approved.

mr-smidge
mr-smidge commented Jul 31, 2021
View reviewed changes
src/library/export/engineprimeexportjob.cpp
Comment on lines +113 to +116
bool tryGetBeatgrid(BeatsPointer pBeats,
mixxx::audio::FramePos cuePlayPos,
int64_t frameCount,
std::vector<djinterop::beatgrid_marker>* pBeatgrid) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code in this method is essentially unchanged from before, but has been lifted out of the main exportMetadata() function, and early-exit conditions added if any positions turn out to be invalid.

mr-smidge
mr-smidge commented Jul 31, 2021
View reviewed changes
src/library/export/engineprimeexportjob.cpp
Comment on lines +262 to +263
qWarning() << "Hot cue" << hotCueIndex << "exists but is invalid for track"
<< pTrack->getId() << "(" << pTrack->getFileInfo().fileName() << ")";
Copy link
Contributor Author

@mr-smidge mr-smidge Jul 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Making the assumption that it is a warning-worthy situation to have a cue point for a track, but for that cue point to have an invalid position.

@mr-smidge mr-smidge marked this pull request as ready for review July 31, 2021 21:50
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1085927887

  • 0 of 31 (0.0%) changed or added relevant lines in 1 file are covered.
  • 6 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.006%) to 26.025%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/library/export/engineprimeexportjob.cpp 0 31 0.0%
Files with Coverage Reduction New Missed Lines %
src/engine/controls/bpmcontrol.cpp 1 63.25%
src/library/basetracktablemodel.cpp 1 22.07%
src/library/dlgtrackinfo.cpp 1 0%
src/track/beatutils.cpp 1 48.44%
src/library/export/engineprimeexportjob.cpp 2 0%
Totals Coverage Status
Change from base Build 1083902170: -0.006%
Covered Lines: 20013
Relevant Lines: 76899
💛 - Coveralls

uklotzde
uklotzde reviewed Jul 31, 2021
View reviewed changes
src/library/basetracktablemodel.cpp
@@ -782,6 +782,7 @@ QVariant BaseTrackTableModel::roleValue(
case ColumnCache::COLUMN_LIBRARYTABLE_BPM: {
bool ok;
const auto bpmValue = rawValue.toDouble(&ok);
// FIXME: calling bpm.value() without checking bpm.isValid()
return ok ? bpmValue : mixxx::Bpm().value();
Copy link
Contributor

Choose a reason for hiding this comment

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

mixxx::Bpm().value() should be replaced with mixxx::Bpm::kValueUndefined wherever it occurs

@uklotzde
Copy link
Contributor

@Holzhaus Maybe add double mixxx::Bpm::valueOrUndefined() that doesn't assert?

uklotzde
uklotzde approved these changes Aug 1, 2021
View reviewed changes
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.

Thank you! We can replace the FIXMEs in subsequent PRs.

@uklotzde
Copy link
Contributor

uklotzde commented Aug 1, 2021 •
edited
Loading

LGTM I didn't notice the conflicts, please fix.

@Be-ing Be-ing merged commit cfaedb6 into mixxxdj:main Aug 1, 2021
@Be-ing
Copy link
Contributor

Be-ing commented Aug 1, 2021

Huh??? I merged #4058 and somehow GitHub merged this too?????

@uklotzde
Copy link
Contributor

uklotzde commented Aug 1, 2021

Strange. I pressed merge once here before I noticed the conflicts and GitHub refused to merge.

@uklotzde uklotzde mentioned this pull request Aug 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uklotzde uklotzde uklotzde approved these changes

Assignees
No one assigned
Labels
library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mr-smidge @coveralls @uklotzde @Be-ing