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

replay: add support for dcecompressing ZST log files #32910

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented Jul 5, 2024

No description provided.

Copy link
Contributor

github-actions bot commented Jul 5, 2024

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

@github-actions github-actions bot added the tools label Jul 5, 2024
@deanlee deanlee changed the title replay: add support for dcecompressing ZST log files4 replay: add support for dcecompressing ZST log files Jul 5, 2024
@deanlee deanlee force-pushed the replay_support_zst branch from 9d5476f to f9d4adf Compare July 5, 2024 05:20
@deanlee deanlee marked this pull request as draft July 5, 2024 20:38
@deanlee deanlee force-pushed the replay_support_zst branch 4 times, most recently from 4602da8 to ed969c9 Compare July 12, 2024 19:49
@sshane
Copy link
Contributor

sshane commented Jul 26, 2024

@deanlee we're getting this merged soon, is this good to go?

@deanlee
Copy link
Contributor Author

deanlee commented Jul 26, 2024

Yes, I've tested it and it's ready to go. However, there is still an issue with the selfdrive/PR comments. It can't find #include <zstd.h>. It seems we need to update the Docker image to include the zstd library. Do you have any ideas on how to resolve this?

@sshane
Copy link
Contributor

sshane commented Jul 27, 2024

Looks like it's building the docker image from the base branch, which is expected to fail here. Hopefully we can clean up this job very soon, but I wouldn't make it a blocker for this PR. It should pass once we merge.

tools/replay/util.cc Outdated Show resolved Hide resolved
Comment on lines 11 to 15
if (url.find(".bz2") != std::string::npos) {
data = decompressBZ2(data, abort);
} else if (url.find(".zst") != std::string::npos) {
data = decompressZST((std::byte *)data.data(), data.size(), abort);
Copy link
Contributor

@sshane sshane Jul 27, 2024

Choose a reason for hiding this comment

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

Heads up we may remove the file extensions soon, so it should also detect from the file header like the python LogReader does.

@deanlee deanlee force-pushed the replay_support_zst branch from ed969c9 to 79c8a82 Compare July 27, 2024 05:30
@deanlee deanlee marked this pull request as ready for review July 27, 2024 05:38
@deanlee
Copy link
Contributor Author

deanlee commented Jul 27, 2024

The PR is ready, suggested changes have been applied.

@sshane sshane merged commit ade1372 into commaai:master Jul 29, 2024
13 of 15 checks passed
@sshane
Copy link
Contributor

sshane commented Jul 29, 2024

ty!

@deanlee deanlee deleted the replay_support_zst branch July 29, 2024 20:49
Edison-CBS pushed a commit to Edison-CBS/openpilot that referenced this pull request Sep 15, 2024
* Add Support for Decompressing ZST Log Files

* 2 space and check magic number

* match BZ2

---------

Co-authored-by: Shane Smiskol <shane@smiskol.com>
old-commit-hash: ade1372
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants