Rockbox

Tasklist

FS#11270 - PictureFlow - WPS Integration patch - quick flip by hotkey

Attached to Project: Rockbox
Opened by Chris Savery (csavery) - Thursday, 13 May 2010, 20:17 GMT
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 09 June 2010, 06:24 GMT
Task Type Patches
Category Settings
Status Closed
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 100%
Votes 0
Private No

Details

Hello,

I have created a patch to provide WPS - PictureFlow integration. I can now flip between the album flow screen and WPS screen with one button. This makes a very nice interface for album based selection now.

I have done this by adding a couple new settings options. There is a PictureFlow option for WPS Hotkey, and an "Auto WPS" option in PictureFlow that enables jumping direct to WPS and remembering which album is selected. When you jump back with WPS hotkey it shows the correct album. It also cuts the splash screen so that bouncing between WPS and PictureFlow is very quick. I had to add a new language string for the hotkey setting.

I've tested this with todays build r25992 and on my Sansa Fuze v2 and UI Sim. I have no other devices to test on so I'm hoping that others can check it out.

After playing around with this I believe it is a significant improvement for those who want a slick "album flow" type browsing experience. Since it is enabled by a couple settings only it's completely optional for users.

This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Wednesday, 09 June 2010, 06:24 GMT
Reason for closing:  Accepted
Comment by Chris Savery (csavery) - Saturday, 15 May 2010, 05:51 GMT
I've made an updated patch that also will jump back to PictureFlow when a playlist ends or is stopped. This is because otherwise you end up in the playlist screen and have to go back through the main menu to PictueFlow because WPS screen isn't available when playlist is finished.

In summary these are the items the patch does:

Add a AutoWPS setting in PictureFlow that causes "set" button to goto WPS after starting playback.
Add a PictureFlow setting for WPS Hotkey so that you can jump directly into PictureFlow from WPS.
Detect when a playlist ends/stops and revert to PictureFlow to allow a new album selection (only active when hotkey is PictureFlow).
Add a language id for PictureFlow hotkey setting menu item.
Comment by Chris Savery (csavery) - Saturday, 15 May 2010, 05:54 GMT
BTW this was tested against r26025 using Sansa Fuze v2 and is working fine for me.
Comment by Chris Savery (csavery) - Saturday, 15 May 2010, 07:30 GMT
I have updated my patch again to support both modes. "Direct" will go to WPS on first click, "Via Track list" will go to the track list first and a second click takes you to WPS.

I changed the setting menu item to "WPS Integration".

Patched and tested against r26026.
Comment by Alexander Levin (fml2) - Sunday, 16 May 2010, 10:34 GMT
Why is the new value written to the config file chosen to be "plugin"? It's too unspecific IMO. I'd rather use "pictureflow". Or we could also have the "plugin" option (which would start a plugin) with another setting that would tell which plugin to start. But that would be another patch.
Comment by Chris Savery (csavery) - Sunday, 16 May 2010, 11:52 GMT
That is a mistake. It's a hangover from when I started the coding and wanted to offer a generic "start plugin" hotkey setting. The code there doesn't make it easy to select and save a plugin as the setting so I cut back to just hard coding the PictureFlow plugin and then forgot to change the setting name as well. I've updated it in this new patch attached to be "pictureflow". I tested this one briefly to make sure it didn't muck it up.

I still think a generic "start plugin" would be nice but would require more extensive code changes and would be more disruptive. Another thing I'd prefer is to have the PictureFlow plugin show whatever is the current playing or last played track rather than just a remembered value. It can get out of sync the way it is. This being my first stab at rockbox code I found the tagcache code hard to get a grip on right now. I'd like to figure it out and then use it to enhance this in the future.

Also, I have one album here that seems to crash when selecting it in PictureFlow. I have no idea why and it doesn't seem to be my code. Other albums all work fine. I've been assuming it's something else in this particular build that doesn't like this album.
Comment by MichaelGiacomelli (saratoga) - Sunday, 16 May 2010, 22:14 GMT
Resync for lang file changes.

