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

feat(draw/sw): added support for LV_COLOR_FORMAT_L8 #5800

Merged
merged 57 commits into from
Apr 20, 2024

Conversation

zjanosy
Copy link
Contributor

@zjanosy zjanosy commented Mar 5, 2024

Description of the feature or fix

Implemented rendering L8, RGB565, RGB888 and ARGB8888 into L8 canvas.
Implemented rendering L8 format image into L8, RGB565, RGB888 and ARGB8888 canvas.

Notes

Documentation, examples and tests are WIP.

@FASTSHIFT FASTSHIFT changed the title Added support for LV_COLOR_FORMAT_L8 feat(draw/sw): added support for LV_COLOR_FORMAT_L8 Mar 7, 2024
@liamHowatt
Copy link
Collaborator

All changes make sense to me at first glance.

L8 cogwheel image looks okay on COLOR_DEPTH 8, 16, 24, 32.
L8,RGB565,RGB888,ARGB8888 cogwheel image looks okay on COLOR_DEPTH 8.
Blending untested.

It's a large space of possibilities to test. The eventual test cases will reveal any non-obvious problems.

@razor5k
Copy link
Contributor

razor5k commented Mar 11, 2024

Hi,
does it make sense to add integer rounding before divide operations?

//example
static inline uint8_t lv_color_luminance(lv_color_t c)
{
return (uint8_t)((uint16_t)(77u * c.red + 151u * c.green + 28u * c.blue) >> 8);
}
//to
static inline uint8_t lv_color_luminance(lv_color_t c)
{
return (uint8_t)((uint16_t)(77u * c.red + 151u * c.green + 28u * c.blue + 128) >> 8);
}

@zjanosy
Copy link
Contributor Author

zjanosy commented Mar 11, 2024

Hi, does it make sense to add integer rounding before divide operations?

I don't think it would be visible, and it would be a little bit slower. In fact, this formula is already an approximation, so I would not bother. Maybe an even simpler (and faster) approximation would do, like ((c.red << 1) + (c.green << 2) + c.blue) >> 3, although this may need some scaling to reach the maximum value of 255.
Here is a comparison of using the "real" formula and the crude approximation:
lenna_0 3R+0 59G+0 11B
lenna_0 25R+0 5G+0 13B
There is little visible difference, except that the latter is a bit darker (which could be fixed by an additional scaling, but then we loose the performance gain -- if there is any on modern architectures).

@razor5k
Copy link
Contributor

razor5k commented Mar 11, 2024

You made a valid point not to round. If you don't see a huge difference, it may be better to be faster.

@kisvegabor
Copy link
Member

Hey,

Thank you or the PR! It was already requested by a lot of people! 🙂

I've tested it like this:

  lv_demo_widgets();

  lv_draw_buf_t * draw_buf = lv_snapshot_create_draw_buf(lv_screen_active(), LV_COLOR_FORMAT_L8);
  lv_snapshot_take_to_draw_buf(lv_screen_active(), LV_COLOR_FORMAT_L8, draw_buf);

  lv_obj_clean(lv_screen_active());
  lv_obj_t * canvas = lv_canvas_create(lv_screen_active());
  lv_canvas_set_draw_buf(canvas, draw_buf);

image
You can notice that

  • the center of the slider is empty
  • the shadow below the the buttons are missing
lv_demo_render(LV_DEMO_RENDER_SCENE_FILL, LV_OPA_50);

image

ARGB as reference
image


I've updated the tests to support rendering to L8 and added the reference images as well for this test case. It seems there are a few places where the rendering is not correct. Could you take a look at these?

@kisvegabor
Copy link
Member

The CI passing for me locally, but here is an off by one error here. Do you have any idea why the rendering can be platform dependent?

@kisvegabor
Copy link
Member

FYI, the issue is here:
image

So when a scaled ARGB8888 image is blended to L8.

@kisvegabor
Copy link
Member

And it's passing now. We will see if the passing or the failure have happened by "accident".

I've discussed it with @zjanosy that he will update lv_demo_render with blending A8 and RGB565A8 images too (so use these as source and not destination)

Zoltan Janosy and others added 2 commits March 12, 2024 17:52
…amma correction before luminance calculation. This method results in more details in the dark areas of the picture.
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

I've just fixed the code formatting issue.

#if defined(LV_LVGL_H_INCLUDE_SIMPLE)
#include "lvgl.h"
#elif defined(LV_BUILD_TEST)
#include "../lvgl.h"
Copy link
Member

Choose a reason for hiding this comment

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

Use an include similar to the other images.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

