fix(canvas): lv_canvas_set_px for indexed images

[kisvegabor]

Description of the feature or fix

fixes https://github.com/lvgl/lvgl/issues/6196

_LV_DRAW_BUF_STRIDE and _LV_DRAW_BUF_SIZE weren't working well for for indexed image.

• _LV_DRAW_BUF_STRIDE gave 2 (instead of 3) for 10 px width and I1.
• _LV_DRAW_BUF_SIZE wasn't considering the palette size.

Notes

• Update the Documentation if needed.
• Add Examples if relevant.
• Add Tests if applicable.
• If you added new options to lv_conf_template.h run lv_conf_internal_gen.py and update Kconfig.
• Run scripts/code-format.py (astyle version v3.4.12 needs to be installed) and follow the Code Conventions.
• Mark the Pull request as Draft while you are working on the first version, and mark is as Ready when it's ready for review.
• When changes were requested, re-request review to notify the maintainers.
• Help us to review this Pull Request! Anyone can approve or request changes.

[XuNeo]

You can use the data pointer from lv_draw_buf_goto_xy directly, except there's a bug in it.

diff --git a/src/draw/lv_draw_buf.c b/src/draw/lv_draw_buf.c
index 10e65b2b5..adb4b7577 100644
--- a/src/draw/lv_draw_buf.c
+++ b/src/draw/lv_draw_buf.c
@@ -379,7 +379,7 @@ void * lv_draw_buf_goto_xy(const lv_draw_buf_t * buf, uint32_t x, uint32_t y)
 
     if(x == 0) return data;
 
-    return data + x * lv_color_format_get_size(buf->header.cf);
+    return data + x * lv_color_format_get_bpp(buf->header.cf) / 8;
 }
 
 lv_result_t lv_draw_buf_adjust_stride(lv_draw_buf_t * src, uint32_t stride)

In this way, the patch can be simplified to below one.

I can push the update if you like it.

commit 08d915216a8048bac9e51d5b46ecf447858cebb6 (HEAD -> canvas)
Author: Gabor Kiss-Vamosi <[email protected]>
Date:   Tue May 14 12:30:01 2024 +0200

    fix(canvas): lv_canvas_set_px for indexed images
    
    fixes #6196
    
    Process indexed image in single case.
    
    Signed-off-by: Xu Xingliang <[email protected]>