Overall I like this patch, although I'm not familiar with this part of the code.
Comment by Chris Savery (csavery) - Monday, 17 May 2010, 07:37 GMT
Thanks for resync. Tested now up to r26096.
I didn't know that the patch extension shows patch detail when uploaded here.
I've changed my file naming convention to pictureflow-wps-4.patch so this works correctly.
Comment by Chris Savery (csavery) - Monday, 17 May 2010, 21:16 GMT
Here's another updated revision.
This one adds a couple things. The Set_Long key will append the album to the current playlist instead of replacing the current one.
I like this for queueing up albums.
For Sansa Fuze the Right and Set button both are set to play the album. I've changed this so that right shows the tracklist instead allowing directly choosing a song to play instead of starting at track 1.
I have also included a bug fix here for PictureFlow sometimes segfaulting when showing tracklist or playing albums in a certain position in album list.
This one tested against r26114 on Sansa Fuze v2 and UISim.
Hope people enjoy it.
Comment by Chris Savery (csavery) - Tuesday, 18 May 2010, 11:44 GMT
Another update. Have added a settings item for "Backlight" options: Always On (current default), Normal.
When integrated with WPS you tend to use PictureFlow a lot more and so I've often leave the screen on PictureFlow and noticed the current behaviour is that the backlight timeout is disabled. This new setting allows making the backlight normally work as in other screens.
Comment by Hayden Pearce (St.) - Thursday, 20 May 2010, 12:43 GMT
Chris Savery (csavery),

Hi, great patch.

One thing about it slightly annoys me though, I think that it would be nice if pictureflow displayed the currently playing track's album when you opened it, instead of the last album it played from in pictureflow.


[St.]
Comment by Chris Savery (csavery) - Thursday, 20 May 2010, 13:33 GMT
Yes. I agree and commented that above. I would like to improve that in future but have to learn better how the tagcache code works.
I may fix that soon as I did get a chance to work with the tagcache when fixing the track indexing.
The reason it depends on tagcache is that I have to use the current playing filename-path from WPS to lookup and find the matching album name. Then use that to get the index from the album index so that the correct position in the index is set.
I expect to update the patch to do this.
Comment by Chris Savery (csavery) - Thursday, 20 May 2010, 15:04 GMT
Ok. Have an update that shows the current playing album when started and if nothing is playing then it shows last album played.
I had another look and found a much easier way to do this - so I coded it right away.
Please test and report back. I built against r26185 on Sansa Fuze v2 and UISim.
Much nicer now. Enjoy.
Comment by Chris Savery (csavery) - Friday, 21 May 2010, 12:00 GMT
Update to fix a couple bugs.
1 - Some devices don't have hot keys and so this fix removes some code in that case to prevent compile errors.
2 - On Clip+ when no music playing the check to determine current playing album segfaults. I added a safer check here.
3 - Includes my bug fix for the configdata format not loading "show album title" value correctly.
Comment by Chris Savery (csavery) - Friday, 21 May 2010, 19:58 GMT
Resync to r26239. English lang file changed.
Comment by Hayden Pearce (St.) - Monday, 24 May 2010, 09:16 GMT
Appending albums to the playlist works for me with my Nano1Gs, but not the Nano2Gs.
Any idea why?
The keymap should be exactly the same.
Comment by Daniel Kalle (DonDan) - Monday, 24 May 2010, 12:53 GMT
Nice patch!
Would it be possible to not only be able to append the (entire) album to the current playlist but also just a single song via the PictureFlow Track list?
That would be really awesome because by this way you could do most of the playlist adding from the PictureFlow plugin without needing to go to the Database.


I have created a complementary patch that provides a shortcut at the Root Menu for the PictureFlow plugin.
For those that use the PictureFlow plugin a lot ;-)

http://www.rockbox.org/tracker/task/11308
Comment by Chris Savery (csavery) - Monday, 24 May 2010, 13:26 GMT
Hayden: I don't have any idea. There must be some difference but I'm not familiar with either device.
Does that key do something different or just nothing?

Daniel: I should be able to do that. It sounds like a good idea. Since right-arrow is set to play I'll make it the same as album where the "long select" key will add a track. I'll post an update in a while for that.

Good idea for a root menu item.
Comment by Chris Savery (csavery) - Monday, 24 May 2010, 16:21 GMT
New update - when you hold select key in track list it will add just the track to the current playlist.
Also added a "Clear Playlist" menu item. I found I needed this as there is no easy way to start with a clean playlist and add tracks.
Patched against r26251 but I don't think there was any conflicts since r26239.
Enjoy. Report issues here. Thanks.
Comment by Daniel Kalle (DonDan) - Monday, 24 May 2010, 16:53 GMT

