Re: [SeaBIOS] [PATCH] Enhance seabios splash picture showing ability

[Kevin O'Connor]

On Fri, Jun 17, 2011 at 05:35:13PM +0800, Wayne Xia wrote:
> Signed-off-by: Wayne Xia

Thanks - see my comments below.

[...]
> --- /dev/null
> +++ b/src/bmp.c
> @@ -0,0 +1,71 @@
> +/*
> +* Basic BMP data process functions.
> +* Could used to adjust pixel data format, get infomation, etc.
> +*
> +* Copyright (C) 2011 Wayne Xia 
> +*
> +* This work is licensed under the terms of the GNU GPL, version 2 or later.
> +*/

SeaBIOS is licensed under LGPLv3.

[...]
> --- /dev/null
> +++ b/src/bmp.h
> @@ -0,0 +1,55 @@
> +#ifndef BMP_H
> +#define BMP_H
> +#include "types.h"
>
> +#define WIDTHBYTES(bits) (((bits)+31)/32*4)
>
> +typedef u8 BYTE;
> +typedef u32 BYTE4;

Can u32/u8 be put into the struct definitions themselves instead of
adding these typedefs?

[...]
> --- a/src/boot.c
> +++ b/src/boot.c
> @@ -372,23 +372,56 @@ boot_add_cbfs(void *data, const char *desc, int prio)
>  static void
>  interactive_bootmenu(void)
>  {
> if (! CONFIG_BOOTMENU || ! qemu_cfg_show_boot_menu())
> int filecfg_size;
> u8 *filecfg_p = romfile_loadfile("bootsplash.cfg", &filecfg_size);
> u8 showflag_normal;
> u16 splash_time;
> if (filecfg_p != NULL && filecfg_size >= 4) {
>    showflag_normal = *filecfg_p;
>    splash_time = *(filecfg_p+2)+(*(filecfg_p+3)<<8); /* little endian */
>    free(filecfg_p);

Could you separate out the time configuration into a separate patch
from the BMP viewing?

[...]
> while (get_keystroke(0) >= 0)
> while (get_keystroke(0) >= 0) {
>     ;
> }

The SeaBIOS style is to not use braces around one line statements.
Should style changes be necessary to existing code it should go in a
separate patch.

[...]
> --- a/src/bootsplash.c
> +++ b/src/bootsplash.c

[...]
> dprintf(5, "Decompressing bootsplash.jpg\n");
> ret = jpeg_show(jpeg, picture, width, height, depth);
> if (ret) {
>    dprintf(1, "jpeg_show failed with return code %d...\n", ret);
>    goto done;
> if (type == 0) {
>    dprintf(5, "Decompressing bootsplash.data\n");
>    ret = jpeg_show(jpeg, picture, width, height, depth);
>    if (ret) {
>        dprintf(1, "jpeg_show failed with return code %d...\n", ret);
>        goto done;
>    }
>    picture_post = picture;
>    post_flag |= SWITCH_RGB;
>    if (depth == 24 && mode_info->bytes_per_scanline >= width * 24 / 8) {
>        bmp_data_format_proc_24bpp(picture_post, picture, width, height,
>                width * 24 / 8, mode_info->bytes_per_scanline, post_flag);
>    }

I'm confused by this - why is the jpeg path calling the bmp_x
function?
> } else {
>    picture_post = malloc_tmphigh(imagesize);
>    if (!picture_post) {
>        warn_noalloc();
>        goto done;
>    }
>    memcpy(picture_post, filedata+data_offset, filesize-data_offset);
>    post_flag |= SWITCH_LINE;
>    if (depth == 24 && mode_info->bytes_per_scanline >= width * 24 / 8) {
>        bmp_data_format_proc_24bpp(picture_post, picture, width, height,
>                width * 24 / 8, mode_info->bytes_per_scanline, post_flag);
>    }
>    free(picture_post);
> }

Can the above be collappsed into a function bmp_show() so that it more
closely mirrors the jpeg_show() code path?

Thanks again,
-Kevin