WPS tokenizer: REPORT PROBLEMS HERE

[DukeLuke]

I've got a problem. Since the inclusion of the patch, the sleep timer of my WPS "azure v2" for e200 doesn't show up anymore. The hold switch DOES show up, but it won't disappear when disabling the hold mode

[Lear]

I'm not sure if this is related to the tokenizer, but when I play MP3 tracks (with ID3V2 tags) I sometimes get slightly corrupted text in the WPS. E.g., track number is an empty string (but still present), year shows part of a replaygain tag name and similar things. Has anyone else seen that? There has been some minor changes to the ID3 reader, but I doubt they would have effects of this kind.

I don't have access to a development environment at the moment, so I can't make any closer investigation of it right now.

[NicolasP]

I've got a problem. Since the inclusion of the patch, the sleep timer of my WPS "azure v2" for e200 doesn't show up anymore. The hold switch DOES show up, but it won't disappear when disabling the hold mode

I'm pretty sure I solved the hold switch with r13054. Did you try this version ?
I added the sleep timer tag in r13056.

I'm not sure if this is related to the tokenizer, but when I play MP3 tracks (with ID3V2 tags) I sometimes get slightly corrupted text in the WPS. E.g., track number is an empty string (but still present), year shows part of a replaygain tag name and similar things. Has anyone else seen that? There has been some minor changes to the ID3 reader, but I doubt they would have effects of this kind.

I don't have access to a development environment at the moment, so I can't make any closer investigation of it right now.

I saw this once on the sim and thought it could be related to cuesheets. Once I disabled them the bug disappeared. Do yo have cuesheet support activated ? Anyway I'll try to find the cause of this but help is welcome

[DukeLuke]

I've got a problem. Since the inclusion of the patch, the sleep timer of my WPS "azure v2" for e200 doesn't show up anymore. The hold switch DOES show up, but it won't disappear when disabling the hold mode

I'm pretty sure I solved the hold switch with r13054. Did you try this version ?
I added the sleep timer tag in r13056.

Yay, solved  

[roolku]

A few more issues with the tokenizer I am afraid:

1) I can confirm Lear's observation, specifically

year shows part of a replaygain tag name

It is hard to reproduce, but I managed to get it several times by skipping backwards in quick succession.

2) conditionals don't work properly:

Code: [Select]

%?rp<| %rp>
%?rr<|%rr>

should display playcount/rating respectively if not 0 but nothing is displayed ever.

3) a dynamic tag which is right aligned such as:

Code: [Select]

%ar%pv

will clear the line including part of any graphic that is displayed to the left of it (conditionally if that matters) I suspect the drawing order has changed?

All observed on gigabeat with svn r13060.

Cheers

[Llorean]

I'm pretty sure that updating (or scrolling) aligned right strings cleared images to their left in the past. I'm almost positive I ran into this problem before, since lines are updated as lines.

[roolku]

I'm pretty sure that updating (or scrolling) aligned right strings cleared images to their left in the past. I'm almost positive I ran into this problem before, since lines are updated as lines.

All I can say is that I have been using the WPS pre-tokeniser without problems. Since the image is conditional as well, I suspect the drawing order has changed so that the line is drawn after the image now, whereas before it was the other way around.

[NicolasP]

Roolku:
I'm trying to find where the 1) comes from. I managed to reproduce it too but I have absolutly no idea what causes it.

The 2) looks like a non-issue to me. According to my tests, the %rr and %rp tags behave exactly like described in the CustomWPS wiki page. The examples you give are true/false constructs, which return true if the tag has a value and false if the tag doesn't. Playcount and rating are numeric so they always have a value, so they'll always return the first case, which is empty in your examples. According to the wiki page, the rating is supposed to be used in an enum with 10 cases and the playcount isn't really meant to be used in a conditional...

About 3), it's possible that some things are refreshed differently than in the old WPS code. This is to try to be more effective. I'd like to see the WPS that has this problem. Also, I'm working on a patch to fix FS#6056, so this might change. You could try the patch and tell me if it helps. I've posted it on the bug entry.

[roolku]

Roolku:
I'm trying to find where the 1) comes from. I managed to reproduce it too but I have absolutly no idea what causes it.

It just occurred to me that it might be worth checking the "ID3 View" to see if it is a display only issue or of the year_text pointer is changed (out of bounds access somewhere?).

The 2) looks like a non-issue to me. According to my tests, the %rr and %rp tags behave exactly like described in the CustomWPS wiki page. The examples you give are true/false constructs, which return true if the tag has a value and false if the tag doesn't.

Sort of. With the old implementation is was possible to abbreviate enumerations.

