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

lv_img_conv_py: minimal python port of node module #1863

Merged
merged 2 commits into from
Oct 26, 2023
Merged

Conversation

NeroBurner
Copy link
Contributor

@NeroBurner NeroBurner commented Sep 21, 2023

Create a minimal python port of the node.js module lv_img_conv. Only
the currently in use color formats CF_INDEXED_1_BIT and
CF_TRUE_COLOR_ALPHA are implemented.

Output only as binary with format ARGB8565_RBSWAP (and ARGB8888 for debugging).

This is enough to create the resources-1.13.0.zip.

In the simulator the pinecone in Casio is visible, and the navigation symbol is visible as well in the simulator when the resource zip is loaded

The new lv_img_conv.py script needs python3-pil python module as runtime dependency.
The runtime dependency is added to the docker container image.

InfiniSim_2023-09-21_230253_casio
InfiniSim_2023-09-21_230304_nav

@NeroBurner NeroBurner changed the title lv_img_conv_py: minimal python port of node module draft: lv_img_conv_py: minimal python port of node module Sep 21, 2023
@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Build size and comparison to main:

Section Size Difference
text 376784B -16B
data 940B 0B
bss 63420B 0B

@NeroBurner NeroBurner marked this pull request as draft September 25, 2023 14:43
@NeroBurner NeroBurner changed the title draft: lv_img_conv_py: minimal python port of node module lv_img_conv_py: minimal python port of node module Sep 25, 2023
@NeroBurner
Copy link
Contributor Author

I've fixed part of the differences between the original nodejs script and the python based port. It was the usage of round() in the helper function classify_pixel().
Python3 implements "propper" "banker's rounding" by rounding to the nearest
even number. Javascript rounds to the nearest integer as we've learned in school.
To have the same output as the original JavaScript implementation add a custom
rounding function, which does "school" rounding (to the nearest integer).


After that there is still a difference between the binaries. Therefore I implemented the ARGB8888 binary-output-format to compare the rgba values read by the scripts from the png.

./lv_img_conv.js pine_logo.png --force --output-file pine_small_js_8888.bin --color-format CF_TRUE_COLOR_ALPHA --output-format bin --binary-format ARGB8888
xxd pine_small_js_8888.bin > pine_small_js_8888.bin.hex

Same with the current python port

python3 lv_img_conv.py  --force --output-file pine_small_js_8888.bin --color-format CF_TRUE_COLOR_ALPHA --output-format bin --binary-format ARGB8888
xxd pine_small_py_8888.bin > pine_small_py_8888.bin.hex

Uploaded the hex files for comparison:

Comparing the two hex files shows there are differences in how the PNG file is read in (because ARGB8888 just outputs the read rgba values)
Screenshot_20230928_222816

The first different pixel is at x==10, y==0. Looking at that pixel nodejs returns rgba=[19, 19, 19, 26] compared to python rgba=[20, 20, 20, 26]

Opened the pine-logo.png in Gimp and it shows the same values as the python script

gimp rgba color values

This off-by-one input differences results in some differences in the normal ARGB8565_RBSWAP output binary format as well.
As the difference is small and I believe the value from Gimp I'd keep the python script as it is (as I don't know how to "fix" it to be like the node input)


The only difference for the navigation0.png and navigation1.png files is the hard coded color palette. The remaining bits are the same for nodejs and python implementation

@NeroBurner NeroBurner force-pushed the lv_img_conv_py branch 2 times, most recently from fa92ff9 to 4edfbae Compare October 18, 2023 20:11
@NeroBurner NeroBurner marked this pull request as ready for review October 18, 2023 20:15
@NeroBurner NeroBurner requested a review from a team October 18, 2023 20:15
@NeroBurner NeroBurner added the maintenance Background work label Oct 18, 2023
Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