I get following error:
patching file apps/plugins/pictureflow/pictureflow.c
patching file apps/lang/english.lang
patch: **** malformed patch at line 331: Index: src/apps/onplay.c

in line 313 it is:
@@ -13745,3 +13745,31 @@
but it must be
@@ -13745,3 +13745,17 @@

Cheers

Comment by Chris Savery (csavery) - Monday, 24 May 2010, 19:11 GMT
Yes, I see now I edited the patch and made a mistake. Here is a corrected version against r26251.
There were changes to the english.lang file that I hadn't realized.
Comment by Chris Savery (csavery) - Sunday, 30 May 2010, 10:57 GMT
Updated to added new auto-build album cache feature.
Now if the database changes you don't need to manually rebuild the album cover cache in PictureFlow.
It will initially show new albums as missing but upon re-entry it will find and cache them.
It is now smart enough to only rebuild albums that are new unless you do a manual rebuild (which does all abums).
To allow this auto-building I have changed the album cache filenames to use a hash rather than the index value.
(Using modified FNV hash - fast and uniform).
Also if album art is missing then it uses the emptyfile bmp to prevent auto-building over and over.
This means if you are missing artwork and later add it then you need a manual rebuild.

This patch sync'd and tested against r26405 on fuze v2 and SIm.
When first using this you may want to delete your pictureflow cached files as they will be redundant once hashed names are used.
(btw hashed names are used for speed to prevent having to scan all albums on every start but still keeping smart-build of cache)
Comment by Jonathan Gordon (jdgordon) - Sunday, 30 May 2010, 11:56 GMT
looking good... are you happy to have it commited now or are you still working on it?
Comment by Chris Savery (csavery) - Sunday, 30 May 2010, 13:41 GMT
I'd give it a bit more time. I keep thinking of refinements.
I haven't had much feedback about possible bugs. So either it's working or no one is testing it.
If I knew a few others had tried it successfully then I'd be happy to see it committed.
Comment by Daniel Kalle (DonDan) - Sunday, 30 May 2010, 16:00 GMT
@Chris: There is one more issue with the PictureFlow plugin, it is not related to your patch but maybe you know were to look to fix it.

If you scroll thought the cover art during playback the sound stops after a few seconds (~6 s).
It occurs always if you scroll constantly for a certain amount of time, you don't need to scroll fast to make it happen. When it reaches the end of the album list (most left or most right) the playback returns (even if you keep scrolling in the same direction, with still makes the frame rate stay up by 30 - 34 fps).

Could it be that "yield()" statement is missing somewhere?Somewhere it load the covers?
I tried to add three "rb-> yield();" statements in the "update_scroll_animation()" function, I placed these statements more or less randomly, because I don't really know what I am doing :-\. But it seams to solve the issue for a "Normal" usage. If I really keep stressing the system with scrolling back and forth, I am still able to freeze the playback for a a few seconds, but it is not frequent.



Comment by Chris Savery (csavery) - Sunday, 30 May 2010, 16:36 GMT
I noticed that as well and assumed it was pictureflow grabbing memory from the audio buffer and causing it to be starved.
It seemed to occur when a lot of albums were being dropped for new ones - it has a limited buffer and frees images as it needs more for new ones.
If you get improvement by adding yields then maybe it is not memory but process hogging.
I kind of doubt that though as there should never be a case where it would not yield for several seconds.

I'd be interested in which locations inserting yield do improve it. It could be some memory handling (alloc/free) that is not yielding when many changes occur at once.

After working on ways to rebuild the cache on the fly and nothing working well I came to the conclusion that the memory is very constrained and even slight changes in how it's handled caused segfaults for me.
Comment by Daniel Kalle (DonDan) - Sunday, 30 May 2010, 17:56 GMT
I suspect that this works because the "update_scroll_animation()" function is called periodically the timer. It means that the yields are called with the same frequency. I have noticed that applying this more_yield patch I get a lower frame rate than without it. With it is about 29 -31 while scrolling and without it it is 21-26 FPS while scrolling. Maybe this is the magic here! Would it maybe be possible to declare a upper limit for the frame rate? Something like 26 FPS...Still looks OK.

If I reduce the yields to only one (like the first one) in the entire "update_scroll_animation()" function the issue persists. Only with at least the three of them I is gone.
Comment by Daniel Kalle (DonDan) - Sunday, 30 May 2010, 19:38 GMT
This time regarding your patch:
I found that if there is an invalid JPG image as cover image for some album, it will show during the first "Preparing album artwork" process at some point "Could not read bmp". This is as before and wasn't a big issue. But it has now the side effect that the pictureflow-plugin will try to auto-update the cover cache the next time you start the plugin, and it will exit with the following error chain: "could not read bmp" > "no album art found > could not create album art cache. Press any button to continue" > "plugin returned error".
Is it possible to regard the faulty cover image as not present and use the emptyfile bmp to prevent auto-building over and over?
Comment by Daniel Kalle (DonDan) - Sunday, 30 May 2010, 19:55 GMT
The hold select key feature in the track list and in the album list to add the track or the entire album to the current playlist doesn't seems to work anymore (revision 26408 + pictureflow-wps-11.patch). Can you confirm?
Comment by Chris Savery (csavery) - Sunday, 30 May 2010, 20:14 GMT
Here comes #12...

This one adds two yields in what I judged to be critical places where it could get jammed up.
In my testing I could not get the audio to stop no matter how crazy I went with the wheel.

It now copies the empty slide when a source jpg can't be read, and only if the empty slide fails does it emit a message and fail.

The hold-select should still work. I checked the code and tested here and it did for me.
It only works when the setting for WPS Integration is not set to Off.

Still patched against r26405.
Comment by Chris Savery (csavery) - Sunday, 30 May 2010, 20:16 GMT
BTW Let me know if you think the screen is not as smooth now.
I thought it may be a bit chunky but I think that's only when you really spin the wheel and I guess it's better like this than cutting audio.
Chris :)
Comment by Chris Savery (csavery) - Sunday, 30 May 2010, 21:12 GMT
Here's a slight mod of last one that doesn't call yield quite so often.
It cuts down on screen glitches but causes more [?] slides to show up.
I still can't get an audio break though so it's still good.
I called it 12a because it's pretty much the same, just not quite so extreme yielding.
Comment by Daniel Kalle (DonDan) - Sunday, 30 May 2010, 21:22 GMT
Agreed, it is a little bit chunky if playback is on and you scroll the wheel like hell, but I also would say that it is better than cutting the audio!
With your patch I also get frame rates up to 26 FPS, but the bitmap loading is better (faster) the way you placed the yields. Nice!!!. Like I said, I didn't knew what I was doing :)
With playback stopped (and paused) I get frame rates up to 29 FPS, before the yields addition it was max 31 FPS.

Regarding the faulty JPG, now it doesn't show any error message at all and basically it works without a flaw, but would it not be better to even so show the error message "could not read bmp". This way you would be notified that somewhere in the album collection is a faulty cover and you would be able to sort it out. Perfect would it be if it would include the name of the Album that has the faulty JPG. Just a suggestion, nothing acute. :)

Sorry about the hold-select feature, I completely forgot about the WPS Integration settings...

Looking forward to see this patch committed, nice improvements! Makes the PictureFlow usable in a daily fashion.
Comment by Daniel Kalle (DonDan) - Sunday, 30 May 2010, 21:37 GMT
I believe that I had a better experience with the pictureflow-wps-12.patch, no lcd glitches, absolutely no audio cutting.
With pictureflow-wps-12a.patch I experience some lcd glitches and when scrolling extreme fast the audio cuts. The frame rate is slightly higher but I looks that the covers are not loaded as fast as before. I am not sure. Will report after making further testings.
Comment by Chris Savery (csavery) - Sunday, 30 May 2010, 21:38 GMT
Yes, agreed. Next update will have a message "Album art is bad: <album name>".
It won't bother warning if the empty cover is also bad. That shouldn't happen anyway.
I'll hold off until I have something more to incorporate.
Comment by Chris Savery (csavery) - Sunday, 30 May 2010, 21:43 GMT
Regarding 12 vs 12a - I found 12a a bit better but I think there is likely still a better place.
I'll come back later and see if I can find a way that doesn't glitch out the screen as much.
It's quite usable but not perfect. The yield probably needs to sync better to keep it clean.
Comment by Daniel Kalle (DonDan) - Sunday, 30 May 2010, 22:00 GMT
Thanks.