diff --git a/src/widgets/canvas/lv_canvas.c b/src/widgets/canvas/lv_canvas.c
index 877f6bfa9..fc31c4425 100644
--- a/src/widgets/canvas/lv_canvas.c
+++ b/src/widgets/canvas/lv_canvas.c
@@ -107,20 +107,31 @@ void lv_canvas_set_px(lv_obj_t * obj, int32_t x, int32_t y, lv_color_t color, lv
     lv_draw_buf_t * draw_buf = canvas->draw_buf;
 
     lv_color_format_t cf = draw_buf->header.cf;
-    uint32_t stride = draw_buf->header.stride;
     uint8_t * data = lv_draw_buf_goto_xy(draw_buf, x, y);
 
     if(LV_COLOR_FORMAT_IS_INDEXED(cf)) {
-        /*Indexed image bpp could be less than 8, calculate again*/
-        uint8_t * buf = (uint8_t *)canvas->draw_buf->data;
-        buf += 8;
-        buf += y * stride;
-        buf += x >> 3;
-        uint32_t bit = 7 - (x & 0x7);
-        uint32_t c_int = color.blue;
-
-        *buf &= ~(1 << bit);
-        *buf |= c_int << bit;
+        uint8_t shift;
+        switch(cf) {
+            case LV_COLOR_FORMAT_I1:
+                shift = 7 - (x & 0x7);
+                break;
+            case LV_COLOR_FORMAT_I2:
+                shift = 6 - 2 * (x & 0x3);
+                break;
+            case LV_COLOR_FORMAT_I4:
+                shift = 4 - 4 * (x & 0x1);
+                break;
+            case LV_COLOR_FORMAT_I8:
+                /*Indexed8 format is a easy case, process and return.*/
+                *data = color.blue;
+            default:
+                return;
+        }
+
+        uint8_t bpp = lv_color_format_get_bpp(cf);
+        uint8_t mask = (1 << bpp) - 1;
+        uint8_t c_int = color.blue & mask;
+        *data = (*data & ~(mask << shift)) | c_int << shift;
     }
     else if(cf == LV_COLOR_FORMAT_L8) {
         *data = lv_color_luminance(color);

[liamHowatt]

Am I using it correctly? If so, I think there are still some issues. I8 looks good though. Maybe add this as a test case if you want.

static void canvas_set_px_indexed(void)
{
    LV_IMAGE_DECLARE(img_cogwheel_rgb);
    LV_ASSERT(((uintptr_t) img_cogwheel_rgb.data) % 4 == 0);
    LV_ASSERT(img_cogwheel_rgb.header.cf == LV_COLOR_FORMAT_XRGB8888);
    const uint32_t w = img_cogwheel_rgb.header.w;
    const uint32_t h = img_cogwheel_rgb.header.h;
    LV_LOG_USER("dims: %u %u", w, h);

    lv_color_format_t ims[] = {
        LV_COLOR_FORMAT_I1,
        LV_COLOR_FORMAT_I2,
        LV_COLOR_FORMAT_I4,
        LV_COLOR_FORMAT_I8,
        LV_COLOR_FORMAT_L8
    };
    int palette_sizes[] = {2, 4, 16, 256};
    for (int im=0; im<(sizeof(ims)/sizeof(*ims)); im++) {
        lv_obj_t * canv = lv_canvas_create(lv_screen_active());
        lv_canvas_set_draw_buf(canv, lv_draw_buf_create(w, h, ims[im], LV_STRIDE_AUTO));
        if(ims[im] != LV_COLOR_FORMAT_L8) {
            for(int pal_i=0; pal_i<palette_sizes[im]; pal_i++) {
                // lv_color_t col = lv_color_hsv_to_rgb(lv_map(pal_i, 0, palette_sizes[im]-1, 0, 359), 100, 100);
                // lv_color32_t col32 = {col.red, col.green, col.blue, 255};
                uint8_t grey = pal_i * 255 / (palette_sizes[im] - 1);
                lv_color32_t col32 = {grey, grey, grey, 255};
                lv_canvas_set_palette(canv, pal_i, col32);
            }
        }
        lv_canvas_fill_bg(canv, lv_color_black(), LV_OPA_COVER);
        lv_obj_set_x(canv, im * w);
        const uint8_t * src = img_cogwheel_rgb.data;
        for (int y = 0; y < h; y++) {
            for (int x = 0; x<w; x++) {
                uint32_t pixel = *((uint32_t *) src);
                lv_canvas_set_px(canv, x, y, lv_color_make((pixel >> 16) & 0xff, (pixel >> 8) & 0xff, (pixel >> 0) & 0xff), LV_OPA_COVER);
                src += 4;
            }
        }
    }
}

[liamHowatt]

In this way, the patch can be simplified to below one.

FYI I got a weird memory error with your patch, XuNeo

[XuNeo]

That's because we used the blue channel of the color as lumaniance, which should be fixed.

There's tested Gabor added in lvgl/tests/src/test_cases/widgets/test_canvas.c.

I'll push the change directly.

@@ -111,6 +112,7 @@ void lv_canvas_set_px(lv_obj_t * obj, int32_t x, int32_t y, lv_color_t color, lv
 
     if(LV_COLOR_FORMAT_IS_INDEXED(cf)) {
         uint8_t shift;
+        uint8_t c_int = lv_color_luminance(color);
         switch(cf) {
             case LV_COLOR_FORMAT_I1:
                 shift = 7 - (x & 0x7);
@@ -130,8 +132,8 @@ void lv_canvas_set_px(lv_obj_t * obj, int32_t x, int32_t y, lv_color_t color, lv
 
         uint8_t bpp = lv_color_format_get_bpp(cf);
         uint8_t mask = (1 << bpp) - 1;
-        uint8_t c_int = color.blue & mask;
-        *data = (*data & ~(mask << shift)) | c_int << shift;
+        c_int >>= (8 - bpp);
+        *data = (*data & ~(mask << shift)) | (c_int << shift);
     }

[XuNeo]

On a second though, we should not convert color to luminance for indexed image. The color value(color.blue) means the index value.

[kisvegabor]

@liamHowatt With a little modification in your code it looks like this:

 for (int y = 0; y < h; y++) {
        for (int x = 0; x < w; x++) {
        	uint32_t pixel = *((uint32_t *) src);
        	if(ims[im] != LV_COLOR_FORMAT_L8) {
        		uint8_t idx = (pixel & 0xff) / (256 / palette_sizes[im]); /*Convert blue to an index*/
        		lv_canvas_set_px(canv, x, y, lv_color_make(0, 0, idx), LV_OPA_COVER);
        	} else {
        		lv_canvas_set_px(canv, x, y, lv_color_make((pixel >> 16) & 0xff, (pixel >> 8) & 0xff, (pixel >> 0) & 0xff), LV_OPA_COVER);
        	}
        	src += 4;
        }
}

[kisvegabor]

I cannot approve it as I added the last commit, but it looks good to me!

[liamHowatt]

Aha, I see how it works RE blue being the index.