[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 <xiawenc at linux.vnet.ibm.com>

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 <xiawenc at cn.ibm.com>
> +*
> +* 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