Re: [RFC] Framebuffer rotation patch

[Vladimir 'φ-coder/phcoder' Serbinenko]

Michal Suchanek wrote:
> 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?
>
>   
It's not easily editable in current form.
> 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.
>
>   
Using XBM is fine (it's easily editable in either gimp or emacs) and you
should be able to
#include "leaf1.xbm"
>> +
>> +  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.
>
>   
Ok, it has to be fixed in mainstream then.
>> 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.
>
>   
Perhaps few casts together with appropiate checks when casting is better
than potentially exposing the whole code to the values it's not intended
for.
>> +/* 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.
>
>   
Not true. Consider an example of empty set: it has no identity element.
Less trivial counter-example: set of transformations: (x,y)->(kx,ky),
0<k<1. It's non-empty but has no identity element. If you replace < 1
with <=1 you will have an identity element but no other element is
invertible.

> And according to Wikipedia it's actually called D4.
>
>   
There are 2 conventions for naming dihedral groups:
1) Geometrical: since dihedral group is a symetry group of a regular
n-gone it's called D_n
2) Algebraical: since dihedral group has 2n elements it's called D_{2n}
Sometimes these conventions bear to confusion.
>> 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.
>
>   
I was thinking of using 2 generators instead of 3:
1) standard generators: s=rot90 or rot270, t=any reflection. Then all
elements are s^k or s^k*t. It makes computation of composition easier.
Rules on generators are:
s^n=t^2=1
tst=s^{-1}
2) or using 2 reflections. E.g. u=| and v=/. You already figured how to
generate with u=|, v=/, w=-
But w=uvuv
Then rules of computation are:
u^2=v^2=(uv)^4=1
>> +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?
>   
enum my_bitfield
{
  MY_BITFIELD_NONE=0,
  MY_BITFIELD_BIT0 = 1 << 0,
  MY_BITFIELD_BIT1 = 1 << 1,
  MY_BITFIELD_BIT2 = 1 << 2
};

> 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.
>
>   
ok

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

signature.asc
Description: OpenPGP digital signature