Re: [PATCH] GSoC #09 Bitmap scaling

[Colin D Bennett <colin@gibibit.com>]

[-- 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 --]