Re: [PATCH] GSoC #09 Bitmap scaling

[Colin D Bennett]

On Mon, 13 Oct 2008 21:27:46 +0300
Vesa Jääskeläinen <address@hidden> wrote:

> Colin D Bennett wrote:
> > On Fri, 03 Oct 2008 23:16:51 +0300
> > Vesa Jääskeläinen <address@hidden> 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

signature.asc
Description: PGP signature