Support and General Use > User Interface and Voice

view Arabic text (cont.) - About the diacritics patch

<< < (2/2)

tomers:

--- Quote from: amr on March 27, 2010, 10:29:54 AM ---I couldn't find a #define stating the max generic glyph bitmap size, while it's not recommended to dynamically allocate memory with a variable runtime value of pf->maxwidth * pf->height / 8

--- End quote ---

Hi Amr,

I would like to review your code a bit. I hope you consider my remarks :)

You have calculated the value of 400 according to the equation stated above. Please verify if it shouldn't be like the formula used to calculate bitmap_size in font.c:cache_create(), which adds 7 to the font height. If so, consider if it is better for these functions to use some common helper function (or macro) which calculates buffer size given the font dimensions to calculates this, since that calculation is used in many places in font.c, e.g. load_cache_entry.

Just a thought: I wonder of it is better to have the statically allocated buffer in font.c, and use a it from the lcd-bitmap driver. I guess not, since the font files should be independent on the lcd driver implementation.

Few other comments:
* As suggested in FlySpray, please use a #define for the bitmap size, instead of hard-coding the value 400 in several places.
* Use memset to initialize bits_tmp
* Consider renaming bits_tmp to something better
* You can use bits[k] instead of *bits++ to avoid mixing of two different conventions,
    and use the |= operator:


--- Code: ---    < bits_tmp[k] = bits_tmp[k] | *bits++;
    > bits_tmp[k] |= bits[k];
--- End code ---

* I think that LCDFN(mono_bitmap_part) doesn't necessarily uses the entire buffer, therefore you don't need to actually iterate over the whole bits_tmp buffer (400 times). Try to find how to get the size mono_bitmap_part will use, and set bits_tmp accordingly, saving some redundant iterations.

amr:
Hi Tomer,
Thanks for the review and of course I'm glad to follow the remarks

Please have a look on the update http://www.rockbox.org/tracker/task/11095

N.B.
400 was just a rough value that didn't have any indication, after that I got the equation from kugel (without considering the height + 7 in font.c)
My calculated working value is 16 but I set it in the updated patch to 32 conservatively.

tomers:
Hi Amr, I'll look at your changes in the near future.

Meanwhile, please review the 'RTL with the text editor? possible?' forum thread. There's some diacritic support needed in the viewer plugin. It would be great if you can help with this too.

Thanks

Navigation

[0] Message Index

[*] Previous page