%?rr<|A|A|A|A|A|A|A|A|A|A>

to

%?rr<|A>

i.e. if there weren't enough cases in the tag, it would use the last one (documented as ELSE in CustomWPS).

I have also used the same mechanism to only display the audio codec if it wasn't mp3:

%?fc<%t4%fc|%t4%fc|%t0|%t4%fc>;%t4test1;%t4test2

and it still works here (thanks again for fixing the %t0 issue)

EDIT3: I have since found out that the problem comes from your assumption that enumerations must have at least 3 possible cases in gwps-common.c:1319:

Code: [Select]

   if (num_options > 2)  /* enum */
which explains why the codec example works. And indeed

Code: [Select]

%?rr<|A|A>  
works as well. This seems to be an unnecessary limitation to me.

Playcount and rating are numeric so they always have a value, so they'll always return the first case, which is empty in your examples.

This mechanism you describe only works/ed if *intval is not used to make the tags enums, but *intval is used here (or at least should be).

According to the wiki page, the rating is supposed to be used in an enum with 10 cases and the playcount isn't really meant to be used in a conditional...

Yes, it was not documented for %rp (hides head in shame), but playcount was an enum in the old implementation. And it still tries to be one if you look at your own code. But it doesn't work with the abreviated notation:

Code: [Select]

       
case WPS_TOKEN_DATABASE_PLAYCOUNT:
            if (intval) {
                *intval = id3->playcount + 1;
            }
            snprintf(buf, buf_size, "%ld", id3->playcount);
            return buf;


About 3), it's possible that some things are refreshed differently than in the old WPS code. This is to try to be more effective. I'd like to see the WPS that has this problem.

I have attached a simplified wps that should demonstrate the issue (has also the examples above).

EDIT: oops, no attachments anymore it seems? I'll try to catch you on irc and dcc it.

EDIT2: you are probably hunting easter eggs instead of bugs right now. You can get the wps here: http://roolku.pwp.blueyonder.co.uk/wps.zip

Also, I'm working on a patch to fix FS#6056, so this might change. You could try the patch and tell me if it helps. I've posted it on the bug entry.

I tried it, but it failed to apply.

All test with r13069 on gigabeat. Thanks for your efforts.

[Lear]

Roolku:
I'm trying to find where the 1) comes from. I managed to reproduce it too but I have absolutly no idea what causes it.

I checked the ID3 info screen when I saw the problem, and it showed the same (incorrect) values there. This suggest it isn't the tokenizer, unless it corrupts memory somehow. Whatever the cause, it doesn't always happen for a certain file.

[stripwax]

It seems like scrolling lines that contain the next track info tag no longer scroll?  Unfortunately I don't have access to an earlier build so I'm not sure how/when/if this broke.  But the following (for me) doesn't scroll long track titles at all

%s%?It<%It|>

I see the same broken behaviour if I run the Unicatcher wps in the sim  (I'm using Ipod Video sim, if that matters).  Take the %ac out of the "%s%acNext: ..." line to see what I mean.  If I pick a long track title, the WPS displays the truncated track title and the line doesn't scroll.

[stripwax]

Hm, why is all the 'next' metadata set to be WPS_REFRESH_DYNAMIC --  apart from %IA (album artist) which is WPS_REFRESH_STATIC - ?  That doesn't make much sense to me.
Setting all the 'next' metadata to be WPS_REFRESH_STATIC seems to fix the above bug.   Am I missing something?

The bug by the way is that the 'next track' text is getting updated continually.  write_line gets called constantly with the (same) next track text. This means that puts_scroll also gets called constantly, so the scroll offset just gets reset to 0 all the time and the scroll_thread doesn't get a chance to actually scroll this text

Edit - amiconn pointed out that the 'next track' metadata needs to be dynamic on hwcodec devices because it's not available until the next track is already in the hw buffer.  Maybe I'll pore over the svn diffs and try and work out how/why this worked before the tokenizer code.

[stripwax]

Here, this patch seems to fix.  The new wps code wasn't testing WPS_REFRESH_SCROLL refresh flag, now it is.  If it's a scroll line and refresh_scroll & WPS_REFRESH_SCROLL is not set, do nothing.  I believe this is the original behaviour too.
I've also put all the 'current' track tags to be STATIC and all the 'next' track tages to be DYNAMIC

http://www.rockbox.org/tracker/task/7000

[NicolasP]

I committed your fix, thanks a lot

[Lear]

Roolku:
I'm trying to find where the 1) comes from. I managed to reproduce it too but I have absolutly no idea what causes it.

Found and fixed it. A recent change in playback.c was the culprit.