Re: [RFC] Framebuffer rotation patch

[Michal Suchanek]

2010/2/11 Vladimir 'φ-coder/phcoder' Serbinenko <address@hidden>:
> Michal Suchanek wrote:
>> Hello
>>
>> Sending a preliminary framebuffer rotation patch.
>>
>> You can use videotest to see 4 tiles rotated from the same bitmap data.
>>
>>
> +char leaf_data[] = { 0x00, 0x0f, 0xe0, 0x00,
> +                     0x00, 0x7f, 0xfc, 0x00,
> +                     0x01, 0xff, 0xff, 0x00,
> +                     0x03, 0xff, 0xff, 0x80,
> This is a blob. Could it be generated automatically at build time?

How less of a blob it would be if it was included as a bitmap?

This is just a shape used for the video tests.

There are some X tools for working with bitmaps/pixmaps which could
generate this from a viewable file but they are usually not available
on Windows and other non-X platforms AFAIK.

> +
> +  sans = grub_font_get ("Helvetica Bold 14");
> Please use free font rather than Helvetica

I am pretty sure I did not choose the fonts, they must have been there
before I started with modifications to the tests.

> Could you split addition of videotests from the rest of the patch?
> -    unsigned int x;
> -    unsigned int y;
> -    unsigned int width;
> -    unsigned int height;
> +    int x;
> +    int y;
> +    int width;
> +    int height;
> Why do you need negative values?

I don't need the negative values everywhere but this change was to
align the typing so that casts are not required.

> +/* Supported operations are simple and easy to understand.
> + * MIRROR  |  swap image across (around) the vertical axis
> + * FLIP    -  swap image across the horizontal axis - upside down
> + * SWAP    /  swap image across the x=y axis - swap the x and y coordinates
> It's just a D_8 group. Could you add a comment to functions what they do
> in group theoretical sense? It would make the code easier to follow and

Obviously it is a group, any set of transforms closed on composition is.

And according to Wikipedia it's actually called D4.

> compute transformations for mathematicians. Perhaps another
> representation of D_8 would result in more efficient code?

If you have clearer explanation or more efficient code please share.

> +static inline void grub_swap_int(int *a, int *b) { int tmp = *a; *a =
> *b; *b = tmp; }
> +static inline void grub_swap_unsigned(unsigned *a, unsigned *b) {
> unsigned tmp = *a; *a = *b; *b = tmp; }
> +
> With typeof macro this can be made type-neutral avoiding potential mistakes.

OK, I will look at typeof.

> +static inline long
> +grub_min (long x, long y)
> +{
> +  if (x > y)
> +    return y;
> +  else
> +    return x;
> +}
> +
> Same here
> +
> +  int transform;
> I would prefer an enum

This is a bit field. How does it transform into an enum?

> Skipping term/gfxterm.c since it has to be adjusted for gfxmenu-related
> gfxterm.c updates.
> -         set_pixel (dst, x + i, y + j, dst_color);
> -       }
> +          if (transform & FB_TRAN_SWAP)
> +            set_pixel (dst, x + j*dy, y + i*dx, dst_color);
> +          else
> +            set_pixel (dst, x + i*dx, y + j*dy, dst_color);
> +        }
> Could you split it into 4 functions? I understand it's the slow fallback
> function but still it should be kept as fast as possible. Perhaps you
> could consider compiling same file with different #define's to obtain
> function variants

I would rather get a patch with minimal changes first so that it's
obvious what it does.

I can look into separating this later.

>  grub_err_t
> -grub_video_fb_set_viewport (unsigned int x, unsigned int y,
> -                           unsigned int width, unsigned int height)
> +grub_video_fb_set_viewport (int x, int y, int width, int height)
>  {
> You don't check for x < 0 and y < 0

Indeed, should check for those.

>> Known issues:
>>
>> - font glyphs and terminal do not rotate properly

This should be working already.

Thanks

Michal