2010/2/11 Vladimir 'φ-coder/phcoder' Serbinenko <phco...@gmail.com>:
> 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