I tryed wps-12 again and I confirm what I have said above about the user experience with pictureflow-wps-12.patch vs pictureflow-wps-12a.patch.
With wps-12 I don't see the covers loading in the background, there are mostly there unlike with wps-12a. Maybe because you can't scroll as fast as in wps-12a? But I definitely prefer wps-12, it is as the scroll speed it slightly adjusted to the available resources. The only drawback that I could imagine with wps-12 is that you take a little longer to scroll from one side of the album collection to the other side. But if somebody only wants an fast catalog maybe he should use the "Database".
I have the impression that there are also a few minor lcd glitches with wps-12 but definitely less than with wps-12a.



Comment by Chris Savery (csavery) - Monday, 31 May 2010, 00:52 GMT
Update 13 - same as 12 for yield placement to remove audio drop outs.

Adds an item for Settings > General > System > Start Screen for PictureFlow.
This allows booting right into PictureFlow on last album ready to select an album and play.

Sync'd to r26427 for language changes.
Comment by Chris Savery (csavery) - Monday, 31 May 2010, 00:52 GMT
Oops. Here's the file.
Comment by Chris Savery (csavery) - Monday, 31 May 2010, 20:57 GMT
Quick update to fix Start Screen option upon exiting.
It will now return to root menu upon exiting with left << button.
Before it ended up in some random place (for me in files tree).
Comment by Chris Savery (csavery) - Monday, 31 May 2010, 21:11 GMT
Bonus post - I found a nice CD icon online and modified it to make a pretty empty slide bmp.
Sweet.
Comment by MichaelGiacomelli (saratoga) - Monday, 31 May 2010, 22:55 GMT
Make sure that CD icon is suitably licensed so that we can distribute it. Preferably something like CC license or else ask the author for permission.
Comment by Chris Savery (csavery) - Tuesday, 01 June 2010, 09:10 GMT
As it is I don't think this one is correctly licensed. At best it's unverified.
I just found it on a "free clipart" type site and changed the background to black and scaled it to 100x100.
Consider it for personal use only and not distributing. I'll see if I can find one on wikipedia that is CC.
Mostly I just wanted to experiment with what format would work ok. This one was saved with Gimp in 16 bit BMP and works fine.
It would look better if the cd edges were anti-aliased though as I just did a quick fill around the edge.
Comment by Daniel Kalle (DonDan) - Tuesday, 01 June 2010, 09:37 GMT
I derived these one from the "The Crystal Project" icon collection. The original icons are LGPL ( http://www.everaldo.com/crystal/?action=license ) and therefore these are also LGPL.
The background from the first 3 are made from the original pictureflow_emptyslide.bmp shipped with rockbox.
I hope that there is something you like. My preferred are the first two: pictureflow_emptyslide_note_with_background_8indexed.bmp and pictureflow_emptyslide_cd_with_background_8indexed.bmp.
All of the BMPs are just 8bit like the original pictureflow_emptyslide.bmp. I hope this helps save some of the precious ram.
I tested them on my fuzev2 and it works.

Cheers

Comment by Chris Savery (csavery) - Tuesday, 01 June 2010, 10:03 GMT
That site is a bit confusing. I'm not sure what is released under GPL or not. The main site appears to be LGPL but if you click on the "stock icons" item it takes you to "YellowIcons" which has details about buying a royalty free license that has restrictions and definitely is not LGPL.

The ones you posted all look fine. Personally I like the one I posted but I'm not suggesting it should be added to the project.
The project one doesn't even really need changing - although I'm not against it being updated either.

Mine was derived from [url=http://www.psdgraphics.com/icons/psd-compact-disc-cd-icon/]here.[/url]
The site owner there states they are free unless for commercial use. But he says you cannot "share" them without a link to his site.
If we want to bother changing it then I'd suggest we find one with more explicit CC type licensing.
Comment by Chris Savery (csavery) - Tuesday, 01 June 2010, 10:06 GMT
This one from wikipedia is GNU license so may make a good starting point.
http://en.wikipedia.org/wiki/File:CD_icon_test.svg
Comment by Chris Savery (csavery) - Tuesday, 01 June 2010, 10:11 GMT
Regarding file size - the empty slide is copied into pfraw format anyway so the original format and size are not important.
I tried a jpg and it wouldn't work without some code changes.
That being said there's no reason to use more than 8-bit (256 colour) mode really.
Comment by Daniel Kalle (DonDan) - Tuesday, 01 June 2010, 10:23 GMT
OK,
if you got to http://www.everaldo.com than you have all this confusing options.
But what I meant was "The Crystal Project" also known as "Crystal Clear" under http://www.everaldo.com/crystal/ and http://commons.wikimedia.org/wiki/Crystal_Clear
and that is for sure under LGPL, that means we can use it and make GPL out of it.

The pictureflow_emptyslide_cd_plain_8_indexed.bmp resembles the pictureflow_emptyslide.bmp that you posted.
I can also upload the 16 or the 32 bit versions of the BMPs if it would increase the quality. What bit depth has the pfraw format?
Comment by Chris Savery (csavery) - Tuesday, 01 June 2010, 10:33 GMT
Ah yes. That wikimedia page and the icons there are more definitive. I'm fine with that.
I think 8-bit indexed is fine. I'm sure no one is going to notice any difference for this use.
I haven't had a look at pfraw to see what format is being used.
Comment by Chris Savery (csavery) - Tuesday, 01 June 2010, 10:46 GMT
This would be my choice from the crystal clear set (saved as 8-bit 100x100 with black background).
I like the non-square look as it stands out from the regular albums and reminds me of loose CDs that are missing covers.
I'm still using my first one on my fuze as it looks nicer.
Comment by Daniel Kalle (DonDan) - Tuesday, 01 June 2010, 11:00 GMT
The only drawback that I noticed with the CD shaped BPMs is that if the slide is not in the center or if you use the "zoom" setting with something other than 100%, the round parts look sluggish. That is why in the end I have decided for my self to use the one with the note on the gray background, because it doesn't suffer to much under this issue and still looks good.
Maybe the pictureflow_emptyslide BMPs should be distributed under the "extras" section as it is a matter of personal taste ;-)
Comment by Hayden Pearce (St.) - Wednesday, 02 June 2010, 13:40 GMT
That .bmp from Chris posted: Tuesday, 01 June 2010, 22:46 GMT+12 is also pretty badly centered...
It's not *awful*, but bad enough for me to notice it on the DAP, then look at the original to confirm it.
the "CD" image needs shifting 1 pixel right, and one pixel down.
Though, even after doing that you can see it's a bit of a rush-job. Just a larger image resized I assume?

Given some time (it's almost 2am here right now...so, not tonight) I'd be happy to purpose make something, as opposed to converting/resizing a premade image.


[St.]
Comment by Chris Savery (csavery) - Thursday, 03 June 2010, 23:12 GMT
Updated to fix glitchy screen painting but still keep ver. 12 yield improvements.
After some time thinking I realized that the src bmp buffer could be changing location during the yield.
So this fix re-inits the src location upon yield return to make sure it gets the right stuff. Looks like it works.
Also resync'd to r26535. Tested on Fuze v2 as usual.

Re: bmp. It was a quick and simple resize of original jpg. No doubt a bit of blur or anti-aliasing could help along with some attention to detail.
If you have the time then go at it, or better to work on the wikipedia one as that could be included.
Comment by Daniel Kalle (DonDan) - Friday, 04 June 2010, 09:06 GMT
Nice!!!
No glitches and smooth! (tested on a fuze2)
Great job.

I noticed that the PictureFlow-plugin doesn't respect the "Idle Poweroff" and therefore it drains the battery if you forget to exit the PictureFlow.
And some further suggestions ;-)
- Concerning the emptyslide BMPs: on a long term, as it will be committed to the svn, I suggest that the BMPs should be part of the "Themes". So if you change the theme it would change also the emptyslide BMP to match the theme.
- The PictureFlow shows currently only the name of the Album, it would be nice if it also would show the name of the Artist (something like 1. row Artist, 2. row Album), it could be optional. This would allow that the cover art is sorted by artist rather than sorted by album, witch is in my opinion more intuitive (you would have all albums from one artist side by side).

Cheers
Comment by Chris Savery (csavery) - Friday, 04 June 2010, 12:59 GMT
I noticed the idle power off not working as well. I did take a brief look and saw no immediate quick fix like with screen dimming.
It's something I'll work on soon. I fell asleep while listening a few days back and noticed the power was still on 5-6 hours later.
I'll have to look into how the normal poweroff idle works and see how to bring it into the plugin.
I think the artist+album sorting/display is a good idea too, so will do that sometime. Coming soon...
Comment by Daniel Kalle (DonDan) - Sunday, 06 June 2010, 17:16 GMT
Hi Chris,
I checked some plugins were the auto power off works and there is no code in these plugins dedicated to this feature, it seams that the plugin does not need anything special to allow an auto poweroff.
It is done by the Power Management in src/firmware/powermgmt.c witch checks, among other statuses, also if there is some disk activity during the timeout defined in the "Idle Poweroff" setting, pooling an storage_last_disk_activity() function.
Could it be that the read_pfraw() function in the PictureFlow is called periodically and causes a repeating "disk activity" even if there is no user interaction?
Just a thought, and as usual I do not know what I am talking about... :)
Comment by Chris Savery (csavery) - Sunday, 06 June 2010, 19:37 GMT
Hi Dan,
I looked into it as well. It appears the normal SYS_POWEROFF event is handled correctly by pictureflow. I did some tests with the sleep timer which also generates this event (as does powermgmt.c) and it works properly for that case. I got as far as concluding something was stopping powermgmt.c from generating the event but hadn't got as far as looking into what was doing it.
Disk activity could be the culprit though, so I'll have a look into that next. It could be that even checking for file existence could prevent idle from occurring.

