search

mixxxdj / mixxx

Mixxx is Free DJ software that gives you everything you need to perform live mixes.
http://mixxx.org
Other
4.25k stars 1.24k forks source link

chore: update rekordbox kaitai definitions #13293

Closed Swiftb0y closed 3 weeks ago

Swiftb0y commented 1 month ago

Should also fix #13291.

Lets see if this builds without updating any additional changes.

Swiftb0y commented 1 month ago

Apparently not, seems like we need an updated kaitaistruct runtime. It also doesn't seem appropriate to vendor that anymore. Would ExternalProject_Add instead be an option? Can somebody help me with the required CMake legwork for that?

JoergAtGithub commented 1 month ago

We have it in VCPKG

Swiftb0y commented 1 month ago

yeah, but thats about it, its not packaged on any linux distro. https://repology.org/project/kaitai-struct-cpp-stl-runtime/versions

daschuer commented 1 month ago

We cannot download external sources on Launchpad. So let's just update our vendored version.

Swiftb0y commented 1 month ago

@0xsuk can you test this PR to see whether this doesn't break anything and fixes your string-encoding issue? Thank you.

0xsuk commented 1 month ago

Everything looks good after I changed UTF-16BE to UTF-16LE in src/library/rekordbox/rekordboxfeature.cpp line 259.

0xsuk commented 1 month ago

image Thank you :)

Swiftb0y commented 1 month ago

whoops. good catch, I assumed kaitai would take care of the conversion automatically. Fixed

Swiftb0y commented 1 month ago

Thanks for testing.

Swiftb0y commented 1 month ago

Ready for review. I suggest to go through on a commit by commit basis. The PR looks bigger as it is because most of the changes are either auto-generated or artifacts because I moved files around.

Swiftb0y commented 3 weeks ago

friendly ping, would be nice if this made it into 2.4.2. I've highlighted the significant changes above.

daschuer commented 3 weeks ago

How important is the fix? Does this rectify a release?

Swiftb0y commented 3 weeks ago

Thank you.

How important is the fix? Does this rectify a release?

Well this PR contains two fixes. First the string decoding issue reported in #13291 and the second one concerns the history playlists created by rekordbox. I don't even think we ingest those.

I don't think an extra release is warranted as only the string decoding issue is affected, judging by the delay with which this was reported, I wouldn't say its critical enough to justify the effort.

brunchboy commented 3 weeks ago

It’s not just history playlists. Any table which has had deletions over time will likely be missing a few rows at the end without the structure fix.

brunchboy commented 3 weeks ago

And not just deletions, rows are sometimes not present in row groups for inscrutable reasons. The fact that the problem was noticed in the history playlist table shows it could happen anywhere. We need to scan the entire last row group for rows marked as present, not assume we can stop scanning based on the value of num_rows.

brunchboy commented 3 weeks ago

Which is not to say that it necessarily merits an urgent release, I have no idea how many people rely on scanning rekordbox USBs in the first place. 😄

Swiftb0y commented 3 weeks ago

Ah, thank you. In that case it would probably justify a release. Considering there is also #13309 #11373 and #10923 which all could be caused by that issue.

brunchboy commented 3 weeks ago

Hmm, those all could be impacted but they sound worse than this! Please feel free to ping me if this doesn’t fix those other issues and perhaps I can help investigate the problematic export files.

Swiftb0y commented 3 weeks ago

That would be wonderful. We can close these issues once we have a release with the fix and ask the respective issue authors to check again. In case it still happens, I would refer them to you (where exactly? crate-digger repo / deep-symmetry zulip) if thats alright for you.

brunchboy commented 3 weeks ago

Let’s start on the Zulip instance and then I can create issues as appropriate.