FS#11310 - PictureFlow - mono tracklist for mono displays

Attached to Project: Rockbox
Opened by Chris Savery (csavery) - Monday, 24 May 2010, 13:36 GMT
Task Type Patches
Category Plugins
Status Unconfirmed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No


This very small mod changes the tracklist so it doesn't fade lines when on a mono display (bit depth 1). Tested against r26241 in Sim only (because I don't have a mono display device).
Please report back if problems.
This task depends upon

Comment by Alejandro Arellano (Vague Rant) - Monday, 24 May 2010, 14:46 GMT
This does noticeably improve the display, but the text of the unselected track titles continues to flicker faintly even so; it appears that all text is flickering at the same rate rather than in accordance with its distance from the selected track, but certainly at a much reduced rate and readability has certainly been improved regardless. Also worth noting is that the highlight bar also flickers as it is not a solid colour; perhaps an inverted colour system (bar coloured, text black, as Rockbox does in its own menus) would improve that.

It's difficult to demonstrate either issue given that the simulator doesn't replicate mono screens very accurately and you can't show flickering in a photo, but attached is a picture demonstrating the potential readabiity issue caused by the selection bar; it's more legible when not being captured by a low-quality webcam, but could be a problem, particularly for users with poor eyesight.
Comment by Chris Savery (csavery) - Monday, 24 May 2010, 16:06 GMT
I'd suggest you make a manual change to the patch and let me know if it looks better.
Go to the few lines changed and you'll see the value 250 is used for text colour except for the current line which is 255.
Try making them all 255 and see how it looks. It may be that anything but 0 and 255 get represented with dithering and cause flicker.
I kept the 250 value as it offers a slight brightness change to indicate highlight but if it cannot show anything but on/off then 0 and 255 may be better.
Let me know if that looks better and I'll make a new patch. Also the highlight may look better as a inverse (black lettering on white bar).
If you're not sure what I mean or what to change then just let me know and I'll upload one for you to test out.
Comment by Alejandro Arellano (Vague Rant) - Monday, 24 May 2010, 16:54 GMT
Indeed, setting the brightness to 255 has it working perfectly with no flicker; I did try to do an inverse bar but only got as far as making the text black before losing my way. Sorry this isn't a patch, but my pictureflow.c already has several other patches applied and also I have no idea how to make a patch; this is as far as I got:

if ( track_i == selected_track ) {
#if LCD_DEPTH > 1
draw_gradient(titletxt_y, titletxt_h);
Comment by Chris Savery (csavery) - Monday, 24 May 2010, 19:43 GMT
It can get a bit confusing with several patches and I've made mistakes editing out bits sometimes.
Here is the above patch updated with the color 255 change for others who may want it.
It's against r26251 now.
Comment by Chris Savery (csavery) - Monday, 24 May 2010, 19:45 GMT
And here is the patch updated with trial inverted menu bar code.
Check if it looks better and post here so others know which one is better to use.
Also against r26251 and tested here only in Sim.
Comment by Alejandro Arellano (Vague Rant) - Monday, 24 May 2010, 21:32 GMT
Compiled from a clean, up-to-date pictureflow.c with that latest patch applied; the highlight bar is still not a solid colour and so has the flickering/dithering effect. Since the bar is ostensibly "grey" it's about as legible now as it was with "white" text, so I really wouldn't want to say whether one or the other was superior, it'd really come down to a matter of preference.

Looking at the patch, I think the problem might be that draw_gradient is being called on all devices, when it really should only be called on devices with bit depth > 1. Unfortunately, the fix is not as simple as putting the draw_gradient behind the #if LCD_DEPTH > 1 line; this simply results in no bar appearing at all meaning that the currently selected track is black-on-black and entirely invisible.
Comment by Chris Savery (csavery) - Tuesday, 25 May 2010, 05:29 GMT
Ah yes, that would be likely. It's too bad I don't have a mono device to see this type of thing before uploading a patch.
Here is a pair of updates with the draw_gradient modified to degrade to solid color when bit depth is 1.
Hopefully this will hot the spot.
Comment by Alejandro Arellano (Vague Rant) - Tuesday, 25 May 2010, 07:42 GMT
Getting closer;

pictureflow-mono-list.patch gives a solid bar which I suspect is overlaid over the text; if the gradient isn't a background but an overlay, setting a solid colour overlay just results in obscuring the text--that's my best guess as to what's happening here. Incidentally, this patch also causes an error while compiling:

CC apps/plugins/pictureflow/pictureflow.c
/home/vague/rockbox/apps/plugins/pictureflow/pictureflow.c: In function ‘draw_gradient’:
/home/vague/rockbox/apps/plugins/pictureflow/pictureflow.c:2251: warning: unused variable ‘c2’
LD pictureflow.rock

pictureflow-mono-list-inv.patch looks the same as the previous version; black text on flickerbar.
Comment by Chris Savery (csavery) - Tuesday, 25 May 2010, 11:35 GMT
Ya, something like that is probably happening.
I knew about the compiler warning but didn't want to add a few more defines to remove.
Just an unused variable hanging around from color version code - causes no problem.
I'll get back to have another look a little later.
Comment by Chris Savery (csavery) - Friday, 28 May 2010, 11:48 GMT
Here's an updated version that shows the menu bar as black text on white bar.
Confirmed to work ok on Clip+ and in USim - against r26251.

Comment by Thomas Martitz (kugel.) - Thursday, 10 June 2010, 08:55 GMT
I think this should be only for the clip, since the greylib works properly for other mono displays.
Comment by Chris Savery (csavery) - Thursday, 10 June 2010, 11:34 GMT
When using greylib does LCD_DEPTH have value 1 or some higher value dependent on greyscale bit depth?
Comment by Chris Savery (csavery) - Thursday, 10 June 2010, 11:55 GMT
Hmmm. Just had a look a various config headers. It seems these devices have LCD_DEPTH == 1 (other than Clip variants):
archos*, iriverifp7xx, logikdax, mrobe100, sansam200.
If we're sure that these players display fine with current support then I need some other value to key off.
I don't see a "Clip family" define.
Suggested solution? Add #define LCD_STRICT_MONO for Clip family devices?
Comment by Chris Savery (csavery) - Thursday, 10 June 2010, 14:11 GMT
Updates to this patch are combined in with reflection in FS#11300.
See that task for new patch just posted.
Comment by Thomas Martitz (kugel.) - Thursday, 10 June 2010, 18:26 GMT
#if CONFIG_LCD == LCD_CLIP should work