Rockbox.org home
release
dev builds
extras
themes manual
wiki
device status forums
mailing lists
IRC bugs
patches
dev guide



Rockbox mail archive

Subject: jdgordon: r30599 - in trunk/apps: . gui/skin_engine

jdgordon: r30599 - in trunk/apps: . gui/skin_engine

From: Thomas Martitz <kugel_at_rockbox.org>
Date: Sun, 25 Sep 2011 16:54:36 +0200

Hello,

I want to discuss that commit as I disagree with it in some ways,
perhaps even see it reverted.

Clearly it introduced a user configurable skin buffer. That is something
we discussed many times before with no clear outcome. In fact, in my
memory there was a general opposition. I feel it silently introduced a
NoDo, although the wiki page doesn't list it.
So I'm bitter that this is introduced via a drive-by commit with no
mention about it at all. This is not the policy I want to see for
controversial changes.

It is especially strange that it's introduced now as the vast majority
of the skin buffer is truly dynamic through buflib (images + backdrops +
fonts). The skin buffer was a multi-100K thing before buflib; now the
configuration is made for a fraction of this. We didn't (want to) have
this configuration when it was still multiple 100K big.

Furthermore this user setting is implemented in a totally obscure way (a
magic file containing a magic number, which isn't checked if it's even
plausible). It's pattern that doesn't exist yet, while there's a clear
existing implementation of user settings. It's rather hidden without a
chance to be discovered by the user. I don't think I need to mention
that the manual doesn't tell anything about this.

Lastly, I complain (also not very strongly) about how it exploits
buflib. It makes an unmovable buffer, which should simply not exist,
because they throw away with the entire point of buflib. The exploit
goes further by forcing it to be the very first allocation. This is a)
introducing assumption, and b) breaks when something else needs to be
unmovable.

I understand it's much work to make the skin buffer movable, and it
might not happen at all. But this is much much less of a problem than
before. And we never wanted a configurable skin buffer. I think the
commit deserves _at least_ some discussion.

Best regards.

> Date: 2011-09-25 14:55:40 +0200 (Sun, 25 Sep 2011)
> New Revision: 30599
>
> Log Message:
> Check for the magic file "/.rockbox/skin_buffer_size.txt" on bootup which can have a number which is the
> amount of kilobytes to allocate for the skin buffer. This is only checked on boot so if you need to change it
> you must reboot to enable.
> Currently the default size is 80KB on colour targets which can be way too much or not enough for users.
> The format of the /.rockbox/skin_buffer_size.txt file is simply a number (so 120 if you want 120
> kilobytes), NO trainling spaces or text of any kind
>
> Modified:
> trunk/apps/gui/skin_engine/skin_engine.c
> trunk/apps/main.c
>
> Modified: trunk/apps/gui/skin_engine/skin_engine.c
> ===================================================================
> --- trunk/apps/gui/skin_engine/skin_engine.c 2011-09-25 12:19:33 UTC (rev 30598)
> +++ trunk/apps/gui/skin_engine/skin_engine.c 2011-09-25 12:55:40 UTC (rev 30599)
> @@ -25,6 +25,7 @@
> #include<limits.h>
> #include "inttypes.h"
> #include "config.h"
> +#include "core_alloc.h"
> #include "action.h"
> #include "crc32.h"
> #include "settings.h"
> @@ -48,9 +49,31 @@
> skins_initialising = false;
> }
> #else
> -static char skin_buffer[SKIN_BUFFER_SIZE];
> +static size_t skin_buffer_size;
> +static char *skin_buffer = NULL;
> +static int buflib_move_callback(int handle, void* current, void* new)
> +{
> + (void)current;
> + (void)new;
> + return BUFLIB_CB_CANNOT_MOVE;
> +}
> +static struct buflib_callbacks buflib_ops = {buflib_move_callback, NULL};
> +
> void theme_init_buffer(void)
> {
> + int fd;
> + size_t size = SKIN_BUFFER_SIZE;
> + fd = open_utf8(ROCKBOX_DIR "/skin_buffer_size.txt", O_RDONLY);
> + if (fd>= 0)
> + {
> + char buf[32];
> + read(fd, buf, sizeof(buf));
> + if (buf[0]>= '0'&& buf[0]<= '9')
> + size = atoi(buf)*1024;
> + close(fd);
> + }
> + skin_buffer = core_get_data(core_alloc_ex("skin buffer", size,&buflib_ops));
> + skin_buffer_size = size;
> skins_initialising = false;
> }
> #endif
> @@ -113,7 +136,7 @@
> skin_data_free_buflib_allocs(&skins[j][i].data);
> }
>
> - skin_buffer_init(skin_buffer, SKIN_BUFFER_SIZE);
> + skin_buffer_init(skin_buffer, skin_buffer_size);
>
> #ifdef HAVE_LCD_BITMAP
> skin_backdrop_init();
>
> Modified: trunk/apps/main.c
> ===================================================================
> --- trunk/apps/main.c 2011-09-25 12:19:33 UTC (rev 30598)
> +++ trunk/apps/main.c 2011-09-25 12:55:40 UTC (rev 30599)
> @@ -350,6 +350,9 @@
> #ifdef HAVE_REMOTE_LCD
> lcd_remote_init();
> #endif
> + /* This init call allocates an**unmovable** block so must be
> + * before any other moveable allocs. */
> + theme_init_buffer();
> #ifdef HAVE_LCD_BITMAP
> FOR_NB_SCREENS(i)
> global_status.font_id[i] = FONT_SYSFIXED;
> @@ -390,7 +393,6 @@
> tree_mem_init();
> filetype_init();
> playlist_init();
> - theme_init_buffer();
>
> #if CONFIG_CODEC != SWCODEC
> mp3_init( global_settings.volume,
> @@ -439,8 +441,11 @@
> #endif
> cpu_boost(true);
> #endif
> -
>
> + /* This init call allocates an**unmovable** block so must be
> + * before any other moveable allocs. */
> + theme_init_buffer();
> +
> settings_reset();
>
> i2c_init();
Received on 2011-09-25


Page was last modified "Jan 10 2012" The Rockbox Crew
aaa