[PATCH] GSoC #09 Bitmap scaling

[PATCH] GSoC #09 Bitmap scaling

[Colin D Bennett]

[-- Attachment #1.1: Type: text/plain, Size: 274 bytes --]

This patch adds image scaling support.  Nearest neighbor and bilinear
interpolation algorithms are supported.

The gfxterm background_image command scales the background image to fit
the screen.  This can be controlled with the new --mode/-m option.

Regards,
Colin

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 09_bitmap-scaling.patch --]
[-- Type: text/x-patch; name=09_bitmap-scaling.patch, Size: 21156 bytes --]

=== modified file 'conf/i386-pc.rmk'
--- conf/i386-pc.rmk	2008-08-30 19:20:13 +0000
+++ conf/i386-pc.rmk	2008-08-31 03:00:53 +0000
@@ -271,7 +271,10 @@
 videotest_mod_LDFLAGS = $(COMMON_LDFLAGS)
 
 # For bitmap.mod
-bitmap_mod_SOURCES = video/bitmap.c
+bitmap_mod_SOURCES = video/bitmap.c \
+                     video/bitmap_scale_nn.c \
+                     video/bitmap_scale_bilinear.c \
+                     video/bitmap_scale.c
 bitmap_mod_CFLAGS = $(COMMON_CFLAGS)
 bitmap_mod_LDFLAGS = $(COMMON_LDFLAGS)
 

=== added file 'include/grub/bitmap_scale.h'
--- include/grub/bitmap_scale.h	1970-01-01 00:00:00 +0000
+++ include/grub/bitmap_scale.h	2008-08-31 03:00:53 +0000
@@ -0,0 +1,54 @@
+/* bitmap_scale.h - Bitmap scaling functions. */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2006,2007  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/>.
+ */
+
+#ifndef GRUB_BITMAP_SCALE_HEADER
+#define GRUB_BITMAP_SCALE_HEADER 1
+
+#include <grub/err.h>
+#include <grub/types.h>
+#include <grub/bitmap_scale.h>
+
+enum grub_video_bitmap_scale_method
+{
+  /* Choose the fastest interpolation algorithm. */
+  GRUB_VIDEO_BITMAP_SCALE_METHOD_FASTEST,
+  /* Choose the highest quality interpolation algorithm. */
+  GRUB_VIDEO_BITMAP_SCALE_METHOD_BEST,
+  /* Nearest neighbor interpolation algorithm. */
+  GRUB_VIDEO_BITMAP_SCALE_METHOD_NEAREST,
+  /* Bilinear interpolation algorithm. */
+  GRUB_VIDEO_BITMAP_SCALE_METHOD_BILINEAR
+};
+
+grub_err_t
+grub_video_bitmap_scale_nn (struct grub_video_bitmap *dst,
+                            struct grub_video_bitmap *src);
+
+grub_err_t
+grub_video_bitmap_scale_bilinear (struct grub_video_bitmap *dst,
+                                  struct grub_video_bitmap *src);
+
+grub_err_t
+grub_video_bitmap_create_scaled (struct grub_video_bitmap **dst,
+                                 int dst_width, int dst_height,
+                                 struct grub_video_bitmap *src,
+                                 enum
+                                 grub_video_bitmap_scale_method scale_method);
+
+#endif /* ! GRUB_BITMAP_SCALE_HEADER */

=== modified file 'term/gfxterm.c'
--- term/gfxterm.c	2008-08-19 18:20:33 +0000
+++ term/gfxterm.c	2008-08-31 03:08:13 +0000
@@ -29,6 +29,7 @@
 #include <grub/env.h>
 #include <grub/video.h>
 #include <grub/bitmap.h>
+#include <grub/bitmap_scale.h>
 
 #define DEFAULT_VIDEO_WIDTH	640
 #define DEFAULT_VIDEO_HEIGHT	480
@@ -1010,8 +1011,18 @@
   dirty_region_redraw ();
 }
 
+
+/* Option array indices. */
+#define BACKGROUND_CMD_ARGINDEX_MODE 0
+
+static const struct grub_arg_option background_image_cmd_options[] = {
+  {"mode", 'm', 0, "Background image mode (`stretch', `normal').", 0, 
+    ARG_TYPE_STRING},
+  {0, 0, 0, 0, 0, 0}
+};
+
 static grub_err_t
-grub_gfxterm_background_image_cmd (struct grub_arg_list *state __attribute__ ((unused)),
+grub_gfxterm_background_image_cmd (struct grub_arg_list *state,
                                    int argc,
                                    char **args)
 {
@@ -1038,12 +1049,36 @@
     if (grub_errno != GRUB_ERR_NONE)
       return grub_errno;
 
+    /* Determine if the bitmap should be scaled to fit the screen. */
+    if (!state[BACKGROUND_CMD_ARGINDEX_MODE].set
+        || grub_strcmp (state[BACKGROUND_CMD_ARGINDEX_MODE].arg,
+                        "stretch") == 0)
+        {
+          if (mode_info.width != grub_video_bitmap_get_width (bitmap)
+              || mode_info.height != grub_video_bitmap_get_height (bitmap)) 
+            {
+              struct grub_video_bitmap *scaled_bitmap;
+              grub_video_bitmap_create_scaled (&scaled_bitmap,
+                                              mode_info.width, 
+                                              mode_info.height,
+                                              bitmap,
+                                              GRUB_VIDEO_BITMAP_SCALE_METHOD_BEST);
+              if (grub_errno == GRUB_ERR_NONE)
+                {
+                  /* Replace the original bitmap with the scaled one. */
+                  grub_video_bitmap_destroy (bitmap);
+                  bitmap = scaled_bitmap;
+                }
+            }
+        }
+
+
     /* If bitmap was loaded correctly, display it.  */
     if (bitmap)
       {
         /* Determine bitmap dimensions.  */
         bitmap_width = grub_video_bitmap_get_width (bitmap);
-        bitmap_height = grub_video_bitmap_get_width (bitmap);
+        bitmap_height = grub_video_bitmap_get_height (bitmap);
         
         /* Mark whole screen as dirty.  */
         dirty_region_reset ();
@@ -1088,7 +1123,7 @@
                          GRUB_COMMAND_FLAG_BOTH,
                          "background_image",
                          "Load background image for active terminal",
-                         0);
+                         background_image_cmd_options);
 }
 
 GRUB_MOD_FINI(term_gfxterm)

=== added file 'video/bitmap_scale.c'
--- video/bitmap_scale.c	1970-01-01 00:00:00 +0000
+++ video/bitmap_scale.c	2008-08-31 03:00:53 +0000
@@ -0,0 +1,101 @@
+/* bitmap_scale.c - Bitmap scaling interface. */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2006,2007  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/mm.h>
+#include <grub/misc.h>
+#include <grub/video.h>
+#include <grub/bitmap.h>
+#include <grub/bitmap_scale.h>
+#include <grub/types.h>
+
+/* 
+ * This function creates a new scaled version of the bitmap SRC.  The new
+ * bitmap has dimensions DST_WIDTH by DST_HEIGHT.  The scaling algorithm
+ * is given by SCALE_METHOD.  If an error is encountered, the return code is
+ * not equal to GRUB_ERR_NONE, and the bitmap DST is either not created, or 
+ * it is destroyed before this function returns.
+ *
+ * Supports only direct color modes which have components separated
+ * into bytes (e.g., RGBA 8:8:8:8 or BGR 8:8:8 true color). 
+ * But because of this simplifying assumption, the implementation is
+ * greatly simplified.
+ */
+grub_err_t
+grub_video_bitmap_create_scaled (struct grub_video_bitmap **dst,
+                                 int dst_width, int dst_height,
+                                 struct grub_video_bitmap *src,
+                                 enum grub_video_bitmap_scale_method
+                                 scale_method)
+{
+  *dst = 0;
+
+  /* Verify the simplifying assumptions. */
+  if (src == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                       "null src bitmap in grub_video_bitmap_create_scaled");
+  if (src->mode_info.red_field_pos % 8 != 0
+      || src->mode_info.green_field_pos % 8 != 0
+      || src->mode_info.blue_field_pos % 8 != 0
+      || src->mode_info.reserved_field_pos % 8 != 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                       "src format not supported for scale");
+  if (src->mode_info.width == 0 || src->mode_info.height == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "bitmap has a zero dimension");
+  if (dst_width <= 0 || dst_height <= 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                       "requested to scale to a size w/ a zero dimension");
+  if (src->mode_info.bytes_per_pixel * 8 != src->mode_info.bpp)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                       "bitmap to scale has inconsistent Bpp and bpp");
+
+  /* Create the new bitmap. */
+  grub_err_t ret;
+  ret = grub_video_bitmap_create (dst, dst_width, dst_height,
+                                  src->mode_info.blit_format);
+  if (ret != GRUB_ERR_NONE)
+    return ret;                 /* Error. */
+
+  switch (scale_method)
+    {
+    case GRUB_VIDEO_BITMAP_SCALE_METHOD_FASTEST:
+    case GRUB_VIDEO_BITMAP_SCALE_METHOD_NEAREST:
+      ret = grub_video_bitmap_scale_nn (*dst, src);
+      break;
+    case GRUB_VIDEO_BITMAP_SCALE_METHOD_BEST:
+    case GRUB_VIDEO_BITMAP_SCALE_METHOD_BILINEAR:
+      ret = grub_video_bitmap_scale_bilinear (*dst, src);
+      break;
+    default:
+      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Invalid scale_method value");
+      break;
+    }
+
+  if (ret == GRUB_ERR_NONE)
+    {
+      /* Success:  *dst is now a pointer to the scaled bitmap. */
+      return GRUB_ERR_NONE;
+    }
+  else
+    {
+      /* Destroy the bitmap and return the error code. */
+      grub_video_bitmap_destroy (*dst);
+      *dst = 0;
+      return ret;
+    }
+}

=== added file 'video/bitmap_scale_bilinear.c'
--- video/bitmap_scale_bilinear.c	1970-01-01 00:00:00 +0000
+++ video/bitmap_scale_bilinear.c	2008-08-31 03:00:53 +0000
@@ -0,0 +1,145 @@
+/* bitmap_scale_bilinear.c - Bilinear image scaling algorithm. */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2006,2007  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/mm.h>
+#include <grub/misc.h>
+#include <grub/video.h>
+#include <grub/bitmap.h>
+#include <grub/bitmap_scale.h>
+#include <grub/types.h>
+
+/* 
+ * Copy the bitmap SRC to the bitmap DST, scaling the bitmap to fit the 
+ * dimensions of DST.  This function uses the bilinear interpolation algorithm
+ * to interpolate the pixels.
+ *
+ * Supports only direct color modes which have components separated
+ * into bytes (e.g., RGBA 8:8:8:8 or BGR 8:8:8 true color). 
+ * But because of this simplifying assumption, the implementation is
+ * greatly simplified.
+ */
+grub_err_t
+grub_video_bitmap_scale_bilinear (struct grub_video_bitmap *dst,
+                                  struct grub_video_bitmap *src)
+{
+  /* Verify the simplifying assumptions. */
+  if (dst == 0 || src == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "null bitmap in scale func");
+  if (dst->mode_info.red_field_pos % 8 != 0
+      || dst->mode_info.green_field_pos % 8 != 0
+      || dst->mode_info.blue_field_pos % 8 != 0
+      || dst->mode_info.reserved_field_pos % 8 != 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "dst format not supported");
+  if (src->mode_info.red_field_pos % 8 != 0
+      || src->mode_info.green_field_pos % 8 != 0
+      || src->mode_info.blue_field_pos % 8 != 0
+      || src->mode_info.reserved_field_pos % 8 != 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "src format not supported");
+  if (dst->mode_info.red_field_pos != src->mode_info.red_field_pos
+      || dst->mode_info.red_mask_size != src->mode_info.red_mask_size
+      || dst->mode_info.green_field_pos != src->mode_info.green_field_pos
+      || dst->mode_info.green_mask_size != src->mode_info.green_mask_size
+      || dst->mode_info.blue_field_pos != src->mode_info.blue_field_pos
+      || dst->mode_info.blue_mask_size != src->mode_info.blue_mask_size
+      || dst->mode_info.reserved_field_pos !=
+      src->mode_info.reserved_field_pos
+      || dst->mode_info.reserved_mask_size !=
+      src->mode_info.reserved_mask_size)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "dst and src not compatible");
+  if (dst->mode_info.bytes_per_pixel != src->mode_info.bytes_per_pixel)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "dst and src not compatible");
+  if (dst->mode_info.width == 0 || dst->mode_info.height == 0
+      || src->mode_info.width == 0 || src->mode_info.height == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "bitmap has a zero dimension");
+
+  grub_uint8_t *ddata = dst->data;
+  grub_uint8_t *sdata = src->data;
+  int dw = dst->mode_info.width;
+  int dh = dst->mode_info.height;
+  int sw = src->mode_info.width;
+  int sh = src->mode_info.height;
+  int dstride = dst->mode_info.pitch;
+  int sstride = src->mode_info.pitch;
+  /* bytes_per_pixel is the same for both src and dst. */
+  int bytes_per_pixel = dst->mode_info.bytes_per_pixel;
+
+  int dy;
+  for (dy = 0; dy < dh; dy++)
+    {
+      int dx;
+      for (dx = 0; dx < dw; dx++)
+        {
+          grub_uint8_t *dptr;
+          grub_uint8_t *sptr;
+          int sx;
+          int sy;
+          int comp;
+
+          /* Compute the source coordinate that the destination coordinate
+           * maps to.  Note: sx/sw = dx/dw  =>  sx = sw*dx/dw. */
+          sx = sw * dx / dw;
+          sy = sh * dy / dh;
+
+          /* Get the address of the pixels in src and dst. */
+          dptr = ddata + dy * dstride + dx * bytes_per_pixel;
+          sptr = sdata + sy * sstride + sx * bytes_per_pixel;
+
+          /* If we have enough space to do so, use bilinear interpolation.
+           * Otherwise, fall back to nearest neighbor for this pixel. */
+          if (sx < sw - 1 && sy < sh - 1)
+            {
+              /* Do bilinear interpolation. */
+
+              /* Fixed-point .8 numbers representing the fraction of the
+               * distance in the x (u) and y (v) direction within the 
+               * box of 4 pixels in the source. */
+              int u = (256 * sw * dx / dw) - (sx * 256);
+              int v = (256 * sh * dy / dh) - (sy * 256);
+
+              for (comp = 0; comp < bytes_per_pixel; comp++)
+                {
+                  /* Get the component's values for the 
+                   * 4 source corner pixels. */
+                  grub_uint8_t f00 = sptr[comp];
+                  grub_uint8_t f10 = sptr[comp + bytes_per_pixel];
+                  grub_uint8_t f01 = sptr[comp + sstride];
+                  grub_uint8_t f11 = sptr[comp + sstride + bytes_per_pixel];
+
+                  /* Do linear interpolations along the top and bottom
+                   * rows of the box. */
+                  grub_uint8_t f0y = (256 - v) * f00 / 256 + v * f01 / 256;
+                  grub_uint8_t f1y = (256 - v) * f10 / 256 + v * f11 / 256;
+
+                  /* Interpolate vertically. */
+                  grub_uint8_t fxy = (256 - u) * f0y / 256 + u * f1y / 256;
+
+                  dptr[comp] = fxy;
+                }
+            }
+          else
+            {
+              /* Fall back to nearest neighbor interpolation. */
+              /* Copy the pixel color value. */
+              for (comp = 0; comp < bytes_per_pixel; comp++)
+                dptr[comp] = sptr[comp];
+            }
+        }
+    }
+  return GRUB_ERR_NONE;
+}

=== added file 'video/bitmap_scale_nn.c'
--- video/bitmap_scale_nn.c	1970-01-01 00:00:00 +0000
+++ video/bitmap_scale_nn.c	2008-08-31 03:00:53 +0000
@@ -0,0 +1,109 @@
+/* bitmap_scale_nn.c - Nearest neighbor image scaling algorithm. */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2006,2007  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/mm.h>
+#include <grub/misc.h>
+#include <grub/video.h>
+#include <grub/bitmap.h>
+#include <grub/bitmap_scale.h>
+#include <grub/types.h>
+
+/* 
+ * Copy the bitmap SRC to the bitmap DST, scaling the bitmap to fit the 
+ * dimensions of DST.  This function uses the nearest neighbor algorithm to
+ * interpolate the pixels.
+ *
+ * Supports only direct color modes which have components separated
+ * into bytes (e.g., RGBA 8:8:8:8 or BGR 8:8:8 true color). 
+ * But because of this simplifying assumption, the implementation is
+ * greatly simplified.
+ */
+grub_err_t
+grub_video_bitmap_scale_nn (struct grub_video_bitmap *dst,
+                            struct grub_video_bitmap *src)
+{
+  /* Verify the simplifying assumptions. */
+  if (dst == 0 || src == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "null bitmap in scale_nn");
+  if (dst->mode_info.red_field_pos % 8 != 0
+      || dst->mode_info.green_field_pos % 8 != 0
+      || dst->mode_info.blue_field_pos % 8 != 0
+      || dst->mode_info.reserved_field_pos % 8 != 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "dst format not supported");
+  if (src->mode_info.red_field_pos % 8 != 0
+      || src->mode_info.green_field_pos % 8 != 0
+      || src->mode_info.blue_field_pos % 8 != 0
+      || src->mode_info.reserved_field_pos % 8 != 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "src format not supported");
+  if (dst->mode_info.red_field_pos != src->mode_info.red_field_pos
+      || dst->mode_info.red_mask_size != src->mode_info.red_mask_size
+      || dst->mode_info.green_field_pos != src->mode_info.green_field_pos
+      || dst->mode_info.green_mask_size != src->mode_info.green_mask_size
+      || dst->mode_info.blue_field_pos != src->mode_info.blue_field_pos
+      || dst->mode_info.blue_mask_size != src->mode_info.blue_mask_size
+      || dst->mode_info.reserved_field_pos !=
+      src->mode_info.reserved_field_pos
+      || dst->mode_info.reserved_mask_size !=
+      src->mode_info.reserved_mask_size)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "dst and src not compatible");
+  if (dst->mode_info.bytes_per_pixel != src->mode_info.bytes_per_pixel)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "dst and src not compatible");
+  if (dst->mode_info.width == 0 || dst->mode_info.height == 0
+      || src->mode_info.width == 0 || src->mode_info.height == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "bitmap has a zero dimension");
+
+  grub_uint8_t *ddata = dst->data;
+  grub_uint8_t *sdata = src->data;
+  int dw = dst->mode_info.width;
+  int dh = dst->mode_info.height;
+  int sw = src->mode_info.width;
+  int sh = src->mode_info.height;
+  int dstride = dst->mode_info.pitch;
+  int sstride = src->mode_info.pitch;
+  /* bytes_per_pixel is the same for both src and dst. */
+  int bytes_per_pixel = dst->mode_info.bytes_per_pixel;
+
+  int dy;
+  for (dy = 0; dy < dh; dy++)
+    {
+      int dx;
+      for (dx = 0; dx < dw; dx++)
+        {
+          grub_uint8_t *dptr;
+          grub_uint8_t *sptr;
+          int sx;
+          int sy;
+          int comp;
+
+          /* Compute the source coordinate that the destination coordinate
+           * maps to.  Note: sx/sw = dx/dw  =>  sx = sw*dx/dw. */
+          sx = sw * dx / dw;
+          sy = sh * dy / dh;
+
+          /* Get the address of the pixels in src and dst. */
+          dptr = ddata + dy * dstride + dx * bytes_per_pixel;
+          sptr = sdata + sy * sstride + sx * bytes_per_pixel;
+
+          /* Copy the pixel color value. */
+          for (comp = 0; comp < bytes_per_pixel; comp++)
+            dptr[comp] = sptr[comp];
+        }
+    }
+  return GRUB_ERR_NONE;
+}


