Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#10596 - extra C displayed under album art and freeze when no backdrop

Attached to Project: Rockbox
Opened by Robert Kukla (roolku) - Wednesday, 09 September 2009, 13:44 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Friday, 18 September 2009, 07:16 GMT+2
Task Type Bugs
Category Themes
Status Closed
Assigned To No-one
Player Type Gigabeat F/X
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

r22667, observed on target and sim

I noticed an extra C in my WPS when the album art didn't fill the whole view port. In an attempt to narrow down the cause I removed items until all that remained was:

----snip----
### Basic Setup ###
%X|wpsbackdrop.bmp|

### Cover ###
%V|0|0|-|240|1|CECFCE|000000|
%Cl|0|0|c240|b240|
%C
----snip----

If there is an album art of say 240x200 a C will show in the top left corner. (not sure how long it has been like this as most of my album art is square)

However, if I also remove the backdrop like here:

----snip----
### Cover ###
%V|0|0|-|240|1|CECFCE|000000|
%Cl|0|0|c240|b240|
%C
----snip----

then rockbox hangs as soon as you go to the WPS (so parsing seems to be okay).
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Friday, 18 September 2009, 07:16 GMT+2
Reason for closing:  Fixed
Comment by Thomas Martitz (kugel.) - Wednesday, 09 September 2009, 17:14 GMT+2
Between s/"parsing seems to be ok"/"parsing didn't fail loudly, but possibly produced bullshit"/ :)

Can you tell the revision it was introduced?
Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 September 2009, 17:49 GMT+2
try thsi patch
   aafix.diff (0.5 KiB) 
 apps/gui/skin_engine/skin_tokens.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comment by Robert Kukla (roolku) - Wednesday, 09 September 2009, 18:04 GMT+2
Now it segfaults in the sim:

Program received signal SIGSEGV, Segmentation fault.
[Switching to thread 43600.0xabe8]
0x004346ac in get_token_value (gwps=0x52fc40, token=0x52fd10,
buf=0x569fc00 "\006", buf_size=128, intval=0x0)
at /home/robert/clean/apps/gui/skin_engine/skin_tokens.c:326
326 *intval = 1;
(gdb)
Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 September 2009, 18:11 GMT+2
f
   aafix.diff (0.6 KiB) 
 apps/gui/skin_engine/skin_tokens.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comment by Robert Kukla (roolku) - Wednesday, 09 September 2009, 18:42 GMT+2
okay, the version with backdrop now works.

When trying the second example without backdrop it either freezes (if I resume playback immediately after starting the simulator) or it segfaults (if I resume playback after (re-) selecting the wps in the theme settings):

Program received signal SIGSEGV, Segmentation fault.
[Switching to thread 44616.0xac14]
0x00431344 in get_line (gwps=0x52fc40, subline=0x55bfa0, align=0x559fd00,
linebuf=0x559fd10 "", linebuf_size=260)
at /home/robert/clean/apps/gui/skin_engine/skin_display.c:640
640 while (*value && (buf < linebuf_end))
(gdb)

Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 September 2009, 18:52 GMT+2
ok, ill try to investigate this properly in the next few days... if you want you can go back to the svn code and just change the C to a space.... (return " "; instead of return "C";)
Comment by Thomas Martitz (kugel.) - Thursday, 10 September 2009, 01:49 GMT+2
That wps works fine for me (tried it in e200 sim).

### Cover ###
%V|0|0|-|220|1|CECFCE|000000|
%Cl|0|0|c220|b220|
%C
Comment by Michael Chicoine (mc2739) - Thursday, 10 September 2009, 02:48 GMT+2
I am also seeing this "C" on the e200 widecabbie theme. The "C" is located at the top left corner of the AA viewport. I believe it started with r22646. I have been able to reproduce in the e200 sim. I have not had Rockbox hang in my case.
Comment by Thomas Martitz (kugel.) - Thursday, 10 September 2009, 02:50 GMT+2
I'm able to reproduce the freeze/crash with the UnhelpulAA theme (you can find it on the theme site for e200).

Interestingly, it doesn't have any images (except AA and progressbar, but those don't count).
Comment by Thomas Martitz (kugel.) - Thursday, 10 September 2009, 04:02 GMT+2
This should fix the crash/freeze.