FASTSHIFT and others added 5 commits April 17, 2024 11:09
Signed-off-by: pengyiqiang <pengyiqiang@xiaomi.com>
Co-authored-by: pengyiqiang <pengyiqiang@xiaomi.com>
…vgl#5979)

Signed-off-by: qinshijing <qinshijing@xiaomi.com>
Co-authored-by: qinshijing <qinshijing@xiaomi.com>
kisvegabor
kisvegabor previously approved these changes Apr 17, 2024
Copy link
Member

@kisvegabor kisvegabor left a comment

Choose a reason for hiding this comment

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

I've removed the accidentally changed test files.

The rest is looking good!

@@ -309,7 +309,7 @@ void lv_obj_scroll_by(lv_obj_t * obj, int32_t dx, int32_t dy, lv_anim_enable_t a
lv_anim_t a;
lv_anim_init(&a);
lv_anim_set_var(&a, obj);
lv_anim_set_completed_cb(&a, scroll_completed_completed_cb);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was it modified? it comes from this fix: #5979

Copy link
Member

Choose a reason for hiding this comment

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

I reverted it.

XuNeo
XuNeo previously approved these changes Apr 18, 2024
scripts/LVGLImage.py Outdated Show resolved Hide resolved
scripts/LVGLImage.py Outdated Show resolved Hide resolved
scripts/LVGLImage.py Outdated Show resolved Hide resolved
scripts/LVGLImage.py Outdated Show resolved Hide resolved
scripts/LVGLImage.py Outdated Show resolved Hide resolved
typedef struct {
uint8_t lumi;
uint8_t alpha;
} lv_color16a_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should it be lv_color8a_t?

Copy link
Member

@kisvegabor kisvegabor Apr 18, 2024

Choose a reason for hiding this comment

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

lv_color16a_t is really misleading as lv_color16_t is RGB565. lv_color_al88_t would be cleanest, but it's different style. lv_color8a_t can be a good trade off.

Copy link
Contributor Author

@zjanosy zjanosy Apr 18, 2024

Choose a reason for hiding this comment

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

While I agree that lv_color16a_t is not the best name, the naming scheme was that the number in the color format name indicates the number of bits of the value. Therefore I think color8a is a bit misleading, too, because it suggest that it is a 8 bit value, which it is not. Maybe lv_color8a8_t would be better (although ARGB8888 is called lv_color32_t, and not something like lv_color24a or lv_color24a8).
Certainly, the cleanest solution would be to call them as the corresponding color format is called, i.e.:
lv_color32_t -> lv_argb8888_t
lv_color16_t -> lv_rgb565_t
lv_color16a_t -> lv_al88_t
But this would likely break too much code.

Copy link
Member

Choose a reason for hiding this comment

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

Actually we can rename these by adding typedef lv_color_argb8888_t lv_color32_t in lv_api_map_v9.h.

Copy link
Collaborator

Choose a reason for hiding this comment

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

lv_color32_t -> lv_argb8888_t
lv_color16_t -> lv_rgb565_t
lv_color16a_t -> lv_al88_t

I agree with it. But it should be lv_color16_t --> lv_al88_t where 16 means it takes 16bits. There will be lv_color[32|24|16|8]_t.

Actually we can rename these by adding typedef lv_color_argb8888_t lv_color32_t in lv_api_map_v9.h.

Let's go for it. It can be a separate PR. We can merge this massive achievement firstly.

def linear_to_sRGB(self, y):
if y <= 0.0031308:
return 12.92 * y
return 1.055 * pow(y, 1/2.4) - 0.055

def _png_to_luma_only(self, cf: ColorFormat, filename: str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the luma here should be called lumi(nance) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, luma and luminance are different terms:
https://www.geeksforgeeks.org/difference-between-luma-and-luminance/
But honestly I don't know which is the correct name in our case.

Co-authored-by: Neo Xu <neo.xu1990@gmail.com>
kisvegabor and others added 4 commits April 18, 2024 21:16
Co-authored-by: Neo Xu <neo.xu1990@gmail.com>
Co-authored-by: Neo Xu <neo.xu1990@gmail.com>
Co-authored-by: Neo Xu <neo.xu1990@gmail.com>
Co-authored-by: Neo Xu <neo.xu1990@gmail.com>
@kisvegabor kisvegabor merged commit abc8a72 into lvgl:master Apr 20, 2024
18 checks passed
@kisvegabor
Copy link
Member

Merged, thank you!

@GorgonMeducer
Copy link
Contributor

I plan to add the Arm-2D (Helium) support for L8. In fact, in arm-2d, we call L8 the GRAY8.

@kisvegabor
Copy link
Member

Amazing, thank you in advance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants