Re: [PATCH] jpeg image reader

["Vesa Jääskeläinen" <chaac@nic.fi>]

Bean wrote:
> Hi,
> 
> I just fix a small bug that can cause color distortion, here is the new patch.

Nice job for implementing JPEG. This is the major reason I made the 
bitmap loader interface, to allow other people to write loaders... now 
if we would just have PNG support ;)

I will not comment on the JPEG format or any possible IPR issues as I am 
not too clear on that myself, just the patch itself.

I would have hoped a bit more comments. Check your tabs and spaces. 
Re-check array handling.

I know lots of JPEG loaders seems to use longjmp for error handling, 
personally I don't like it, but I suppose it is ok. In grub2 you could 
just check if grub_errno != GRUB_ERR_NONE and then just return and at 
that level do the same check. This mechanism works a bit like c++'s 
exceptions.

Another thing is that if there is a problem loading JPEG, user gets a 
message like "invalid marker: 1234", this doesn't really tell user a 
thing. So I would propose to use jpeg prefix like "jpeg: invalid 
marker...", or more descriptive from user's perspective.

> diff --git a/conf/i386-pc.rmk b/conf/i386-pc.rmk
> index a46bc74..4beeeff 100644
> --- a/conf/i386-pc.rmk
> +++ b/conf/i386-pc.rmk
> @@ -136,7 +136,7 @@ pkglib_MODULES = biosdisk.mod _chain.mod
> _linux.mod linux.mod normal.mod \
>  	_multiboot.mod chain.mod multiboot.mod reboot.mod halt.mod	\
>  	vbe.mod vbetest.mod vbeinfo.mod video.mod gfxterm.mod \
>  	videotest.mod play.mod bitmap.mod tga.mod cpuid.mod serial.mod	\
> -	ata.mod vga.mod
> +	ata.mod vga.mod jpeg.mod
> 
>  # For biosdisk.mod.
>  biosdisk_mod_SOURCES = disk/i386/pc/biosdisk.c
> @@ -249,6 +249,11 @@ tga_mod_SOURCES = video/readers/tga.c
>  tga_mod_CFLAGS = $(COMMON_CFLAGS)
>  tga_mod_LDFLAGS = $(COMMON_LDFLAGS)
> 
> +# For jpeg.mod
> +jpeg_mod_SOURCES = video/readers/jpeg.c
> +jpeg_mod_CFLAGS = $(COMMON_CFLAGS)
> +jpeg_mod_LDFLAGS = $(COMMON_LDFLAGS)
> +
>  # For cpuid.mod.
>  cpuid_mod_SOURCES = commands/i386/cpuid.c
>  cpuid_mod_CFLAGS = $(COMMON_CFLAGS)
> diff --git a/video/readers/jpeg.c b/video/readers/jpeg.c
> new file mode 100755
> index 0000000..bcbf820
> --- /dev/null
> +++ b/video/readers/jpeg.c
> @@ -0,0 +1,734 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2008  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/bitmap.h>
> +#include <grub/types.h>
> +#include <grub/normal.h>
> +#include <grub/dl.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +#include <grub/arg.h>
> +#include <grub/file.h>
> +#include <grub/setjmp.h>
> +
> +/* Uncomment following define to enable JPEG debug.  */
> +//#define JPEG_DEBUG
> +
> +struct grub_jpeg_data
> +{
> +  grub_file_t file;
> +  grub_jmp_buf jumper;
> +
> +  int image_width;
> +  int image_height;
> +
> +  grub_uint8_t *huff_value[4];
> +  int huff_offset[4][16];
> +  int huff_maxval[4][16];
> +
> +  grub_uint8_t quan_table[2][64];
> +  int comp_index[3][3];
> +
> +  int ydu[4][64], cbdu[64], crdu[64];
> +
> +  int Cr_r_tab[256], Cb_b_tab[256], Cr_g_tab[256], Cb_g_tab[256];
> +
> +  int dc_value[3];
> +
> +  int bit_mask, bit_save;
> +
> +  struct grub_video_bitmap **bitmap;
> +};
> +
> +static grub_uint8_t
> +get_byte (struct grub_jpeg_data *data)
> +{
> +  grub_uint8_t r;
> +
> +  if (grub_file_read (data->file, (char *) &r, 1) != 1)
> +    grub_longjmp (data->jumper, 1);
> +
> +  return r;
> +}
> +
> +static unsigned

unsigned -> grub_uint16_t

> +get_word (struct grub_jpeg_data *data)
> +{
> +  unsigned aa, bb;
> +
> +  aa = get_byte (data);
> +  bb = get_byte (data);
> +  return (aa << 8) + bb;
> +}
> +
> +static int
> +get_bit (struct grub_jpeg_data *data)
> +{
> +  int ret;
> +
> +  if (data->bit_mask == 0)
> +    {
> +      data->bit_save = get_byte (data);
> +      if (data->bit_save == 0xFF)
> +	{
> +	  if (get_byte (data) != 0)
> +	    {
> +	      grub_error (GRUB_ERR_BAD_FILE_TYPE,
> +			  "invalid 0xFF in data stream");
> +	      grub_longjmp (data->jumper, 1);
> +	    }
> +	}
> +      data->bit_mask = 0x80;
> +    }
> +
> +  ret = (data->bit_save & data->bit_mask);
> +  data->bit_mask >>= 1;
> +  return ret;
> +}
> +
> +static int
> +get_huff_leng (struct grub_jpeg_data *data, int num)

_length?

> +{
> +  int value, i, bit;
> +
> +  if (num == 0)
> +    return 0;
> +
> +  bit = get_bit (data);
> +  value = (bit != 0);
> +  for (i = 1; i < num; i++)
> +    value = value * 2 + (get_bit (data) != 0);
> +  if (!bit)
> +    value += 1 - (1 << num);
> +
> +  return value;
> +}
> +
> +static int
> +get_huff_base (struct grub_jpeg_data *data, int id)
> +{
> +  int code, i;
> +
> +  code = 0;
> +  for (i = 0; i < 16; i++)
> +    {
> +      code <<= 1;
> +      if (get_bit (data))
> +	code++;
> +      if (code < data->huff_maxval[id][i])
> +	return data->huff_value[id][code + data->huff_offset[id][i]];
> +    }
> +  grub_error (GRUB_ERR_BAD_FILE_TYPE, "huffman decode fails");
> +  grub_longjmp (data->jumper, 1);
> +  return 0;
> +}
> +
> +static void
> +decode_huff_table (struct grub_jpeg_data *data)
> +{
> +  unsigned id;

unsigned -> grub_uint8_t

> +  int ac, i, n, base, ofs;
> +  grub_uint8_t count[16];
> +
> +  get_word (data);

not important word?

> +  id = get_byte (data);
> +  ac = (id >> 4);
> +  id &= 0xF;
> +  if (id > 1)
> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE, "too many huff table");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  if (grub_file_read (data->file, (char *) &count, sizeof (count)) !=
> +      sizeof (count))
> +    grub_longjmp (data->jumper, 1);
> +
> +  n = 0;
> +  for (i = 0; i < 16; i++)
> +    n += count[i];
> +
> +  id += ac * 2;
> +  if ((data->huff_value[id] = grub_malloc (n)) == NULL)

try to keep malloc on own line and then check for pointer or for 
grub_errno (or both).

> +    grub_longjmp (data->jumper, 1);
> +
> +  if (grub_file_read (data->file, (char *) data->huff_value[id], n) != n)
> +    grub_longjmp (data->jumper, 1);
> +
> +  base = 0;
> +  ofs = 0;
> +  for (i = 0; i < 16; i++)
> +    {
> +      base += count[i];
> +      ofs += count[i];
> +
> +      data->huff_maxval[id][i] = base;
> +      data->huff_offset[id][i] = ofs - base;
> +
> +      base <<= 1;
> +    }
> +}
> +
> +static void
> +decode_quan_table (struct grub_jpeg_data *data)
> +{
> +  unsigned id;
> +
> +  get_word (data);
> +  id = get_byte (data);
> +  if (id >= 0x10)

0x10?

> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE,
> +		  "only 8-bit precision is supported");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  if (id > 1)
> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE, "too many quantization table");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  if (grub_file_read (data->file, (char *) &data->quan_table[id], 64) != 64)
> +    grub_longjmp (data->jumper, 1);
> +}
> +
> +static void
> +decode_sof (struct grub_jpeg_data *data)
> +{
> +  int nc, i;
> +
> +  get_word (data);
> +  if (get_byte (data) != 8)
> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE,
> +		  "only 8-bit precision is supported");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  data->image_height = get_word (data);
> +  data->image_width = get_word (data);
> +
> +  if ((!data->image_height) || (!data->image_width))
> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE, "invalid image size");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  nc = get_byte (data);
> +  if (nc != 3)
> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE, "component count must be 3");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  for (i = 0; i < nc; i++)
> +    {
> +      int id, ss;
> +
> +      id = get_byte (data) - 1;
> +      ss = get_byte (data);
> +      if (((!id) && (ss != 0x22)) || ((id) && (ss != 0x11)))
> +	{
> +	  grub_error (GRUB_ERR_BAD_FILE_TYPE, "only support 2:1:1 sampling");
> +	  grub_longjmp (data->jumper, 1);
> +	}
> +      data->comp_index[id][0] = get_byte (data);
Buffer overflow possibility.
> +    }
> +}
> +
> +#define DCTSIZE     8
> +
> +#define CONST_BITS  8
> +#define PASS1_BITS  2
> +
> +#define FIX_1_082392200  ((int)  277)	/* FIX(1.082392200) */
> +#define FIX_1_414213562  ((int)  362)	/* FIX(1.414213562) */
> +#define FIX_1_847759065  ((int)  473)	/* FIX(1.847759065) */
> +#define FIX_2_613125930  ((int)  669)	/* FIX(2.613125930) */
> +
> +#define ONE	((long) 1)
> +#define DESCALE(x,n)  (((x) + (ONE << ((n)-1))) >> (n))
> +
> +#define MULTIPLY(var,const)  DESCALE((var) * (const), CONST_BITS)
> +
> +static const unsigned char natural_order[64] = {

grub_uint8_t, or just (unsigned) int

> +  0, 1, 8, 16, 9, 2, 3, 10,
> +  17, 24, 32, 25, 18, 11, 4, 5,
> +  12, 19, 26, 33, 40, 48, 41, 34,
> +  27, 20, 13, 6, 7, 14, 21, 28,
> +  35, 42, 49, 56, 57, 50, 43, 36,
> +  29, 22, 15, 23, 30, 37, 44, 51,
> +  58, 59, 52, 45, 38, 31, 39, 46,
> +  53, 60, 61, 54, 47, 55, 62, 63
> +};
> +
> +static const short aanscales[64] = {

use just int (as it is anyway casted to int, and I don't think it is 
necessary to save some bytes on here from module.

> +  /* precomputed values scaled up by 14 bits */
> +  16384, 22725, 21407, 19266, 16384, 12873, 8867, 4520,
> +  22725, 31521, 29692, 26722, 22725, 17855, 12299, 6270,
> +  21407, 29692, 27969, 25172, 21407, 16819, 11585, 5906,
> +  19266, 26722, 25172, 22654, 19266, 15137, 10426, 5315,
> +  16384, 22725, 21407, 19266, 16384, 12873, 8867, 4520,
> +  12873, 17855, 16819, 15137, 12873, 10114, 6967, 3552,
> +  8867, 12299, 11585, 10426, 8867, 6967, 4799, 2446,
> +  4520, 6270, 5906, 5315, 4520, 3552, 2446, 1247
> +};
> +
> +static void
> +idct_transform (int *du)
> +{
> +  int *pd;
> +  int ctr;
> +  int tmp0, tmp1, tmp2, tmp3, tmp4, tmp5, tmp6, tmp7;
> +  int tmp10, tmp11, tmp12, tmp13;
> +  int z5, z10, z11, z12, z13;
> +
> +  /* Pass 1: process columns from input, store into work array. */

Extra space after last dot.

> +
> +  pd = du;
> +  for (ctr = DCTSIZE; ctr > 0; ctr--)
> +    {
> +      if ((pd[DCTSIZE * 1] | pd[DCTSIZE * 2] | pd[DCTSIZE * 3] |
> +	   pd[DCTSIZE * 4] | pd[DCTSIZE * 5] | pd[DCTSIZE * 6] |
> +	   pd[DCTSIZE * 7]) == 0)
> +	{
> +	  pd[DCTSIZE * 1] = pd[DCTSIZE * 2]
> +	    = pd[DCTSIZE * 3] = pd[DCTSIZE * 4]
> +	    = pd[DCTSIZE * 5] = pd[DCTSIZE * 6]
> +	    = pd[DCTSIZE * 7] = pd[DCTSIZE * 0];
> +
> +	  pd++;			/* advance pointers to next column */
> +	  continue;
> +	}
> +
> +      /* Even part */
> +
> +      tmp0 = pd[DCTSIZE * 0];
> +      tmp1 = pd[DCTSIZE * 2];
> +      tmp2 = pd[DCTSIZE * 4];
> +      tmp3 = pd[DCTSIZE * 6];
> +
> +      tmp10 = tmp0 + tmp2;	/* phase 3 */
> +      tmp11 = tmp0 - tmp2;
> +
> +      tmp13 = tmp1 + tmp3;	/* phases 5-3 */
> +      tmp12 = MULTIPLY (tmp1 - tmp3, FIX_1_414213562) - tmp13;	/* 2*c4 */
> +
> +      tmp0 = tmp10 + tmp13;	/* phase 2 */
> +      tmp3 = tmp10 - tmp13;
> +      tmp1 = tmp11 + tmp12;
> +      tmp2 = tmp11 - tmp12;
> +
> +      /* Odd part */
> +
> +      tmp4 = pd[DCTSIZE * 1];
> +      tmp5 = pd[DCTSIZE * 3];
> +      tmp6 = pd[DCTSIZE * 5];
> +      tmp7 = pd[DCTSIZE * 7];
> +
> +      z13 = tmp6 + tmp5;	/* phase 6 */
> +      z10 = tmp6 - tmp5;
> +      z11 = tmp4 + tmp7;
> +      z12 = tmp4 - tmp7;
> +
> +      tmp7 = z11 + z13;		/* phase 5 */
> +      tmp11 = MULTIPLY (z11 - z13, FIX_1_414213562);	/* 2*c4 */
> +
> +      z5 = MULTIPLY (z10 + z12, FIX_1_847759065);	/* 2*c2 */
> +      tmp10 = MULTIPLY (z12, FIX_1_082392200) - z5;	/* 2*(c2-c6) */
> +      tmp12 = MULTIPLY (z10, -FIX_2_613125930) + z5;	/* -2*(c2+c6) */
> +
> +
> +      tmp6 = tmp12 - tmp7;	/* phase 2 */
> +      tmp5 = tmp11 - tmp6;
> +      tmp4 = tmp10 + tmp5;
> +
> +      pd[DCTSIZE * 0] = (int) (tmp0 + tmp7);
> +      pd[DCTSIZE * 7] = (int) (tmp0 - tmp7);
> +      pd[DCTSIZE * 1] = (int) (tmp1 + tmp6);
> +      pd[DCTSIZE * 6] = (int) (tmp1 - tmp6);
> +      pd[DCTSIZE * 2] = (int) (tmp2 + tmp5);
> +      pd[DCTSIZE * 5] = (int) (tmp2 - tmp5);
> +      pd[DCTSIZE * 4] = (int) (tmp3 + tmp4);
> +      pd[DCTSIZE * 3] = (int) (tmp3 - tmp4);
> +
> +      pd++;			/* advance pointers to next column */
> +    }
> +
> +  /* Pass 2: process rows from work array, store into output array. */
> +  /* Note that we must descale the results by a factor of 8 == 2**3, */
> +  /* and also undo the PASS1_BITS scaling. */
> +
> +  pd = du;
> +  for (ctr = 0; ctr < DCTSIZE; ctr++)
> +    {
> +      /* Even part */
> +
> +      tmp10 = pd[0] + pd[4];
> +      tmp11 = pd[0] - pd[4];
> +
> +      tmp13 = pd[2] + pd[6];
> +      tmp12 = MULTIPLY (pd[2] - pd[6], FIX_1_414213562) - tmp13;
> +
> +      tmp0 = tmp10 + tmp13;
> +      tmp3 = tmp10 - tmp13;
> +      tmp1 = tmp11 + tmp12;
> +      tmp2 = tmp11 - tmp12;
> +
> +      /* Odd part */
> +
> +      z13 = pd[5] + pd[3];
> +      z10 = pd[5] - pd[3];
> +      z11 = pd[1] + pd[7];
> +      z12 = pd[1] - pd[7];
> +
> +      tmp7 = z11 + z13;		/* phase 5 */
> +      tmp11 = MULTIPLY (z11 - z13, FIX_1_414213562);	/* 2*c4 */
> +
> +      z5 = MULTIPLY (z10 + z12, FIX_1_847759065);	/* 2*c2 */
> +      tmp10 = MULTIPLY (z12, FIX_1_082392200) - z5;	/* 2*(c2-c6) */
> +      tmp12 = MULTIPLY (z10, -FIX_2_613125930) + z5;	/* -2*(c2+c6) */
> +
> +      tmp6 = tmp12 - tmp7;	/* phase 2 */
> +      tmp5 = tmp11 - tmp6;
> +      tmp4 = tmp10 + tmp5;
> +
> +      /* Final output stage: scale down by a factor of 8 and range-limit */
> +
> +      pd[0] = DESCALE (tmp0 + tmp7, PASS1_BITS + 3) + 128;
> +      pd[7] = DESCALE (tmp0 - tmp7, PASS1_BITS + 3) + 128;
> +      pd[1] = DESCALE (tmp1 + tmp6, PASS1_BITS + 3) + 128;
> +      pd[6] = DESCALE (tmp1 - tmp6, PASS1_BITS + 3) + 128;
> +      pd[2] = DESCALE (tmp2 + tmp5, PASS1_BITS + 3) + 128;
> +      pd[5] = DESCALE (tmp2 - tmp5, PASS1_BITS + 3) + 128;
> +      pd[4] = DESCALE (tmp3 + tmp4, PASS1_BITS + 3) + 128;
> +      pd[3] = DESCALE (tmp3 - tmp4, PASS1_BITS + 3) + 128;
> +
> +      pd += DCTSIZE;		/* advance pointer to next row */
> +    }
> +
> +  for (ctr = 0; ctr < 64; ctr++)
> +    {
> +      if (du[ctr] < 0)
> +	du[ctr] = 0;
> +      if (du[ctr] > 255)
> +	du[ctr] = 255;
> +    }
> +}
> +
> +static void
> +decode_du (struct grub_jpeg_data *data, int id, int *du)
> +{
> +  int pos, h1, h2, qt;
> +
> +  grub_memset (du, 0, sizeof (int) * 64);

du is always that size ? perhaps add warning about it to prototype, or 
provide length?

I didn't check for indexes below, verify them.
> +
> +  qt = data->comp_index[id][0];
> +  h1 = data->comp_index[id][1];
> +  h2 = data->comp_index[id][2];
> +
> +  data->dc_value[id] += get_huff_leng (data, get_huff_base (data, h1));
> +
> +  du[0] =
> +    data->dc_value[id] * DESCALE ((int) data->quan_table[qt][0] *
> +				  (int) aanscales[0], 14 - 2);
> +  pos = 1;
> +  while (pos < 64)
> +    {
> +      int num, val;
> +
> +      num = get_huff_base (data, h2);
> +      if (!num)
> +	break;
> +      val = get_huff_leng (data, num & 0xF);
> +      num >>= 4;
> +      pos += num;
> +      du[natural_order[pos]] =
> +	val * DESCALE ((int) data->quan_table[qt][pos] *
> +		       (int) aanscales[natural_order[pos]], 14 - 2);
> +      pos++;
> +    }
> +
> +  idct_transform (du);
> +}
> +
> +#define SCALEBITS	16	/* speediest right-shift on some machines */
> +#define ONE_HALF	((int) 1 << (SCALEBITS-1))
> +#define FIX(x)		((int) ((x) * (1L<<SCALEBITS) + 0.5))
> +
> +static void
> +build_color_table (struct grub_jpeg_data *data)
> +{
> +  int i, x;
> +
> +  for (i = 0, x = -128; i <= 255; i++, x++)
> +    {
> +      data->Cr_r_tab[i] = (int) (FIX (1.40200) * x + ONE_HALF) >> SCALEBITS;
> +      data->Cb_b_tab[i] = (int) (FIX (1.77200) * x + ONE_HALF) >> SCALEBITS;
> +      data->Cr_g_tab[i] = (-FIX (0.71414)) * x;
> +      data->Cb_g_tab[i] = (-FIX (0.34414)) * x + ONE_HALF;
> +    }
> +}
> +
> +static void
> +decode_sos (struct grub_jpeg_data *data)
> +{
> +  int nc, i, r1, c1;
> +  grub_uint8_t *ptr;
> +
> +  get_word (data);

No use for first word?

> +  nc = get_byte (data);
> +
> +  if (nc != 3)
> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE, "component count must be 3");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  for (i = 0; i < nc; i++)
> +    {
> +      int id, ht;
> +
> +      id = get_byte (data) - 1;
> +      ht = get_byte (data);

> +      data->comp_index[id][1] = (ht >> 4);
> +      data->comp_index[id][2] = (ht & 0xF) + 2;
Buffer overflows.

> +    }
> +
> +  get_byte (data);
> +  get_word (data);
> +
> +  if (grub_video_bitmap_create (data->bitmap, data->image_width,
> +				data->image_height,
> +				GRUB_VIDEO_BLIT_FORMAT_R8G8B8))
> +    grub_longjmp (data->jumper, 1);
> +
> +  ptr = (*data->bitmap)->data;
> +  data->bit_mask = 0x0;
> +
> +  for (r1 = 0; r1 < (data->image_height >> 4); r1++)
> +    for (c1 = 0; c1 < (data->image_width >> 4); c1++)
> +      {
> +	int r2, c2, r3, c3;
> +
> +	decode_du (data, 0, &data->ydu[0][0]);
> +	decode_du (data, 0, &data->ydu[1][0]);
> +
> +	decode_du (data, 0, &data->ydu[2][0]);
> +	decode_du (data, 0, &data->ydu[3][0]);
> +
> +	decode_du (data, 1, &data->cbdu[0]);
> +	decode_du (data, 2, &data->crdu[0]);
> +
> +	for (r2 = 0; r2 < 2; r2++)
> +	  for (c2 = 0; c2 < 2; c2++)
> +	    for (r3 = 0; r3 < 8; r3++)
> +	      for (c3 = 0; c3 < 8; c3++)
> +		{
> +		  int i0, i1, i2, cr, cb, dd;
> +
> +		  i0 =
> +		    ((r1 * 16 + r2 * 8 + r3) * (data->image_width) + c1 * 16 +
> +		     c2 * 8 + c3) * 3;
> +		  i1 = r3 * 8 + c3;
> +		  i2 = (r2 * 32 + c2 * 4) + (r3 >> 1) * 8 + (c3 >> 1);

I have following indexes are correct.... (didn't check them)

> +
> +		  cr = data->crdu[i2];
> +		  cb = data->cbdu[i2];
> +
> +		  // Red

Try to use C comments. /* ...

> +		  dd = data->ydu[r2 * 2 + c2][i1] + data->Cr_r_tab[cr];
> +		  if (dd < 0)
> +		    dd = 0;
> +		  ptr[i0] = dd;
> +
> +		  // Green
> +		  dd =
> +		    data->ydu[r2 * 2 + c2][i1] +
> +		    ((data->Cb_g_tab[cb] + data->Cr_g_tab[cr]) >> SCALEBITS);
> +		  if (dd < 0)
> +		    dd = 0;
> +		  ptr[i0 + 1] = dd;
> +
> +		  // Blue
> +		  dd = data->ydu[r2 * 2 + c2][i1] + data->Cb_b_tab[cb];
> +		  if (dd < 0)
> +		    dd = 0;
> +		  ptr[i0 + 2] = dd;
> +		}
> +      }
> +}
> +
> +
> +static grub_uint8_t
> +get_marker (struct grub_jpeg_data *data)
> +{
> +  grub_uint8_t r;
> +
> +  r = get_byte (data);
> +
> +  if (r != 0xFF)
0xFF?
> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE, "invalid maker");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  return get_byte (data);
> +}
> +
> +static void
> +decode_jpeg (struct grub_jpeg_data *data)
> +{
> +  build_color_table (data);
> +
> +  if (get_marker (data) != 0xD8)

0xD8?

> +    {
> +      grub_error (GRUB_ERR_BAD_FILE_TYPE, "invalid jpeg file");
> +      grub_longjmp (data->jumper, 1);
> +    }
> +
> +  while (1)
> +    {
> +      grub_uint8_t marker;
> +
> +      marker = get_marker (data);
> +#ifdef JPEG_DEBUG
> +      grub_printf ("marker: %x\n", marker);
> +#endif
> +      switch (marker)
> +	{
> +	case 0xc4:
0xC4 ?
> +	  decode_huff_table (data);
> +	  break;
> +	case 0xdb:
Same...
> +	  decode_quan_table (data);
> +	  break;
> +	case 0xc0:
> +	  decode_sof (data);
> +	  break;
> +	case 0xda:
> +	  decode_sos (data);
> +	  break;
> +	case 0xd9:
> +	  return;
> +	case 0xe0:
> +	case 0xfe:
> +	  {
> +	    unsigned sz;

unsigned -> grub_uint16_t

> +
> +	    sz = get_word (data);
> +	    grub_file_seek (data->file, data->file->offset + sz - 2);
> +	    break;
> +	  }
> +	default:
> +	  grub_error (GRUB_ERR_BAD_FILE_TYPE, "unrecognized marker %x",
> +		      marker);
> +	  grub_longjmp (data->jumper, 1);
> +	}
> +    }
> +}
> +
> +static grub_err_t
> +grub_video_reader_jpeg (struct grub_video_bitmap **bitmap,
> +			const char *filename)
> +{
> +  grub_file_t file;
> +  struct grub_jpeg_data *data;
> +
> +  file = grub_file_open (filename);
> +  if (!file)
> +    return grub_errno;
> +
> +  if ((data = grub_malloc (sizeof (*data))) != NULL)

Separate as two lines.

> +    {
> +      int i;
> +
> +      grub_memset (data, 0, sizeof (*data));
> +      data->file = file;
> +      data->bitmap = bitmap;
> +      if (!grub_setjmp (data->jumper))
> +	decode_jpeg (data);
> +
> +      for (i = 0; i < 4; i++)
> +	if (data->huff_value[i])
> +	  grub_free (data->huff_value[i]);
> +      grub_free (data);
> +    }
> +
> +  if ((grub_errno != GRUB_ERR_NONE) && (*bitmap))

No need to check for *bitmap

> +    {
> +      grub_video_bitmap_destroy (*bitmap);
> +      *bitmap = 0;
> +    }
> +
> +  grub_file_close (file);
> +  return grub_errno;
> +}
> +
> +#if defined(JPEG_DEBUG)
> +static grub_err_t
> +grub_cmd_jpegtest (struct grub_arg_list *state __attribute__ ((unused)),
> +		   int argc, char **args)
> +{
> +  struct grub_video_bitmap *bitmap = 0;
> +
> +  if (argc != 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "file name required");
> +
> +  grub_video_reader_jpeg (&bitmap, args[0]);
> +  if (grub_errno != GRUB_ERR_NONE)
> +    return grub_errno;
> +
> +  grub_video_bitmap_destroy (bitmap);
> +
> +  return GRUB_ERR_NONE;
> +}
> +#endif
> +
> +static struct grub_video_bitmap_reader jpg_reader = {
> +  .extension = ".jpg",
> +  .reader = grub_video_reader_jpeg,
> +  .next = 0
> +};
> +
> +static struct grub_video_bitmap_reader jpeg_reader = {
> +  .extension = ".jpeg",
> +  .reader = grub_video_reader_jpeg,
> +  .next = 0
> +};
> +
> +GRUB_MOD_INIT (video_reader_jpeg)
> +{
> +  grub_video_bitmap_reader_register (&jpg_reader);
> +  grub_video_bitmap_reader_register (&jpeg_reader);
> +#if defined(JPEG_DEBUG)
> +  grub_register_command ("jpegtest", grub_cmd_jpegtest,
> +			 GRUB_COMMAND_FLAG_BOTH, "jpegtest FILE",
> +			 "Tests loading of JPEG bitmap.", 0);
> +#endif
> +}
> +
> +GRUB_MOD_FINI (video_reader_jpeg)
> +{
> +#if defined(JPEG_DEBUG)
> +  grub_unregister_command ("jpegtest");
> +#endif
> +  grub_video_bitmap_reader_unregister (&jpeg_reader);
> +  grub_video_bitmap_reader_unregister (&jpg_reader);
> +}
> 
>