FS#11665 - LCD functions ignore viewport bg_pattern if a background image is loaded

Attached to Project: Rockbox
Opened by Jonathan Gordon (jdgordon) - Monday, 11 October 2010, 03:02 GMT
Last edited by Dave Chapman (linuxstb) - Monday, 11 October 2010, 14:23 GMT
Task Type Patches
Category LCD
Status New
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.6
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No


at least in the 16bit lcd drivers, all functions which use transparency (or clear pixels) get the pixel colour from the backdrop image, completly ignoring the bg_pattern.

This isnt a problem except in skins where it is perfectly reasonable to want a background image for most of the screen but a solid colour for one or two viewports.

There is a tiny problem though, bg_pattern is set to LCD_DEFAULT_BG which is a valid colour, so we probably need to add a flag to the viewport struct to say if it should ignore the background image or not.
This task depends upon

Comment by Jonathan Gordon (jdgordon) - Monday, 11 October 2010, 10:50 GMT
I'm not sure about the 2bit versions, 16bit seems correct.

lcd-2bit-vert.c has "if ((current_vp->drawmode & DRMODE_BG) && !lcd_backdrop)" and lcd-2bit-vi.c has if ((current_vp->drawmode & DRMODE_BG) && !backdrop), should these blocks get entered when lcd_using_backdrop()==false ?
Comment by Jonathan Gordon (jdgordon) - Monday, 11 October 2010, 12:59 GMT
cleaner version, just fiddle with the function pointers.
The question in the above comment still stands
Comment by Dave Chapman (linuxstb) - Monday, 11 October 2010, 14:22 GMT
A few comments:

1) This isn't a bug, it's behaviour you (and I think everyone) would like to change. So simply a new feature.

2) It's called the "backdrop", so "ignore_backdrop" might be a better variable name than "ignore_bg_image".

3) "lcd_fix_functions" isn't a very descriptive name for the that function. How about "lcd_set_fastpixelfuncs()" ? Also, why define a prototype for that function, then implement it later than needed? You could simply place it before it's called in the code. Also, why make it "inline" - shouldn't that decision be left to the compiler?

There is also the unresolved question raised by amiconn in IRC today about whether this should be an attribute in the viewport, or if it should be part of the drawmode.

I should add that I haven't looked deep enough to see if this parch actually looks correct. I assume you've tested it, and that it works fine when you set that ignore_bg_image variable to true?

Comment by Jonathan Gordon (jdgordon) - Monday, 11 October 2010, 23:30 GMT
1) semanics
2) fair enough
3) because otherwise the file needs to be rearranged, the fastpixelfuncs are half way down the file and the viewport funcitons are at the top.. this is simply to get it working.