TimJTi commented on code in PR #16257:
URL: https://github.com/apache/nuttx/pull/16257#discussion_r2066063738
##########
include/nuttx/video/fb.h:
##########
@@ -892,47 +930,94 @@ struct fb_fix_screeninfo
struct fb_bitfield
{
- uint32_t offset; /* Beginning of bitfield */
- uint32_t length; /* Length of bitfield */
- uint32_t msb_right; /* != 0 : Most significant bit is right */
+ uint32_t offset; /* Beginning of bitfield */
+ uint32_t length; /* Length of bitfield */
+ uint32_t msb_right; /* != 0 : Most significant bit is right */
};
struct fb_var_screeninfo
{
- uint32_t xres; /* Visible resolution */
+ uint32_t xres; /* Visible resolution */
uint32_t yres;
- uint32_t xres_virtual; /* Virtual resolution */
+ uint32_t xres_virtual; /* Virtual resolution */
uint32_t yres_virtual;
- uint32_t xoffset; /* Offset from virtual to visible */
- uint32_t yoffset; /* Resolution */
- uint32_t bits_per_pixel; /* Guess what */
- uint32_t grayscale; /* 0 = color, 1 = grayscale, >1 = FOURCC */
- struct fb_bitfield red; /* Bitfield in fb mem if true color, */
- struct fb_bitfield green; /* else only length is significant */
+ uint32_t xoffset; /* Offset from virtual to visible */
+ uint32_t yoffset; /* Resolution */
+ uint32_t bits_per_pixel; /* Guess what */
+ uint32_t grayscale; /* 0 = color, 1 = grayscale, >1 = FOURCC */
+ struct fb_bitfield red; /* Bitfield in fb mem if true color, */
+ struct fb_bitfield green; /* else only length is significant */
struct fb_bitfield blue;
- struct fb_bitfield transp; /* Transparency */
- uint32_t nonstd; /* != 0 Non standard pixel format */
- uint32_t activate; /* See FB_ACTIVATE_* */
- uint32_t height; /* Height of picture in mm */
- uint32_t width; /* Width of picture in mm */
- uint32_t accel_flags; /* (OBSOLETE) See fb_info.flags */
+ struct fb_bitfield transp; /* Transparency */
+ uint32_t nonstd; /* != 0 Non standard pixel format */
+ uint32_t activate; /* See FB_ACTIVATE_* */
+ uint32_t height; /* Height of picture in mm */
+ uint32_t width; /* Width of picture in mm */
+ uint32_t accel_flags; /* (OBSOLETE) See fb_info.flags */
/* Timing: All values in pixclocks, except pixclock (of course) */
- uint32_t pixclock; /* Pixel clock in ps (pico seconds) */
- uint32_t left_margin; /* Time from sync to picture */
- uint32_t right_margin; /* Time from picture to sync */
- uint32_t upper_margin; /* Time from sync to picture */
+ uint32_t pixclock; /* Pixel clock in ps (pico seconds) */
+ uint32_t left_margin; /* Time from sync to picture */
+ uint32_t right_margin; /* Time from picture to sync */
+ uint32_t upper_margin; /* Time from sync to picture */
uint32_t lower_margin;
- uint32_t hsync_len; /* Length of horizontal sync */
- uint32_t vsync_len; /* Length of vertical sync */
- uint32_t sync; /* See FB_SYNC_* */
- uint32_t vmode; /* See FB_VMODE_* */
- uint32_t rotate; /* Angle we rotate counter clockwise */
- uint32_t colorspace; /* Colorspace for FOURCC-based modes */
- uint32_t reserved[4]; /* Reserved for future compatibility */
+ uint32_t hsync_len; /* Length of horizontal sync */
+ uint32_t vsync_len; /* Length of vertical sync */
+ uint32_t sync; /* See FB_SYNC_* */
+ uint32_t vmode; /* See FB_VMODE_* */
+ uint32_t rotate; /* Angle we rotate counter clockwise */
+ uint32_t colorspace; /* Colorspace for FOURCC-based modes */
+ uint32_t reserved[4]; /* Reserved for future compatibility */
+};
+
+#ifdef CONFIG_VIDEO_FB_SPLASHSCREEN
+# if defined(CONFIG_VIDEO_FB_SPLASHSCREEN_BPP8) || \
+ defined(CONFIG_VIDEO_FB_SPLASHSCREEN_MONO) || \
+ defined(CONFIG_VIDEO_FB_SPLASHSCREEN_GREY)
+ typedef uint8_t fb_pixel_t;
+# elif defined(CONFIG_VIDEO_FB_SPLASHSCREEN_BPP16)
+ typedef uint16_t fb_pixel_t;
+ #elif defined(CONFIG_VIDEO_FB_SPLASHSCREEN_BPP24)
+ typedef uint32_t fb_pixel_t;
+# elif defined(CONFIG_VIDEO_FB_SPLASHSCREEN_BPP32)
+ typedef uint32_t fb_pixel_t;
+# else
+# error "Pixel depth is unknown"
+#endif
+
+/* Describes a point on the display */
+
+struct fb_point_s
+{
+ fb_coord_t x; /* X position, range: 0 to screen width - 1 */
+ fb_coord_t y; /* Y position, range: 0 to screen height - 1 */
};
+struct fb_rect_s
+{
+ struct fb_point_s pt1; /* Upper, left-hand corner */
+ struct fb_point_s pt2; /* Lower, right-hand corner */
+};
+
+/* This structure describes the splashscreen */
+
+struct splscr_bitmap_s
+{
+ uint8_t npixels; /* Number of pixels */
+ uint8_t lookup; /* Pixel RGB lookup index */
+};
+
+struct palette_bitmap_s
Review Comment:
@xiaoxiang781216
Here is my reasoning.
1. LVGL is not Apache and I didn't want to _rely_ on a 3rd party tool. You
may use LVGL (and I do, actually) and have an inclination to use it's tools but
not everyone feels the same
2. LVGL output files do not meet nxstyle rules. Especially since this
implementation is for the kernel not just an app (where excuses might be
made?), it has to pass nxstyle.
3. LVGL tool does not create greyscale or monochrome outputs, meaning
another 3rd party tool is needed before an LVGL c file creation.
4. We want an easy "works out of the box" solution for newcomers to NuttX,
so I wanted to ensure the bitmap size and colour mapping could be selected by
Kconfig, meaning all c files have to exist. A quick test shows the LVGL-created
c files for 80x80, 160x160 and 320x320, in ARGB8888, RGB888 and RGB565 occupy
4x the disk space of the RLE file created by "my" python converter. I like
small.
5. I initially started to use the existing host-side tool
(bitmap_converter.py) as described
[here](https://nuttx.apache.org/docs/latest/applications/tools/index.html) but
found:
1. The files created did not meet nxstyle rules
2. the python script itself failed nxstyle and CI.
3. It did not run using Python3. I submitted a patch for this which is
when I found it failed CI...requested assistance...and none was forthcoming so
I gave up on it and just used it as a base for my own (having to learn some
Python along the way!) and found out about `black` etc. this way. See
[this](https://github.com/apache/nuttx/issues/16264) PR though!
6. I could not find any NuttX bitmap structs other than the app-side structs
used by NXwidgets, SRlePaletteBitmapEntry and SRlePaletteBitmap. These are c++
headers, and not suitable without changing them to include kernel-side. But
they were a good basis and I used C-equivalents to start with, and only deleted
the first three struct members (bpp, fmt and nlut) late-on as they were
superfluous.
7. This is intended as a quick "NuttX is running, yay!" type splashscreen,
not for an app-side, full colour, beautiful corporate multi-colour graphic. If
that's wanted, the User app can provide it - like I do in LVGL once my "real"
app is running with only a need for one full quality ARGB888 c file (which
fails nxstyle...). I want to keep it simple and compact to suit as many
architectures as possible (so also no direct reading in and decoding of jpeg,
etc, from MTD or other storage media.
So, on balance, I really don't think the use of the LVGL tool itself is
right at all, and I have explained my reasons for the format I chose.
I can see an argument for using the LVGL image description struct:
```
const lv_image_dsc_t imagebutton_right = {
.header.w = 8,
.header.h = 50,
.header.stride = 32,
.header.cf = LV_COLOR_FORMAT_ARGB8888,
.data = imagebutton_right_map,
.data_size = sizeof(imagebutton_right_map),
};
```
This has unnecessary information compared to my more simple version but it
is fine, so hey, why not - but the only actual benefit I can think of is to
allow the use of an LVGL-created image c file rather than the NuttX RLE file,
as you suggested.
Which - to me - suggests it comes down to this perhaps:
1. do we want this kernel-side start up splashscreen to only support RLE
images, to keep the size small
2. or do we want to support full bitmap files, with no RLE, to allow the
likes of LVGL tools to be used
3. or, via Kconfig, a choice of either
```
To fully implement 2. or 3. we would, I think, need to:
- Add macros to convert ARGB to greyscale and mono. Not difficult.
- Stipulate that the converted full-bitmap file is ARGB888
- Use the Kconfig choice of colour format to ensure the decode uses the
correct conversion macro (a bit like this PR does already with the MKRGB macro
etc).
So not onerous.
```
If the answer is 3, may I suggest we leave this PR as-is (as it hopefully
meets the requirements of option 1 and half of option 3 and it can be revisited
with an upgrade to add alternative support for full bitmap c files?
If I'm missing something, or misunderstood, please say so!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
