chore: update rekordbox kaitai definitions - #13293
Conversation
|
Apparently not, seems like we need an updated kaitaistruct runtime. It also doesn't seem appropriate to vendor that anymore. Would |
|
We have it in VCPKG |
|
yeah, but thats about it, its not packaged on any linux distro. https://repology.org/project/kaitai-struct-cpp-stl-runtime/versions |
|
We cannot download external sources on Launchpad. So let's just update our vendored version. |
ab24091 to
4ab1375
Compare
4ab1375 to
277b5c1
Compare
| // workaround for https://github.com/kaitai-io/kaitai_struct_cpp_stl_runtime/issues/67 | ||
| static std::string bytes_to_str(const std::string src, const std::string& enc) { | ||
| return bytes_to_str(src, enc.c_str()); | ||
| } |
There was a problem hiding this comment.
only changed I made to the kaitai files except copy-pasting in the newest commit and moving them to match the includes the kaitai-compiler generates.
| # NOTE: unmasking is disabled because the C++ backend of kaitai_struct | ||
| # doesn't seem to support typecasting as of kaitai version 0.10? | ||
| # process: 'xor(is_masked ? mask : [0])' | ||
| instances: | ||
| c: | ||
| value: len_entries | ||
| # mask: | ||
| # value: | | ||
| # [ | ||
| # (0xCB+c).as<u1>, (0xE1+c).as<u1>, (0xEE+c).as<u1>, (0xFA+c).as<s1>, (0xE5+c).as<s1>, (0xEE+c).as<s1>, (0xAD+c).as<s1>, (0xEE+c).as<s1>, | ||
| # (0xE9+c).as<u1>, (0xD2+c).as<u1>, (0xE9+c).as<u1>, (0xEB+c).as<s1>, (0xE1+c).as<s1>, (0xE9+c).as<s1>, (0xF3+c).as<s1>, (0xE8+c).as<s1>, | ||
| # (0xE9+c).as<u1>, (0xF4+c).as<u1>, (0xE1+c).as<u1> | ||
| # ].as<bytes> | ||
| raw_mood: |
| for (std::vector<rekordbox_pdb_t::table_t*>::iterator table = | ||
| reckordboxDB.tables()->begin(); | ||
| table != reckordboxDB.tables()->end(); | ||
| ++table) { | ||
| if ((*table)->type() == tableOrder[tableOrderIndex]) { | ||
| uint16_t lastIndex = (*table)->last_page()->index(); | ||
| rekordbox_pdb_t::page_ref_t* currentRef = (*table)->first_page(); | ||
| for (const auto& table : *rekordboxDB.tables()) { | ||
| if (table->type() == tableOrder[tableOrderIndex]) { | ||
| uint16_t lastIndex = table->last_page()->index(); | ||
| rekordbox_pdb_t::page_ref_t* currentRef = table->first_page(); |
There was a problem hiding this comment.
this didn't build anymore because the table_t* changed to a unique_ptr<table_t>. I took the liberty and modernized the loop in the same step (same below).
277b5c1 to
7df39ab
Compare
|
@0xsuk can you test this PR to see whether this doesn't break anything and fixes your string-encoding issue? Thank you. |
9a05287 to
c9ca71f
Compare
|
Everything looks good after I changed UTF-16BE to UTF-16LE in src/library/rekordbox/rekordboxfeature.cpp line 259. |
|
|
This makes it easier to ignore warnings in the autogenerated code. It also makes more sense since the `*.ksy` files are effectively vendored and the generated C++ files are autogenerated. Both something that doesn't belong in `src/`.
c9ca71f to
157feda
Compare
|
whoops. good catch, I assumed kaitai would take care of the conversion automatically. Fixed |
|
Thanks for testing. |
|
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. |
|
friendly ping, would be nice if this made it into 2.4.2. I've highlighted the significant changes above. |
| add_library(rekordbox_metadata STATIC EXCLUDE_FROM_ALL | ||
| lib/rekordbox-metadata/rekordbox_pdb.cpp | ||
| lib/rekordbox-metadata/rekordbox_anlz.cpp | ||
| ) | ||
| target_include_directories(rekordbox_metadata SYSTEM PUBLIC lib/rekordbox-metadata) | ||
| target_link_libraries(mixxx-lib PRIVATE rekordbox_metadata) | ||
|
|
||
| # Kaitai for reading Rekordbox libraries | ||
| add_library(Kaitai STATIC EXCLUDE_FROM_ALL | ||
| lib/kaitai/kaitaistream.cpp | ||
| lib/kaitai/kaitai/kaitaistream.cpp | ||
| ) |
There was a problem hiding this comment.
I got a lot of code quality complaints on the output of the kaitai compiler, so I moved the generated code to lib/ (first and foremost because the linters already ignore that and second because it also makes sense to treat autogenerated code that way).
|
How important is the fix? Does this rectify a release? |
|
Thank you.
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. |
|
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. |
|
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 |
|
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. 😄 |
|
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. |
|
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. |
|
Let’s start on the Zulip instance and then I can create issues as appropriate. |
Fixes mixxxdj#14789 Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>
Fixes mixxxdj#14789 Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>
…descriptions-utf16be fix: hotcue comments are utf-16be, regression from #13293
5 participants
Should also fix #13291.
Lets see if this builds without updating any additional changes.