[-- Attachment #1.3: 09_ChangeLog.txt --]
[-- Type: text/plain, Size: 835 bytes --]

2008-08-30  Colin D Bennett <colin@gibibit.com>
	
	Image scaling support.  Also, gfxterm's background_image command can
	scale the image to fit the screen (--mode/-m option).

	* conf/i386-pc.rmk (bitmap_mod_SOURCES): Added new source files.

	* include/grub/bitmap_scale.h: New file.
	* video/bitmap_scale.c: New file.
	* video/bitmap_scale_bilinear.c: New file.
	* video/bitmap_scale_nn.c: New file.

	* term/gfxterm.c (BACKGROUND_CMD_ARGINDEX_MODE): New macro.
	(background_image_cmd_options): New static variable.
	(grub_gfxterm_background_image_cmd): Handle --mode/-m option and scale
	the bitmap to the screen size if requested.  Also includes a
	"drive-by" fix to an erroneous assignment of the bitmap width to
	'bitmap_height'.
	(GRUB_MOD_INIT): Pass background_image_cmd_options to
	grub_register_command.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] GSoC #09 Bitmap scaling

[Vesa Jääskeläinen]

Colin D Bennett wrote:
> This patch adds image scaling support.  Nearest neighbor and bilinear
> interpolation algorithms are supported.
> 
> The gfxterm background_image command scales the background image to fit
> the screen.  This can be controlled with the new --mode/-m option.

Copyrights years for new files seems wrong

It is really necessary that all generic graphical menu code like bitmap
scaler works always. Not just for some optimized formats as otherwise it
would render graphical menu unusable in some cases. It can be not so
pretty on low end systems, but it should at least work. That is the
reason there is map/unmap functions in video api.

So there has to be first generic implementation and then you can use
optimized one if there exist one.

Basically I have not bothered to go formats below 1 byte. But obviously
as there is now 1 bit format that needs to be supported too.

Perhaps there should be set/getpixel for bitmap or so. I do not want
those to appear in video drivers thou at least as external.

Re: [PATCH] GSoC #09 Bitmap scaling

[Vesa Jääskeläinen]

Vesa Jääskeläinen wrote:
> Colin D Bennett wrote:
>> This patch adds image scaling support.  Nearest neighbor and bilinear
>> interpolation algorithms are supported.
>>
>> The gfxterm background_image command scales the background image to fit
>> the screen.  This can be controlled with the new --mode/-m option.
> 
> Copyrights years for new files seems wrong
> 
> It is really necessary that all generic graphical menu code like bitmap
> scaler works always. Not just for some optimized formats as otherwise it
> would render graphical menu unusable in some cases. It can be not so
> pretty on low end systems, but it should at least work. That is the
> reason there is map/unmap functions in video api.
> 
> So there has to be first generic implementation and then you can use
> optimized one if there exist one.
> 
> Basically I have not bothered to go formats below 1 byte. But obviously
> as there is now 1 bit format that needs to be supported too.
> 
> Perhaps there should be set/getpixel for bitmap or so. I do not want
> those to appear in video drivers thou at least as external.

Oh one extra thing. I would like that only create_scaled is visible out
side as accessible function. So this would only provide one easy entry
point for such functionality.

I am just wondering do we have module globals that are not exported
outside...

Re: [PATCH] GSoC #09 Bitmap scaling

[Vesa Jääskeläinen]

Vesa Jääskeläinen wrote:
> It is really necessary that all generic graphical menu code like bitmap
> scaler works always. Not just for some optimized formats as otherwise it
> would render graphical menu unusable in some cases. It can be not so
> pretty on low end systems, but it should at least work. That is the
> reason there is map/unmap functions in video api.
> 
> So there has to be first generic implementation and then you can use
> optimized one if there exist one.

Just thinking as this is operating in a bitmap, we could just make sure
we use only couple of formats and let video driver dot the rest. That
would simplify it a bit.

Re: [PATCH] GSoC #09 Bitmap scaling

[Vesa Jääskeläinen]

Vesa Jääskeläinen wrote:
> Vesa Jääskeläinen wrote:
>> It is really necessary that all generic graphical menu code like bitmap
>> scaler works always. Not just for some optimized formats as otherwise it
>> would render graphical menu unusable in some cases. It can be not so
>> pretty on low end systems, but it should at least work. That is the
>> reason there is map/unmap functions in video api.
>>
>> So there has to be first generic implementation and then you can use
>> optimized one if there exist one.
> 
> Just thinking as this is operating in a bitmap, we could just make sure
> we use only couple of formats and let video driver dot the rest. That
> would simplify it a bit.

Hi,

Now that Colin is back in action I am reviving this thread :).

I have been thinking this a bit and I think best solution to bitmaps is
something like following.

Two types of bitmap: easily accessible and optimized bitmaps for hardware.

Easily accessible are meant to be modified by user and provides slow
blitting performance. Basically we should only support two formats.
RGB888 and ARGB8888 (order can be different). This way we can make easy
code to modify bitmaps.

When there is no need to modify bitmap anymore, one calls
grub_video_bitmap_optimize(bitmap, rendering_target) and then bitmap is
optimized to match reder_targets format.

there would be two new helpers that gives access to bitmap data.
grub_video_bitmap_lock() and to release it grub_video_bitmap_unlock().
Lock will fail if bitmap is optimized.