edit: 2nd try
   crash_fix.diff (0.7 KiB) 
 b/apps/gui/skin_engine/skin_parser.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Thursday, 10 September 2009, 07:01 GMT+2
Pretty sure u want to redo that..... x-x always =0
Comment by Thomas Martitz (kugel.) - Thursday, 10 September 2009, 12:46 GMT+2
there's no x-x, it's x - x ? 1:0.

the ? takes precedence, doesn't it?
Comment by robin (robin0800) - Thursday, 10 September 2009, 14:50 GMT+2
I also discovered yesterday that this "%C<%t10......" will display album art if true which is confusing to say the least %C should be the only tag to display album art IMHO
Comment by Thomas Martitz (kugel.) - Thursday, 10 September 2009, 14:55 GMT+2
Sure it will display it. You need %?C if you want something conditionally on that.
Comment by Jonathan Gordon (jdgordon) - Thursday, 10 September 2009, 18:42 GMT+2
curr_line->curr_subline->last_token_idx = wps_data->num_tokens - wps_data->num_tokens ? 1 : 0;
add brackets
curr_line->curr_subline->last_token_idx = (wps_data->num_tokens - wps_data->num_tokens ? 1 : 0);
curr_line->curr_subline->last_token_idx = ((wps_data->num_tokens - wps_data->num_tokens) ? 1 : 0);
wps_data->num_tokens - wps_data->num_tokens is never going to equal anything other than 0
Comment by robin (robin0800) - Thursday, 10 September 2009, 18:50 GMT+2
Kugal my mistake I meant to write %?C<t10...... I.e. the conditional does this hence perhaps a bug?
Comment by Mark Fawcus (yapper) - Sunday, 13 September 2009, 17:48 GMT+2
I'm still seeing extra C's with r22695 on the Plain&Simple2 theme for the c200v1. This wps alternates textual information with album art using a 5 second timer, but the album art is being left on the screen.
Comment by Teruaki Kawashima (teru) - Monday, 14 September 2009, 15:04 GMT+2
I think we shuold separate %C tag to two tags, %C and %Cd.
%C indicates whether album art is available while %Cd displays aa.
but this breaks existing wps...
What is problem of current implementation is it doesn't care if %C tag is used as conditional or not.
Displaying aa even when %C is used as conditional e.g. %?C<...> is not correct behavior and makes it difficult to use %?C<...> in the viewport where aa isn't intended to be displayed. I'm guessing this is the cause of  FS#10599 .
thus %C should be handled completely differently if it is used as conditional.
this would be almost equivalent to have two tags except about breaking existing wps.
Comment by Thomas Martitz (kugel.) - Monday, 14 September 2009, 15:23 GMT+2
I thought about that too. Breaking all WPSs isn't really nice. Can we fix %?C to not display the AA? Other conditionals don't show this behavior too.
Comment by Thomas Martitz (kugel.) - Monday, 14 September 2009, 16:12 GMT+2
This should fix that AA is drawn due to the conditional (%?C).
i can't tell yet why the AA in  FS#10599  is drawn in the wrong vp.
   fix-aa-drawn-in-cond.diff (1.7 KiB) 
 b/apps/gui/skin_engine/skin_display.c |   15 ++++++++-------
 b/apps/gui/skin_engine/skin_tokens.c  |    1 -
 2 files changed, 8 insertions(+), 8 deletions(-)

Comment by Teruaki Kawashima (teru) - Monday, 14 September 2009, 16:24 GMT+2
function parse_albumart_display in skin_parser.c sets the viewport for albumart while parsing %C and %?C<..>.
so, album art is drawn in the last viewport having %C or %?C<..>.
Comment by Thomas Martitz (kugel.) - Monday, 14 September 2009, 16:28 GMT+2
arg, that's a tricky one. I don't think that's fixable very cleanly without introducing %Cd
Comment by Thomas Martitz (kugel.) - Monday, 14 September 2009, 16:33 GMT+2
Introducing %Cd is possibly a good idea if we want multiple album arts (different sizes, or for the next track) too (i.e. with an identifier following, such as %Cda).

Loading...