Hi all,
The only long term solution to the issue of the skin buffer size is to
completly redo how the skin engine uses its buffer, to that end I have
just started the rather large task of replacing every pointer in it to
an offset into the (currently) shared buffer which is then turned back
into a pointer when it is being used.
One annoyance with the current skin engine code is that all skins are
linked together with the single buffer, this means that if any skins
need reloading (user request to change theme perhaps) all of them need
to be reloaded. So one of the goals with this new work is to split
that up so each skin manages its own buflib handle, allowing skins to
be un/loaded on demand (there is no point loading the fm skin and
wasting buffer space if the user never actualy loads the fm screen for
example.)
The plan is to load the skin into the plugin buffer (and buflib
handles for images as done now), then do a memcpy() of the loaded skin
into a new buflib handle of the exact size needed for the skin. This
means people wanting boring themes don't limit people wanting Over The
Top themes (and no extra ram is wasted). Using offsets to do this
means there is also no need to manage pointers when the handle moves
around.
Another ultimate goal is to draw more of the UI using the skin engine,
specifically screens which are currently hard coded for different
screen sizes (the time&date screen is an obvious candidate), this is
very far down the track though so we'll see if that ever actually
happens.
The attached patch is the very start of this work. I'm really writing
this email because before I get too far in I want to make sure the
macros are going to be acceptable.
Three macros and a typedef have been added.
SKINOFFSETTOPTR() and PTRTOSKINOFFSET() convert between the offset and
the real pointer. I originally wanted to put the type in the first
macro and use that instead of void* but not sure if that is really
needed.
the 3rd macro OFFSETTYPE() is added because I realised the structs
used by the various tags will become very confusing if all the members
are skinoffset (or long) types instead of the pointer type which the
data actually contains. So I added this macro to hopefully help
document the code a bit.
Are these going to cause people to grimace? Is there a better way to
do what they are doing?
This is going to be a rather massive diff so if these macros are
unacceptable I really want to know now rather than once it is all
done.
Cheers
Jonathan
P.S This is on my skinengine_to_offsets github branch
https://github.com/jdgordon/rockbox/tree/skinengine_to_offsets and
will likely be pushed upstream as a single patch when it is done, but
would anyone like to help out? :D
P.P.S If you have a patch that touches apps/gui/skin_engine/* please
talk to me on IRC before committing so we dont cause extra work for
eachother.
diff --git a/apps/gui/skin_engine/skin_display.c b/apps/gui/skin_engine/skin_display.c
index 95e4310..a352252 100644
--- a/apps/gui/skin_engine/skin_display.c
+++ b/apps/gui/skin_engine/skin_display.c
@@ -406,7 +406,7 @@ void wps_display_images(struct gui_wps *gwps, struct viewport* vp)
{
wps_draw_image(gwps, img, img->display);
}
- else if (img->always_display && img->vp == vp)
+ else if (img->always_display && SKINOFFSETTOPTR(skin_buffer, img->vp) == vp)
{
wps_draw_image(gwps, img, 0);
}
diff --git a/apps/gui/skin_engine/skin_engine.c b/apps/gui/skin_engine/skin_engine.c
index bd875fe..8a6152f 100644
--- a/apps/gui/skin_engine/skin_engine.c
+++ b/apps/gui/skin_engine/skin_engine.c
@@ -48,7 +48,7 @@ void theme_init_buffer(void)
skins_initialising = false;
}
#else
-static char skin_buffer[SKIN_BUFFER_SIZE];
+char skin_buffer[SKIN_BUFFER_SIZE];
void theme_init_buffer(void)
{
skins_initialising = false;
diff --git a/apps/gui/skin_engine/skin_parser.c b/apps/gui/skin_engine/skin_parser.c
index 1557783..bf2179c 100644
--- a/apps/gui/skin_engine/skin_parser.c
+++ b/apps/gui/skin_engine/skin_parser.c
@@ -168,7 +168,7 @@ void *skin_find_item(const char *label, enum skin_find_what what,
#ifdef HAVE_LCD_BITMAP
case SKIN_FIND_IMAGE:
ret = list.linkedlist->token->value.data;
- itemlabel = ((struct gui_img *)ret)->label;
+ itemlabel = SKINOFFSETTOPTR(skin_buffer, ((struct gui_img *)ret)->label);
break;
#endif
#ifdef HAVE_TOUCHSCREEN
@@ -295,9 +295,9 @@ static int parse_image_display(struct skin_element *element,
{
return WPS_ERROR_INVALID_PARAM;
}
- id->label = label;
+ id->label = PTRTOSKINOFFSET(skin_buffer, label);
id->offset = 0;
- id->token = NULL;
+ id->token = PTRTOSKINOFFSET(skin_buffer, NULL);
if (img->using_preloaded_icons)
{
token->type = SKIN_TOKEN_IMAGE_DISPLAY_LISTICON;
@@ -306,7 +306,7 @@ static int parse_image_display(struct skin_element *element,
if (element->params_count > 1)
{
if (element->params[1].type == CODE)
- id->token = element->params[1].data.code->data;
+ id->token = PTRTOSKINOFFSET(skin_buffer, element->params[1].data.code->data);
/* specify a number. 1 being the first subimage (i.e top) NOT 0 */
else if (element->params[1].type == INTEGER)
id->subimage = element->params[1].data.number - 1;
@@ -358,7 +358,7 @@ static int parse_image_load(struct skin_element *element,
return WPS_ERROR_INVALID_PARAM;
/* save a pointer to the filename */
img->bm.data = (char*)filename;
- img->label = id;
+ img->label = PTRTOSKINOFFSET(skin_buffer, id);
img->x = x;
img->y = y;
img->num_subimages = 1;
@@ -368,7 +368,7 @@ static int parse_image_load(struct skin_element *element,
img->buflib_handle = -1;
/* save current viewport */
- img->vp = &curr_vp->vp;
+ img->vp = PTRTOSKINOFFSET(skin_buffer, &curr_vp->vp);
if (token->type == SKIN_TOKEN_IMAGE_DISPLAY)
{
@@ -924,7 +924,7 @@ static int parse_progressbar_tag(struct skin_element* element,
return WPS_ERROR_INVALID_PARAM;
/* save a pointer to the filename */
img->bm.data = (char*)image_filename;
- img->label = image_filename;
+ img->label = PTRTOSKINOFFSET(skin_buffer, image_filename);
img->x = 0;
img->y = 0;
img->num_subimages = 1;
@@ -932,7 +932,7 @@ static int parse_progressbar_tag(struct skin_element* element,
img->display = -1;
img->using_preloaded_icons = false;
img->buflib_handle = -1;
- img->vp = &curr_vp->vp;
+ img->vp = PTRTOSKINOFFSET(skin_buffer, &curr_vp->vp);
struct skin_token_list *item =
(struct skin_token_list *)new_skin_token_list_item(NULL, img);
if (!item)
diff --git a/apps/gui/skin_engine/skin_render.c b/apps/gui/skin_engine/skin_render.c
index 369bd46..3213470 100644
--- a/apps/gui/skin_engine/skin_render.c
+++ b/apps/gui/skin_engine/skin_render.c
@@ -184,11 +184,11 @@ static bool do_non_text_tags(struct gui_wps *gwps, struct skin_draw_info *info,
case SKIN_TOKEN_IMAGE_PRELOAD_DISPLAY:
{
struct image_display *id = token->value.data;
- const char* label = id->label;
+ const char* label = SKINOFFSETTOPTR(skin_buffer, id->label);
struct gui_img *img = skin_find_item(label,SKIN_FIND_IMAGE, data);
if (img && img->loaded)
{
- if (id->token == NULL)
+ if (SKINOFFSETTOPTR(skin_buffer, id->token) == NULL)
{
img->display = id->subimage;
}
@@ -197,8 +197,8 @@ static bool do_non_text_tags(struct gui_wps *gwps, struct skin_draw_info *info,
char buf[16];
const char *out;
int a = img->num_subimages;
- out = get_token_value(gwps, id->token, info->offset,
- buf, sizeof(buf), &a);
+ out = get_token_value(gwps, SKINOFFSETTOPTR(skin_buffer, id->token),
+ info->offset, buf, sizeof(buf), &a);
/* NOTE: get_token_value() returns values starting at 1! */
if (a == -1)
@@ -331,7 +331,7 @@ static void do_tags_in_hidden_conditional(struct skin_element* branch,
if (token->type == SKIN_TOKEN_IMAGE_PRELOAD_DISPLAY)
{
struct image_display *id = token->value.data;
- struct gui_img *img = skin_find_item(id->label,
+ struct gui_img *img = skin_find_item(SKINOFFSETTOPTR(skin_buffer, id->label),
SKIN_FIND_IMAGE, data);
clear_image_pos(gwps, img);
}
diff --git a/apps/gui/skin_engine/wps_internals.h b/apps/gui/skin_engine/wps_internals.h
index ed09ad0..7c61a4e 100644
--- a/apps/gui/skin_engine/wps_internals.h
+++ b/apps/gui/skin_engine/wps_internals.h
@@ -25,6 +25,16 @@
#ifndef _WPS_ENGINE_INTERNALS_
#define _WPS_ENGINE_INTERNALS_
+/* Use this type and macro to convert a pointer from the
+ * skin buffer to a useable pointer */
+typedef long skinoffset;
+#define SKINOFFSETTOPTR(base, offset) ((offset) < 0 ? NULL : ((void*)&base[offset]))
+#define PTRTOSKINOFFSET(base, pointer) ((pointer) ? ((void*)pointer-(void*)base) : -1)
+/* Use this macro when declaring a variable to self-document the code.
+ * type is the actual type being pointed to (i.e OFFSETTYPE(char*) foo )
+ */
+#define OFFSETTYPE(type) skinoffset
+extern char skin_buffer[];
/* Timeout unit expressed in HZ. In WPS, all timeouts are given in seconds
(possibly with a decimal fraction) but stored as integer values.
@@ -71,14 +81,14 @@
#ifdef HAVE_LCD_BITMAP
struct gui_img {
- struct viewport* vp; /* The viewport to display this image in */
+ OFFSETTYPE(struct viewport*) vp; /* The viewport to display this image in */
short int x; /* x-pos */
short int y; /* y-pos */
short int num_subimages; /* number of sub-images */
short int subimage_height; /* height of each sub-image */
struct bitmap bm;
int buflib_handle;
- const char *label;
+ OFFSETTYPE(char*) label;
bool loaded; /* load state */
bool always_display; /* not using the preload/display mechanism */
int display;
@@ -86,9 +96,9 @@ struct gui_img {
};
struct image_display {
- const char *label;
+ OFFSETTYPE(char*) label;
int subimage;
- struct wps_token *token; /* the token to get the subimage number from */
+ OFFSETTYPE(struct wps_token*) token; /* the token to get the subimage number from */
int offset; /* offset into the bitmap strip to start */
};
