Rockbox

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, 11:44 GMT
Last edited by Jonathan Gordon (jdgordon) - Friday, 18 September 2009, 05:16 GMT
Task Type Bugs
Category Themes
Status Closed
Assigned To No-one
Operating System Gigabeat F/X
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
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, 05:16 GMT
Reason for closing:  Fixed
Comment by Thomas Martitz (kugel.) - Wednesday, 09 September 2009, 15:14 GMT
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, 15:49 GMT
try thsi patch
Comment by Robert Kukla (roolku) - Wednesday, 09 September 2009, 16:04 GMT
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, 16:11 GMT
f
Comment by Robert Kukla (roolku) - Wednesday, 09 September 2009, 16:42 GMT
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, 16:52 GMT
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.) - Wednesday, 09 September 2009, 23:49 GMT
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, 00:48 GMT
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, 00:50 GMT
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, 02:02 GMT
This should fix the crash/freeze.

edit: 2nd try
Comment by Jonathan Gordon (jdgordon) - Thursday, 10 September 2009, 05:01 GMT
Pretty sure u want to redo that..... x-x always =0
Comment by Thomas Martitz (kugel.) - Thursday, 10 September 2009, 10:46 GMT
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, 12:50 GMT
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, 12:55 GMT
Sure it will display it. You need %?C if you want something conditionally on that.
Comment by Jonathan Gordon (jdgordon) - Thursday, 10 September 2009, 16:42 GMT
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, 16:50 GMT
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, 15:48 GMT
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, 13:04 GMT
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, 13:23 GMT
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, 14:12 GMT
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.
Comment by Teruaki Kawashima (teru) - Monday, 14 September 2009, 14:24 GMT
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, 14:28 GMT
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, 14:33 GMT
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...