Re: [PATCH] GSoC #09 Bitmap scaling

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

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.