I am wondering should we have grub_video_bitmap_unoptimize() to map back
to editable mode. But that might be more pain and promote bad ways to do
conversion.

Now bitmap loaders would be modified to return data in easily accessible
format so bitmaps can be modified and so on.

Now to bitmap scaling. This can only be done for easily accessible
formats and could be something like this:

+grub_err_t
+grub_video_bitmap_create_scaled (struct grub_video_bitmap **dst,
+                                 int dst_width, int dst_height,
+                                 struct grub_video_bitmap *src,
+                                 enum
+                                 grub_video_bitmap_scale_method
scale_method);

Now in example static bitmaps would be optimized right away in order to
make their blitting fast. I do not know if we need special handling for
transparent bitmaps. May need some experimenting.

Actual scalers would be hidden from API and can only be specified by
enum type.

It could be a good idea to implement all scalers in same file. Unless
there is some weird scaler that needs thousands of lines of code.

Any opinions?

Thanks,
Vesa Jääskeläinen

Re: [PATCH] GSoC #09 Bitmap scaling

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 3819 bytes --]

On Fri, 03 Oct 2008 23:16:51 +0300
Vesa Jääskeläinen <chaac@nic.fi> wrote:

> Vesa Jääskeläinen wrote:
> > Vesa Jääskeläinen wrote:
> >> It is really necessary that all generic graphical menu code like
> >> bitmap scaler works always. Not just for some optimized formats as
> >> otherwise it would render graphical menu unusable in some cases.
> >> It can be not so pretty on low end systems, but it should at least
> >> work. That is the reason there is map/unmap functions in video api.
> >>
> >> So there has to be first generic implementation and then you can
> >> use optimized one if there exist one.
> > 
> > Just thinking as this is operating in a bitmap, we could just make
> > sure we use only couple of formats and let video driver dot the
> > rest. That would simplify it a bit.
> 
> Hi,
> 
> Now that Colin is back in action I am reviving this thread :).
> 
> I have been thinking this a bit and I think best solution to bitmaps
> is something like following.
> 
> Two types of bitmap: easily accessible and optimized bitmaps for
> hardware.
> 
> Easily accessible are meant to be modified by user and provides slow
> blitting performance. Basically we should only support two formats.
> RGB888 and ARGB8888 (order can be different). This way we can make
> easy code to modify bitmaps.
> 
> When there is no need to modify bitmap anymore, one calls
> grub_video_bitmap_optimize(bitmap, rendering_target) and then bitmap
> is optimized to match reder_targets format.
> 
> there would be two new helpers that gives access to bitmap data.
> grub_video_bitmap_lock() and to release it grub_video_bitmap_unlock().
> Lock will fail if bitmap is optimized.

What is the effect of lock/unlock on an unoptimized bitmap?  Is it like
a reference counting memory management scheme?

> I am wondering should we have grub_video_bitmap_unoptimize() to map
> back to editable mode. But that might be more pain and promote bad
> ways to do conversion.

If some code needed to modify an optimized bitmap, we could just have
that code keep the unoptimized version of the bitmap around for
modification, and just re-optimize the bitmap after modifying it.  Of
course it would only make sense to optimize bitmaps that will be
blitted more than once.

> Now bitmap loaders would be modified to return data in easily
> accessible format so bitmaps can be modified and so on.
> 
> Now to bitmap scaling. This can only be done for easily accessible
> formats and could be something like this:
> 
> +grub_err_t
> +grub_video_bitmap_create_scaled (struct grub_video_bitmap **dst,
> +                                 int dst_width, int dst_height,
> +                                 struct grub_video_bitmap *src,
> +                                 enum
> +                                 grub_video_bitmap_scale_method
> scale_method);
> 
> Now in example static bitmaps would be optimized right away in order
> to make their blitting fast. I do not know if we need special
> handling for transparent bitmaps. May need some experimenting.

I have been scaling bitmaps with alpha transparency and it works fine
with the scaling code in the patch.  It's used for the menu frame, the
selected item highlight, and entry icons, to name a few uses.

> Actual scalers would be hidden from API and can only be specified by
> enum type.

Ok; sounds good.

> It could be a good idea to implement all scalers in same file. Unless
> there is some weird scaler that needs thousands of lines of code.

I'm fine with that.  (Though in general my preference is for short
source files:  900+ line files become like a labyrinth of code, a
maze of twisty passages all alike.)

> Any opinions?
> 
> Thanks,
> Vesa Jääskeläinen

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] GSoC #09 Bitmap scaling

[Vesa Jääskeläinen]

Colin D Bennett wrote:
> On Fri, 03 Oct 2008 23:16:51 +0300
> Vesa Jääskeläinen <chaac@nic.fi> wrote:
>> I have been thinking this a bit and I think best solution to bitmaps
>> is something like following.
>>
>> Two types of bitmap: easily accessible and optimized bitmaps for
>> hardware.
>>
>> Easily accessible are meant to be modified by user and provides slow
>> blitting performance. Basically we should only support two formats.
>> RGB888 and ARGB8888 (order can be different). This way we can make
>> easy code to modify bitmaps.
>>
>> When there is no need to modify bitmap anymore, one calls
>> grub_video_bitmap_optimize(bitmap, rendering_target) and then bitmap
>> is optimized to match reder_targets format.
>>
>> there would be two new helpers that gives access to bitmap data.
>> grub_video_bitmap_lock() and to release it grub_video_bitmap_unlock().
>> Lock will fail if bitmap is optimized.
> 
> What is the effect of lock/unlock on an unoptimized bitmap?  Is it like
> a reference counting memory management scheme?

Idea is that if bitmap is still locked you cannot optimize it or you
cannot lock it again. And if bitmap is optimized you cannot get pointer
to modify it. Eg. Function call returns memory pointer.

>> I am wondering should we have grub_video_bitmap_unoptimize() to map
>> back to editable mode. But that might be more pain and promote bad
>> ways to do conversion.
> 
> If some code needed to modify an optimized bitmap, we could just have
> that code keep the unoptimized version of the bitmap around for
> modification, and just re-optimize the bitmap after modifying it.  Of
> course it would only make sense to optimize bitmaps that will be
> blitted more than once.

And if bitmap is animated, there is no good reason to optimize it either.

>> Now bitmap loaders would be modified to return data in easily
>> accessible format so bitmaps can be modified and so on.
>>
>> Now to bitmap scaling. This can only be done for easily accessible
>> formats and could be something like this:
>>
>> +grub_err_t
>> +grub_video_bitmap_create_scaled (struct grub_video_bitmap **dst,
>> +                                 int dst_width, int dst_height,
>> +                                 struct grub_video_bitmap *src,
>> +                                 enum
>> +                                 grub_video_bitmap_scale_method
>> scale_method);
>>
>> Now in example static bitmaps would be optimized right away in order
>> to make their blitting fast. I do not know if we need special
>> handling for transparent bitmaps. May need some experimenting.
> 
> I have been scaling bitmaps with alpha transparency and it works fine
> with the scaling code in the patch.  It's used for the menu frame, the
> selected item highlight, and entry icons, to name a few uses.

Well... For our use it should be sufficient. So lets use that.

>> Actual scalers would be hidden from API and can only be specified by
>> enum type.
> 
> Ok; sounds good.
> 
>> It could be a good idea to implement all scalers in same file. Unless
>> there is some weird scaler that needs thousands of lines of code.
> 
> I'm fine with that.  (Though in general my preference is for short
> source files:  900+ line files become like a labyrinth of code, a
> maze of twisty passages all alike.)

True. But sometimes it is good idea to keep it in same place. I didn't
look at the files but I think it should not be too long. In example you
can hide those other methods with static specifier.

Thanks,
Vesa Jääskeläinen

Re: [PATCH] GSoC #09 Bitmap scaling

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 4632 bytes --]

On Mon, 13 Oct 2008 21:27:46 +0300
Vesa Jääskeläinen <chaac@nic.fi> wrote:

> Colin D Bennett wrote:
> > On Fri, 03 Oct 2008 23:16:51 +0300
> > Vesa Jääskeläinen <chaac@nic.fi> wrote:
> >> I have been thinking this a bit and I think best solution to
> >> bitmaps is something like following.
> >>
> >> Two types of bitmap: easily accessible and optimized bitmaps for
> >> hardware.
> >>
> >> Easily accessible are meant to be modified by user and provides
> >> slow blitting performance. Basically we should only support two
> >> formats. RGB888 and ARGB8888 (order can be different). This way we
> >> can make easy code to modify bitmaps.
> >>
> >> When there is no need to modify bitmap anymore, one calls
> >> grub_video_bitmap_optimize(bitmap, rendering_target) and then
> >> bitmap is optimized to match reder_targets format.
> >>
> >> there would be two new helpers that gives access to bitmap data.
> >> grub_video_bitmap_lock() and to release it
> >> grub_video_bitmap_unlock(). Lock will fail if bitmap is optimized.
> > 
> > What is the effect of lock/unlock on an unoptimized bitmap?  Is it
> > like a reference counting memory management scheme?
> 
> Idea is that if bitmap is still locked you cannot optimize it or you
> cannot lock it again. And if bitmap is optimized you cannot get
> pointer to modify it. Eg. Function call returns memory pointer.

I thought perhaps the 'optimize' operation would simply return a _new_
(and completely independent from that point forward) bitmap equivalent
to the input bitmap but in the same format as the current video mode
uses.

Are you thinking that it would be best to have the
'optimize'/'unoptimize' operations actually just modify the bitmap in
place?  I guess this is nice from a memory conservation standpoint, but
in some cases it wouldn't work well (i.e., 24-bit to 32-bit formats).

> 
> >> I am wondering should we have grub_video_bitmap_unoptimize() to map
> >> back to editable mode. But that might be more pain and promote bad
> >> ways to do conversion.
> > 
> > If some code needed to modify an optimized bitmap, we could just
> > have that code keep the unoptimized version of the bitmap around for
> > modification, and just re-optimize the bitmap after modifying it.
> > Of course it would only make sense to optimize bitmaps that will be
> > blitted more than once.
> 
> And if bitmap is animated, there is no good reason to optimize it
> either.

Agreed -- an animated bitmap is just a specific instance of a single
use bitmap.

> >> Now bitmap loaders would be modified to return data in easily
> >> accessible format so bitmaps can be modified and so on.
> >>
> >> Now to bitmap scaling. This can only be done for easily accessible
> >> formats and could be something like this:
> >>
> >> +grub_err_t
> >> +grub_video_bitmap_create_scaled (struct grub_video_bitmap **dst,
> >> +                                 int dst_width, int dst_height,
> >> +                                 struct grub_video_bitmap *src,
> >> +                                 enum
> >> +                                 grub_video_bitmap_scale_method
> >> scale_method);
> >>
> >> Now in example static bitmaps would be optimized right away in
> >> order to make their blitting fast. I do not know if we need special
> >> handling for transparent bitmaps. May need some experimenting.
> > 
> > I have been scaling bitmaps with alpha transparency and it works
> > fine with the scaling code in the patch.  It's used for the menu
> > frame, the selected item highlight, and entry icons, to name a few
> > uses.
> 
> Well... For our use it should be sufficient. So lets use that.

Ok.

> >> Actual scalers would be hidden from API and can only be specified
> >> by enum type.
> > 
> > Ok; sounds good.
> > 
> >> It could be a good idea to implement all scalers in same file.
> >> Unless there is some weird scaler that needs thousands of lines of
> >> code.
> > 
> > I'm fine with that.  (Though in general my preference is for short
> > source files:  900+ line files become like a labyrinth of code, a
> > maze of twisty passages all alike.)
> 
> True. But sometimes it is good idea to keep it in same place. I didn't
> look at the files but I think it should not be too long. In example
> you can hide those other methods with static specifier.

Yes, it might be best here to simply put it all in one file--sounds
fine to me.  I agree that making things static is a benefit of putting
all related stuff together in a file, to improve encapsulation.

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] GSoC #09 Bitmap scaling

[Vesa Jääskeläinen]

Colin D Bennett wrote:
> On Mon, 13 Oct 2008 21:27:46 +0300
> Vesa Jääskeläinen <chaac@nic.fi> wrote:
>> Idea is that if bitmap is still locked you cannot optimize it or you
>> cannot lock it again. And if bitmap is optimized you cannot get
>> pointer to modify it. Eg. Function call returns memory pointer.
> 
> I thought perhaps the 'optimize' operation would simply return a _new_
> (and completely independent from that point forward) bitmap equivalent
> to the input bitmap but in the same format as the current video mode
> uses.

Problem with that is that it makes supporting code harder to use. With
only handful of supported formats it much easier to write support code
to modify bitmap. If you allow to use any format supported by video
subsystem it is nightmare to support them all.

So if we just support two formats. We only need to care about RGB and
RGBA formats, rather easy task. Can be modified by using simple loops or
indexing. When we know that there is no modifications to be done for
bitmap, we can just call optimize() and it will convert (edited) bitmap
to fast to blit format.

As we have same handle all the time for bitmap we can just continue to
use it as is. If we would make new instance of it, would either need to
delete previous one and replace its usage with new one. Of course use
case here affects.

> Are you thinking that it would be best to have the
> 'optimize'/'unoptimize' operations actually just modify the bitmap in
> place?  I guess this is nice from a memory conservation standpoint, but
> in some cases it wouldn't work well (i.e., 24-bit to 32-bit formats).

I do not think at this point how optimize() or unoptimize() will be
implemented. Just general idea. Whether it is in-place operation or
re-allocation for memory, is same to me. It just have to work :)

Thanks,
Vesa Jääskeläinen

Re: [PATCH] GSoC #09 Bitmap scaling

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 4084 bytes --]

On Tue, 14 Oct 2008 00:11:42 +0300
Vesa Jääskeläinen <chaac@nic.fi> wrote:

> Colin D Bennett wrote:
> > On Mon, 13 Oct 2008 21:27:46 +0300
> > Vesa Jääskeläinen <chaac@nic.fi> wrote:
> >> Idea is that if bitmap is still locked you cannot optimize it or
> >> you cannot lock it again. And if bitmap is optimized you cannot get
> >> pointer to modify it. Eg. Function call returns memory pointer.
> > 
> > I thought perhaps the 'optimize' operation would simply return a
> > _new_ (and completely independent from that point forward) bitmap
> > equivalent to the input bitmap but in the same format as the
> > current video mode uses.
> 
> Problem with that is that it makes supporting code harder to use. With
> only handful of supported formats it much easier to write support code
> to modify bitmap. If you allow to use any format supported by video
> subsystem it is nightmare to support them all.
> 
> So if we just support two formats. We only need to care about RGB and
> RGBA formats, rather easy task. Can be modified by using simple loops
> or indexing. When we know that there is no modifications to be done
> for bitmap, we can just call optimize() and it will convert (edited)
> bitmap to fast to blit format.

A minor point:  You mentioned "RGB" and "RGBA" format--do you mean "true
color" (either RGB[A] or BGR[A] layout) or do you mean literal RGB byte
order? If we are talking about picking a single component order to
standardize on, it should be BGR[A].  Every video adapter I have tested
on so far (nVidia 7600GT, VIA Unichrome, VMware, QEMU) has supported
BGRA (32 bpp) or BGR (24 bpp) but not RGBA or RGB.  Perhaps others have
found the contrary to be true; if so I would like to know.

> As we have same handle all the time for bitmap we can just continue to
> use it as is. If we would make new instance of it, would either need
> to delete previous one and replace its usage with new one. Of course
> use case here affects.

Ok.  That's fine.  I'm still a little confused about the purpose of
lock/unlock, however.  Is it basically a way of catching mistakes in
the code where we accidentally try to modify bitmap data when we don't
want to?  I guess what I'm asking is, do lock/unlock do anything more
than set a flag that is checked, as in: (pseudocode)


void *get_ptr(bitmap b)
{
  if (b.optimized) return error();
  return b.ptr; 
}
void optimize(bitmap b)
{
  if (b.locked) error();
  /* optimize b... */
}
void lock(bitmap b)
{
  if (b.locked) error();
  b.locked = 1; 
}
void unlock(bitmap b) { b.locked = 0; }


> > Are you thinking that it would be best to have the
> > 'optimize'/'unoptimize' operations actually just modify the bitmap
> > in place?  I guess this is nice from a memory conservation
> > standpoint, but in some cases it wouldn't work well (i.e., 24-bit
> > to 32-bit formats).
> 
> I do not think at this point how optimize() or unoptimize() will be
> implemented. Just general idea. Whether it is in-place operation or
> re-allocation for memory, is same to me. It just have to work :)

Ok.

Another idea:  What if the image-loading functions automatically
optimize()'d the bitmaps when loaded, since we don't normally expect to
modify loaded bitmaps before display.  Then most all the bitmaps in use
would automatically get the performance benefit with no change to all
the users of the code.  The only thing we do with loaded images is the
scale them and blit them.

The scaling algorithms can easily work on any 24 or 32 bit color mode
without knowing details of which components are which (the process is
the same regardless of the color component).  Thus optimized images
could still be scaled highly efficiently (without an
unoptimize->scale->optimize process).  For 15/16 bit or indexed color
modes, we would have to unopt->scale->opt, but I really think that no
one should be using those modes.  If your video memory is too limited
for 24 or 32 bit color, then just use the perfectly great text mode
menu.

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] GSoC #09 Bitmap scaling

[Vesa Jääskeläinen]

Colin D Bennett wrote:
> On Tue, 14 Oct 2008 00:11:42 +0300
> Vesa Jääskeläinen <chaac@nic.fi> wrote:
> A minor point:  You mentioned "RGB" and "RGBA" format--do you mean "true
> color" (either RGB[A] or BGR[A] layout) or do you mean literal RGB byte
> order? If we are talking about picking a single component order to
> standardize on, it should be BGR[A].  Every video adapter I have tested
> on so far (nVidia 7600GT, VIA Unichrome, VMware, QEMU) has supported
> BGRA (32 bpp) or BGR (24 bpp) but not RGBA or RGB.  Perhaps others have
> found the contrary to be true; if so I would like to know.

As I stated before order is not an issue. We can use BGR[A].

I am just used to speak about RGB :)

>> As we have same handle all the time for bitmap we can just continue to
>> use it as is. If we would make new instance of it, would either need
>> to delete previous one and replace its usage with new one. Of course
>> use case here affects.
> 
> Ok.  That's fine.  I'm still a little confused about the purpose of
> lock/unlock, however.  Is it basically a way of catching mistakes in
> the code where we accidentally try to modify bitmap data when we don't
> want to?  I guess what I'm asking is, do lock/unlock do anything more
> than set a flag that is checked, as in: (pseudocode)
> 
> 
> void *get_ptr(bitmap b)
> {
>   if (b.optimized) return error();
>   return b.ptr; 
> }
> void optimize(bitmap b)
> {
>   if (b.locked) error();
>   /* optimize b... */
> }
> void lock(bitmap b)
> {
>   if (b.locked) error();
>   b.locked = 1; 
> }
> void unlock(bitmap b) { b.locked = 0; }

No, more like:

void *lock(bitmap b)
{
  if (b.locked) return NULL;
  if (b.optimized) return NULL;

  b.locked = 1;

  return b.dataptr;
}

void unlock(bitmap b)
{
  b.locked = 0;
}

grub_err_t optimize(bitmap b, rendertarget r)
{
  if (b.locked) return error;
  if (b.optimized) return error;

  // do magic
  b.optimized = 1;

  return success;
}

Now one would use it like:

ptr = lock();
if (ptr)
{
  // modify it.
  ptr[0] = blue;
  ptr[1] = green;
  ptr[2] = red;
  if (has_alpha)
    ptr[3] = alpha;

  unlock();
}

>>> Are you thinking that it would be best to have the
>>> 'optimize'/'unoptimize' operations actually just modify the bitmap
>>> in place?  I guess this is nice from a memory conservation
>>> standpoint, but in some cases it wouldn't work well (i.e., 24-bit
>>> to 32-bit formats).
>> I do not think at this point how optimize() or unoptimize() will be
>> implemented. Just general idea. Whether it is in-place operation or
>> re-allocation for memory, is same to me. It just have to work :)
> 
> Ok.
> 
> Another idea:  What if the image-loading functions automatically
> optimize()'d the bitmaps when loaded, since we don't normally expect to
> modify loaded bitmaps before display.  Then most all the bitmaps in use
> would automatically get the performance benefit with no change to all
> the users of the code.  The only thing we do with loaded images is the
> scale them and blit them.

No. If user has low color mode optimize will really make image look
ugly. So best to postpone this conversion to far as possible.

> The scaling algorithms can easily work on any 24 or 32 bit color mode
> without knowing details of which components are which (the process is
> the same regardless of the color component).  Thus optimized images
> could still be scaled highly efficiently (without an
> unoptimize->scale->optimize process).  For 15/16 bit or indexed color
> modes, we would have to unopt->scale->opt, but I really think that no
> one should be using those modes.  If your video memory is too limited
> for 24 or 32 bit color, then just use the perfectly great text mode
> menu.

I would still like to support all RGB modes. Indexed color modes are
just backup option.

Re: [PATCH] GSoC #09 Bitmap scaling

[Colin D Bennett]

[-- Attachment #1: Type: text/plain, Size: 3839 bytes --]

On Wed, 15 Oct 2008 17:34:12 +0300
Vesa Jääskeläinen <chaac@nic.fi> wrote:

> Colin D Bennett wrote:
> > On Tue, 14 Oct 2008 00:11:42 +0300
> > Vesa Jääskeläinen <chaac@nic.fi> wrote:
> >> As we have same handle all the time for bitmap we can just
> >> continue to use it as is. If we would make new instance of it,
> >> would either need to delete previous one and replace its usage
> >> with new one. Of course use case here affects.
> > 
> > Ok.  That's fine.  I'm still a little confused about the purpose of
> > lock/unlock, however.  Is it basically a way of catching mistakes in
> > the code where we accidentally try to modify bitmap data when we
> > don't want to?  I guess what I'm asking is, do lock/unlock do
> > anything more than set a flag that is checked, as in: (pseudocode)
> > 
> > 
> > void *get_ptr(bitmap b)
> > {
> >   if (b.optimized) return error();
> >   return b.ptr; 
> > }
> > void optimize(bitmap b)
> > {
> >   if (b.locked) error();
> >   /* optimize b... */
> > }
> > void lock(bitmap b)
> > {
> >   if (b.locked) error();
> >   b.locked = 1; 
> > }
> > void unlock(bitmap b) { b.locked = 0; }
> 
> No, more like:
> 
> void *lock(bitmap b)
> {
>   if (b.locked) return NULL;
>   if (b.optimized) return NULL;
> 
>   b.locked = 1;
> 
>   return b.dataptr;
> }
> 
> void unlock(bitmap b)
> {
>   b.locked = 0;
> }
> 
> grub_err_t optimize(bitmap b, rendertarget r)
> {
>   if (b.locked) return error;
>   if (b.optimized) return error;
> 
>   // do magic
>   b.optimized = 1;
> 
>   return success;
> }
> 
> Now one would use it like:
> 
> ptr = lock();
> if (ptr)
> {
>   // modify it.
>   ptr[0] = blue;
>   ptr[1] = green;
>   ptr[2] = red;
>   if (has_alpha)
>     ptr[3] = alpha;
> 
>   unlock();
> }

Any more thoughts on this?  I can try to implement the bitmap
optimization when I have a chance, but I wondered if you had any more
insights into how you think it should work before I got to the effort
of implementing it.  Perhaps if we were to modify some of the "user"
code to use bitmap optimization, we could see how the proposed API
works out, and whether there are any things we could do better to make
it easier for the users of the bitmap API, since there are many users
and only one bitmap API that has to do the dirty work.  It would be
nice if the API made simple, common tasks are made easy for users.

Do you think we should return a uint8_t* from the lock() function?  The
other option is to return a basic structure to the caller that provides
the necessary information to manipulate the data in the 'unoptimized'
format (perhaps we could call it the 'intermediate representation' or
common format to avoid the cumbersome 'unoptimized' term).

User code that uses the "accessible" or "portable" standard bitmap
format for manipulation does not need most of the stuff in the bitmap's
mode_info struct, so it might simplify the task of user code if we had
a structure with just the data relevant to manipulation of these
accessible bitmaps.

For instance:

struct common_bitmap    // An RGB/RGBA bitmap in the standard format.
{
  uint8_t *data;    // Pixel data in BGR[A] order
  int width;        // Width in pixels
  int height;       // Height in pixels (number of rows in DATA)
  int stride;       // Number of bytes per row in DATA
  bool has_alpha;   // Alpha channel? 0 => BGR, 1 => BGRA
};

Then user code could do something like this:

struct bitmap *
prepare_frob_bitmap (render_target target)
{
  struct bitmap *bitmap;
  struct common_bitmap work;

  create_bitmap (&bitmap, 640, 480, NO_ALPHA);
  bitmap_lock (bitmap, &work);
  do_something_to_bitmap (&work);
  bitmap_unlock (bitmap);
  bitmap_optimize (bitmap, target);
}

Regards,
Colin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]