Re: New patch for video subsystem...

[Johan Rydberg]

Vesa Jääskeläinen <address@hidden> writes:

> Here is the newest modifications to video subsystem.

Hi Vesa.  What a suitable name :)

Sorry for the delay, and lack of earlier comments, but I've been out
of the GRUB loop for a while.  But I have a few comments on the video
subsystem;

Correct me if I am wrong here, but what you call "render target" seems
to be what other video systems call "surface" or "drawable".  

Why use the concept of a "active" render target?  Why not instead let
all functions that operate on the active render target take a pointer
to a specific render target?  Poking through your patch, it seems that
there a lot of the following (forgive my pseudo-code)

   grub_video_set_active_render_target (target);
   // .. fill it with something ..
   grub_video_fill_rect (color, 0, 0, width, height);

   grub_video_set_active_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY);
   grub_video_blit_render_target (target, 0, 0, 0, 0, width, height);
   
I would feel more comfortable with the following workflow:

   grub_video_fill_rect (target, color, 0, 0, width, height);
   grub_video_blit_render_target (GRUB_VIDEO_RENDER_TARGET_DISPLAY,
                                  target, 0, 0, 0, 0, width, height);

Also, I think it is important the user can get hold of a pointer to
the render targets data, and an exact pixel format, to do private
rendering.  It would be hard to make a perfect gradient using fill_rect.

I'm not sure the concept of "viewports" are needed at all; instead let
the 'application' (e.g, the terminal) render into a render target, and
blit that to the screen at the desired position.  To minimize memory,
the videport-render target can be a sub-render target of the main
render target (ie, they share the same buffer) unless the user wants
any fancy stuff like a background picture.

Thanks,
Johan