Re: [RFC] Framebuffer rotation patch

[Vladimir 'φ-coder/phcoder' Serbinenko]

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?
+
+  sans = grub_font_get ("Helvetica Bold 14");
Please use free font rather than Helvetica
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?
+/* 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
compute transformations for mathematicians. Perhaps another
representation of D_8 would result in more efficient code?
+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.
+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
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
 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
> Known issues:
>
> - font glyphs and terminal do not rotate properly
> - no accelerated blitters for rotated modes, at least the 1bit blitter
> should work
>
>   
> To be done
>
> - split out signed rectangle patch that changes graphics coordinates
> from unsigned to signed
> - make up some naming scheme and rename functions and macros accordingly
> - make up some acceptable way to specify framebnuffer rotation in the
> environment like gfxmode
>
> Thanks
>
> Michal
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko

signature.asc
Description: OpenPGP digital signature