Looks great overall, performance is a lot better as well. Haven't tested on hardware but according to your earlier tests I trust the output
Mostly code cleanliness suggestions/doc suggestions, only architectural one is whether you want to import the file directly or run it as a subprocess

src/resources/generate-img.py Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
doc/buildAndProgram.md Outdated Show resolved Hide resolved
@@ -31,6 +31,10 @@ jobs:
uses: actions/checkout@v3
with:
submodules: recursive
- name: Install python3-pil
Copy link
Member

Choose a reason for hiding this comment

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

Maybe "Install resource build dependencies" would be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

once the docker file is updated this install should be removed as it is already done in the used docker image 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed as recommended, probably should remove in future 🤔
but keeping it for now to allow a smooth CI until docker image is updated

src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@NeroBurner NeroBurner left a comment

Choose a reason for hiding this comment

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

partial fixes, gonna do the rest at a later time

@@ -31,6 +31,10 @@ jobs:
uses: actions/checkout@v3
with:
submodules: recursive
- name: Install python3-pil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

once the docker file is updated this install should be removed as it is already done in the used docker image 🤔

doc/buildAndProgram.md Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
src/resources/lv_img_conv.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@JF002 JF002 left a comment

Choose a reason for hiding this comment

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

I'm far from a Python expert, so I trust you on that regard 👍

This scripts makes the setup of the build environment much easier and it works fine on my setup and in the build Docker container so... LGTM :-)

Create a minimal python port of the node.js module `lv_img_conv`. Only
the currently in use color formats `CF_INDEXED_1_BIT` and
`CF_TRUE_COLOR_ALPHA` are implemented.

Output only as binary with format `ARGB8565_RBSWAP`.

This is enough to create the `resources-1.13.0.zip`.

Python3 implements "propper" "banker's rounding" by rounding to the nearest
even number. Javascript rounds to the nearest integer.
To have the same output as the original JavaScript implementation add a custom
rounding function, which does "school" rounding (to the nearest integer)

Update CMake file in `resources` folder to call `lv_img_conf.py` instead of
node module.

For docker-files install `python3-pil` package for `lv_img_conv.py` script.
And remove the `lv_img_conv` node installation.

---

gen_img: special handling for python lv_img_conv script

Not needed on Linux systems, as the shebang of the python script is read
and used. But just to be sure use the python interpreter found by CMake.
Also helps if tried to run on Windows host.

---

doc: buildAndProgram: remove node script lv_img_conv mention

Remove node script `lv_img_conv` mention and replace it for
runtime-depency `python3-pil` of python script `lv_img_conv.py`.
Used by script `lv_img_conv.py`, should be provided by docker image, but
until then explicitly install in workflow.
@NeroBurner
Copy link
Contributor Author

code review incorporated, squashed and pushed, gonna merge as soon as possible

@NeroBurner
Copy link
Contributor Author

User MorsMortium informed me on Matrix, that there is a full port available at https://github.com/W-Mai/lvgl_image_converter

Still using this minimum port, and if we need more we could either switch to the full port or use the full port as inspiration to implement the newly needed parts

Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@NeroBurner NeroBurner merged commit e6b96c2 into main Oct 26, 2023
6 checks passed
@NeroBurner NeroBurner deleted the lv_img_conv_py branch October 26, 2023 20:45
NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request Oct 26, 2023
Install new build dependency introduced with move to minimum python
implementation of `lv_img_conv` node js script
InfiniTimeOrg/InfiniTime#1863
NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request Oct 26, 2023
Install new build dependency introduced with move to minimum python
implementation of `lv_img_conv` node js script
InfiniTimeOrg/InfiniTime#1863
NeroBurner added a commit to InfiniTimeOrg/InfiniSim that referenced this pull request Oct 26, 2023
Install new build dependency introduced with move to minimum python
implementation of `lv_img_conv` node js script
InfiniTimeOrg/InfiniTime#1863
@JF002 JF002 added this to the 1.14.0 milestone Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Background work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants