Re: [SeaBIOS] [PATCH v2 1/2] Boot up with a splash picture configurable

[Kevin O'Connor]

On Mon, Jun 27, 2011 at 03:10:38PM +0800, Wayne Xia wrote:
> added a bmp decoder, changed jpeg decoder to fix some problem in 24 bpp mode

Thanks.  In general it looks good to me.  I have a few minor
comments.

[...]
> --- /dev/null
> +++ b/src/bmp.c
> @@ -0,0 +1,90 @@
> +/*
> +* Basic BMP data process and Raw picture data handle functions.
> +* Could be used to adjust pixel data format, get infomation, etc.
> +*
> +* Copyright (C) 2011 Wayne Xia 
> +*
> +* This work is licensed under the terms of the GNU LGPLv3.
> +*/
> +#include "util.h"
> +#include "bmp.h"
>
> +void raw_data_format_adjust_24bpp(u8 *src, u8 *dest, int width, int height,
>        int bytes_per_line_src, int bytes_per_line_dest, u8 switch_flag)

This should be declared static.  Also, doesn't this function just
become a series of memcpy calls now that bytes_per_line_dest and
switch_flag aren't needed?

[...]
> --- /dev/null
> +++ b/src/bmp.h
> @@ -0,0 +1,78 @@
> +#ifndef BMP_H
> +#define BMP_H
> +#include "types.h"
>
> +#define WIDTHBYTES(bits) (((bits)+31)/32*4)
>
> +typedef struct tagBITMAPFILEHEADER {
> u8 bfType[2];
> u8 bfSize[4];
> u8 bfReserved1[2];
> u8 bfReserved2[2];
> u8 bfOffBits[4];
>
> +} BITMAPFILEHEADER, tagBITMAPFILEHEADER;

I think it would be preferable to move all the bmp specific
declarations into bmp.c.

[...]
> --- a/src/jpeg.c
> +++ b/src/jpeg.c
> @@ -394,10 +394,10 @@ void jpeg_get_size(struct jpeg_decdata *jpeg, int *width, int *height)
>      *height = jpeg->height;
>  }
>
> -int jpeg_show(struct jpeg_decdata *jpeg, unsigned char *pic
>          , int width, int height, int depth)
>
> +int jpeg_show(struct jpeg_decdata *jpeg, unsigned char *pic, int width
>          , int height, int depth, int bytes_per_line_dest)
>
> {
> int m, mcusx, mcusy, mx, my;
> int m, mcusx, mcusy, mx, my, mloffset, jpgbpl;

Thanks.  It would be preferable if the jpeg fixes were in a separate
patch from the bmp feature addition.

-Kevin