Hi Alexey,
On 2026-09-18T10:52:27, Alexey Charkov <[email protected]> wrote:
> binman: Allow choosing the compression level
>
> Every compression algorithm binman drives has a level, and the best one
> to use depends on what the image is for: a payload sent over USB wants
> the smallest it can get, one read from fast storage would rather not
> wait for it. Today only the tool's own default is reachable.
>
> Add a 'compress-level' property, passed through as -<level>, which is how
> all compressors want it except lzma: the LZMA SDK tool has no such option,
> and is controlled by the -lc/-lp/-pb values binman already passes.
>
> Signed-off-by: Alexey Charkov <[email protected]>
>
> tools/binman/binman.rst | 21 +++++++++++++
> tools/binman/bintool.py | 10 +++++--
> tools/binman/btool/lz4.py | 6 ++--
> tools/binman/btool/lzma_alone.py | 6 +++-
> tools/binman/entry.py | 6 +++-
> tools/binman/ftest.py | 34
> ++++++++++++++++++++++
> tools/binman/test/entry/compress_level.dts | 12 ++++++++
> tools/binman/test/entry/compress_level_gzip.dts | 12 ++++++++
> tools/binman/test/entry/compress_level_lzma.dts | 12 ++++++++
> .../test/entry/compress_level_no_compress.dts | 11 +++++++
> 10 files changed, 124 insertions(+), 6 deletions(-)
> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> @@ -711,6 +711,11 @@ compress:
> +compress-level:
> + Sets how hard the compression algorithm should try, as the level number
> + the tool itself uses. Requires 'compress'. See the entry documentation
> + for details.
The entry documentation is generated from the docstring in
etype/blob.py and that still only lists 'filename' and 'compress'.
Please can you add 'compress-level' there too?
> diff --git a/tools/binman/binman.rst b/tools/binman/binman.rst
> @@ -1182,6 +1187,22 @@ The entry will then contain the compressed data, using
> the 'lz4' compression
> +The number is the tool's own level, so its meaning and range follow that
> tool:
> +1 to 9 for gzip, bzip2, lzop and xz, 1 to 12 for lz4, 1 to 19 for zstd.
> Asking
> +for more than an algorithm offers usually gets a warning and its maximum
> +instead. Leaving the property out uses the tool's default, which is what
> binman
> +has always done. It cannot be used with 'lzma', whose LZMA SDK tool has no
> such
> +option.
Not quite. I tried this and only zstd warns and clamps. lz4 clamps
silently. gzip, bzip2, xz and lzop read -12 as -1 followed by -2, so
you silently get level 2, i.e. asking for more gives you much less.
See below for a suggestion.
Also, with lz4 binman passes -5, which is not the tool's default (that is -1).
> diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
> @@ -570,7 +573,10 @@ class BintoolPacker(Bintool):
> + args = self.compress_args[:]
> + if level is not None:
> + args.append(f'-{level}')
> + args += ['--stdout', tmp.name]
Given the above, I don't think binman can pass the number through
unchecked. A typo in the level, or changing 'compress' from zstd to
gzip while leaving the level at 19, produces a valid image that is
larger than the default, with nothing to say why.
How about having each compression bintool declare the range it
supports (e.g. a max_level passed to the BintoolPacker constructor,
with None meaning that levels are not supported), then have Entry
check the value against it and call self.Raise() if it is out of
range? What do you think?
> diff --git a/tools/binman/btool/lz4.py b/tools/binman/btool/lz4.py
> @@ -90,7 +92,7 @@ class Bintoollz4(bintool.Bintool):
> - args = ['--no-frame-crc', '-B4', '-5', '-c', tmp.name]
> + args = ['--no-frame-crc', '-B4', f'-{level or 5}', '-c',
> tmp.name]
This treats a level of 0 as 'not set', whereas BintoolPacker checks
for None and passes -0 to the tool. gzip, bzip2 and lzop reject that
as an invalid option, while xz and zstd accept it. Please can you make
these consistent? Rejecting 0 in the range check would be fine, I
think.
> diff --git a/tools/binman/btool/lzma_alone.py
> b/tools/binman/btool/lzma_alone.py
> @@ -55,15 +55,19 @@ class Bintoollzma_alone(bintool.Bintool):
> + if level is not None:
> + raise ValueError("lzma_alone does not support 'compress-level'")
This error does not include the node path, so with several compressed
entries it is hard to tell which one is wrong. It also only fires if
the tool is present, since otherwise CompressData() records a missing
bintool and never gets here.
If the range lives in the bintool as suggested above, Entry can check
this with self.Raise() (e.g. in AddBintools() where comp_bintool is
set up), in the same way as the 'requires compress' check, and this
special case goes away.
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> @@ -2168,6 +2168,40 @@ class TestFunctional(unittest.TestCase):
> + def testCompressLevel(self):
> + """Test that a compression level reaches the tool and round-trips"""
> + self._CheckLz4()
> + data = self._DoReadFile('entry/compress_level.dts',
> use_real_dtb=True)
> + self.assertEqual(COMPRESS_DATA, self._decompress(data))
> +
> + entry = control.images['image'].GetEntries()['blob']
> + self.assertEqual(12, entry.compress_level)
This checks that the property is parsed and that the data round-trips,
but not that the level reaches the tool. The test would still pass if
compress() dropped the level, and the same applies to
testCompressLevelPacker().
Please can you check the arguments, e.g. by using
unittest.mock.patch.object() on bintool.Bintool.run_cmd as the cst
test does, recording the args and then calling the original?
Since you are adding range checks, you may need a test for an
out-of-range level too.
Regards,
Simon
