Add support of decoding legacy frames

[yestyle]

Hi @PSeitz ,

While working on an utility ikconfig extracting Linux kernel images, I chose lz4_flex as a dependency to do lz4 decompression. However, Linux kernel is using legacy frame and lz4_flex doesn't support it yet. I added the support in my fork and thinking it'd be good to have it upstream so here's the pull request.

Cheers, Philip

[codecov-commenter]

Codecov Report

Merging #66 (7d2185a) into main (e112ef3) will increase coverage by 0.02%. The diff coverage is 88.57%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
+ Coverage   87.65%   87.67%   +0.02%     
==========================================
  Files          11       11              
  Lines        2292     2321      +29     
==========================================
+ Hits         2009     2035      +26     
- Misses        283      286       +3     

Impacted Files	Coverage Δ
src/frame/decompress.rs	75.17% <80.00%> (+0.36%)	:arrow_up:
src/frame/header.rs	88.31% <100.00%> (+0.75%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[PSeitz]

Awesome thank you! Can you add a decompression test for the legacy format? to make sure there's no regression later

[PSeitz]

I fixed the broken gh action

[yestyle]

Awesome thank you! Can you add a decompression test for the legacy format? to make sure there's no regression later

Sure, let me try to add one.

[PSeitz]

I noticed you force pushed against the branch, but there's no test added

[yestyle]

Hi, thanks for following up. I was force pushing the existing commit rebased onto your github action fix and no new commits added.

I had a quick look at the test cases and they highly depend on both compression and decompression, so I was thinking to add support of compression of legacy frame as well and then add test cases for them. What do you think?

On Sat, 28 Jan 2023 at 7:48 PM, PSeitz @.***> wrote:

I noticed you force pushed against the branch, but there's no test added

— Reply to this email directly, view it on GitHub https://github.com/PSeitz/lz4_flex/pull/66#issuecomment-1407311935, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM6ZKFREFENRSK2MNWE4UTWUS6LDANCNFSM6AAAAAAUINL7JI . You are receiving this because you authored the thread.Message ID: @.***>

[PSeitz]

Since the legacy format is used in the linux kernel, that may be a big enough use case. I would put it behind a feature flag, if existing code is affected performance wise.

You can also add a special test only for legacy decompression out site the test suite.

[yestyle]

Thanks for the suggestion. As described in the commit message, I created benches/dickens.lz4 using CLI utility lz4:

lz4 -l benches/dickens.txt benches/dickens.lz4

and decompress and compare with benches/dickens.txt in the test case. Hope this is acceptable.

[PSeitz]

A 6MB binary is a little big for git, can you pick something smaller?

[yestyle]

Thanks for the review. Since legacy format uses fixed 8MB block size, I think 10MB dickens.txt is a good source to test multiple blocks and more similar to the size of a Linux kernel image. Does it make sense?

I'm open to choose a smaller one (like compression_66k_JSON.txt) if you insist. Cheers.

[PSeitz]

Yes, good point. Thanks for the PR!

[yestyle]

Sweet! By the way, any clue when will be the next release publish to crates.io?

[PSeitz]

Published a new version on crates.io https://crates.io/crates/lz4_flex

[yestyle]

Thank you very much!