On the sort by artist+album I did some work as well. The db routines make it fairly easy to do single tag sorting but it's a bit more confusing how to handle two tags. Some tests I did didn't work correctly so it's going to be more complex than I initially hoped for. I'd like to code a solution that allows for more general cases where adding different sort orders would be simple. eg. it may be nice to have a genre+artist+album to keep same types of music together as well. Still looking at this too.
Comment by Daniel Kalle (DonDan) - Sunday, 06 June 2010, 21:11 GMT
I have commented out the load_new_slide() funktion in the "background thread":
=================================================
void thread(void)
{
long sleep_time = 5 * HZ;
struct queue_event ev;
while (1) {
rb->queue_wait_w_tmo(&thread_q, &ev, sleep_time);
switch (ev.id) {
case EV_EXIT:
return;
case EV_WAKEUP:
/* we just woke up */
break;
}
/*while ( load_new_slide() ) {
rb->yield();
switch (ev.id) {
case EV_EXIT:
return;
}
}*/
}
}
==================================================
and this way it obwiosly doent show no cover images anymore(only the emptyslide BMP) but the auto power down feature works.
This is of course not a solution, but it confirms that the issue is related with the "disk activity".
I tryed to change load_new_slide() funktion, but it is far to comlicated for me =/ .
Comment by Chris Savery (csavery) - Sunday, 06 June 2010, 21:32 GMT
Dan,
Here's an update with fix for the idle poweroff bug. Your idea on file access was good and led me fairly quickly to the problem.
There is some wonky code in there that waits for events and times out every 5 seconds. When the timeout occurs it re-loads a slide even when there is no reason to. I simply bypass the re-load when the event is due to a timeout. Ideally this code should be re-written to not use a timeout period. There doesn't appear to be any reason for it.
Tested against r26535 on Fuzev2.

Comment by MichaelGiacomelli (saratoga) - Sunday, 06 June 2010, 21:35 GMT
I think the WPS integration patch should go in before you put too much effort into rewriting the rest of pictureflow. Separate patches for separate changes are nice. Maybe ping jdgordon when you're ready and get this into SVN.
Comment by Chris Savery (csavery) - Sunday, 06 June 2010, 21:42 GMT
Michael,
Would you suggest breaking it into a series of separate patches to go into svn?
There are several distinct issues in this patch and some are bug fixes that may be nice to have if even some other changes need to be reverted due to problems on other devices.
Comment by MichaelGiacomelli (saratoga) - Sunday, 06 June 2010, 21:47 GMT
I would put the parts needed for WPS integration, and any bug fixes specific to getting that to work in one patch. Then I would put anything else into subsequent patches.
Comment by Chris Savery (csavery) - Monday, 07 June 2010, 01:58 GMT
Ok. I've taken #16 and broken it into 7 separate but consecutive patches. They must be done in order.
Each one is named according to the changes it includes. It's not so easy breaking them up and hopefully I didn't make any mistakes.
Since svn won't do local diffs I had to use the diff program with the unified option.
These should be ready for svn as I tested them again after breaking up.
I have sync'd to r26638 due to language changes in svn.
Comment by Chris Savery (csavery) - Monday, 07 June 2010, 13:17 GMT
Here's an updated #1 that replaces the #1 above. Much simpler changes outside pictureflow and more default behaviour.
This fixes issues in root_menu.c, wps.c and restores current "return code" screen changes.
It removes load_plugin calls and leaves only one in root_menu with a new generic load_plugin_screen function.
As is it achieves mostly what I want. It doesn't make pictureflow a default screen on music stop.
It doesn't touch pictureflow.c so patches #2-#7 should still work fine after #1a.

There is a known problem I'm tracking down now. When selecting a track from the pictureflow tracklist it starts WPS ok but for some reason the first key after that gets eaten. Whether next/prev/hotkey whatever I tried, the first keystroke does nothing. Not sure where this came from or why. It may have been there before anyway. Will try to track it down and do a separate patch on it.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 June 2010, 02:29 GMT
I'm not seeing the button issue (in the sim anyway)...
A few more comments:
it looks like unless start screen is pictureflow the previous browser doesnt work... I can see why but I dont like it.. I'm almost considering allowing the database option to load the dbtree or pictureflow as an option.
it appears we need a HAVE_PICTUREFLOW define... HAVE_TAGCACHE isnt enugh because some targets might not be setup for pf (does it work on grey/mono displays?)
add a wps context menu optoin to go directly to pictureflow to make it more obvious you can do that (or lets you use the hotkey for something else which makes sense if you use revious browser anyway)
The while loop in root_menu needs a splash or something... splash(0, LANG_LOADING); should be enough (and drop the action_userabort(HZ) timeout to HZ/5 or something to make it faster when it isnt initialiy ready).


which target are you testing on? the button eating might be keymap related.

If you set the "follow playlist" setting (under generl > playback) it will return to previous browser when music stops instead of going to the menu
Comment by Chris Savery (csavery) - Wednesday, 09 June 2010, 02:48 GMT
I'm testing on Fuzev2. It's good it doesn't happen on other devices then. There is a custom context keymap so it's likely something there.
I think pf does work for grey/mono displays. I worked on a fix patch for someone using pf with Clip+ and it was todo with mono display menu issues. I tested that with Clip+ in Sim and it worked fairly well despite very small screen and being greyscale. I can't speak for other devices though. Don't really know.
A WPS context menu item is a good idea. I'll look into that.
Usually on my Fuze the start screen wait for db is so brief you don't even notice. Except when it's commiting the db at start, in which case I don't know if we'd want a splash screen. It just does the commit (with status msg) and then pops into pf right after. I use the start screen always. Maybe on other devices it is slow? People can't use pf without the db/tag_cache b/c it won't start anyway. It will give splash a plugin_error and return.
I added the wait_for_db loop b/c otherwise it is never ready at start and pf will always return with error.
I can set it for HZ/5 - no reason not to, was just an arbitrary choice when I did it.
I don't understand this comment - "I'm almost considering allowing the database option to load the dbtree or pictureflow as an option." ??
Comment by Jonathan Gordon (jdgordon) - Wednesday, 09 June 2010, 02:54 GMT
hard disk targets will take longer to start and be ready than the fuze.

I meant adding a setting so the database menu option would actually load pictureflow instead of the database browser.
Comment by Chris Savery (csavery) - Wednesday, 09 June 2010, 04:03 GMT
Here's an updated #1b.
It has wps context menu item for pictureflow.
Added splash during start screen wait. Couldn't find LANG_LOADING but used LANG_TAGCACHE_BUSY instead, which makes sense.
Also HZ/5 for shorter waiting.
Haven't added database setting to use PictureFlow yet. Maybe leave that for discussion?

I tried your suggestion on "Follow Playlist" but didn't seem to make any difference. I'm fairly happy with behaviour on play stopping.
When last used pf it goes there on stop. If you drop down to menu and return to wps then stop, it goes to menu on stop.
But if you hit pf first, then wps and stop it goes to pf on stop. I tend to bounce b/w wps and pf, so for me it works as desired.

Loading...