Bug 15753 - Tile:loadImage() should return nil when the file path is non readable
: Tile:loadImage() should return nil when the file path is non readable
Status: CLOSED FIXED
Product: SqueezePlay
Classification: Unclassified
Component: UI Skin
: 7.5.x
: PC Other
: P2 normal (vote)
: 7.5.0
Assigned To: Alan Young
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-22 08:59 UTC by Ben Klaas
Modified: 2010-04-08 17:26 UTC (History)
3 users (show)

See Also:
Category: Bug


Attachments
picture of controller with bad screen drawing (2.65 MB, image/jpeg)
2010-02-22 08:59 UTC, Ben Klaas
Details
movie of behavior (17.33 MB, application/octet-stream)
2010-02-22 09:04 UTC, Ben Klaas
Details
Zip of c:\users\username\AppData\Roaming\SqueezePlay when screen is drawn incorrectly (4.59 KB, application/x-zip-compressed)
2010-02-22 10:20 UTC, Mike Richichi
Details
c:\users\username\AppData\Roaming\SqueezePlay when screen is drawn CORRECTLY. (2.78 KB, application/x-zip-compressed)
2010-02-22 10:22 UTC, Mike Richichi
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Klaas 2010-02-22 08:59:14 UTC
Created attachment 6548 [details]
picture of controller with bad screen drawing

This has been reported on the forums, seen in windows desktop squeezeplay. I've
also seen it on controller.

http://forums.slimdevices.com/showthread.php?t=75538

Screen draws over itself resulting in a visually broken display.

Suggest that it may have something to do with recent surface and tile C
changes.
Comment 1 Ben Klaas 2010-02-22 09:04:25 UTC
Created attachment 6549 [details]
movie of behavior

this is a big file so don't know whether it will work, but here's a movie of it...
Comment 2 Mike Richichi 2010-02-22 10:20:38 UTC
Created attachment 6550 [details]
Zip of c:\users\username\AppData\Roaming\SqueezePlay when screen is drawn incorrectly

These configuration settings cause a distorted display within Windows when running the latest SqueezePlay builds.
Comment 3 Mike Richichi 2010-02-22 10:22:53 UTC
Created attachment 6551 [details]
c:\users\username\AppData\Roaming\SqueezePlay when screen is drawn CORRECTLY.  

This profile was created fresh by the latest February 22nd SqueezePlay Windows build.
Comment 4 Mike Richichi 2010-02-22 10:36:05 UTC
Specifically, it looks like SetupWallpaper.lua is the problem.  The file from the old profile causes the behavior.
Comment 5 Ben Klaas 2010-02-22 12:02:42 UTC
I've managed to get the screen on my desktop SP messed up using Mike's SetupWallpaper.lua settings file-- thanks Mike.

Wow, that's interesting...not at all what I would have thought to cause this issue. I'm somewhat skeptical that it's the only way to manifest this, but it's certainly one way.

error log--
20100222 19:32:46.054 ERROR  squeezeplay.ui.draw - jive_tile_load_tiles:490 Can't find image applets/SetupWallpaper/wallpaper/midnight.png

20100222 19:32:51.091 ERROR  squeezeplay.ui.draw - jive_tile_load_tiles:490 Can't find image applets/SetupWallpaper/wallpaper/midnight.png

20100222 19:32:52.452 ERROR  squeezeplay.ui.draw - jive_tile_load_tiles:490 Can't find image applets/SetupWallpaper/wallpaper/midnight.png


Additional info, the settings file that Mike attached for SetupWallpaper is in a format that hasn't been used for about (I'm guessing) a year's time.

I'm going to look into a fix in SetupWallpaper to not try to draw a non-existent file as the background wallpaper, but that will only be a band-aid I think to whatever is causing the lower level drawing error.
Comment 6 Ben Klaas 2010-02-22 12:14:56 UTC
here's the offending bit of code:

    elseif wallpaper then
                -- try firmware wallpaper
                srf = Tile:loadImage(firmwarePrefix .. wallpaper)
        end
        if srf ~= nil then
                Framework:setBackground(srf)
        end

I believe srf should be nil in the case of a bad path to a wallpaper file, and for some reason it is not.

Still investigating, but Michael's checkin r8476 may have exposed this issue...
Comment 7 Ben Klaas 2010-02-22 12:21:05 UTC
in applet:

 srf = Tile:loadImage(firmwarePrefix .. wallpaper)
 log:warn(srf)

in log:

20100222 20:18:58.317 ERROR  squeezeplay.ui.draw - jive_tile_load_tiles:490 Can't find image applets/SetupWallpaper/wallpaper/midnight.png

20100222 20:18:58.317 WARN   applet.SetupWallpaper - SetupWallpaperApplet.lua:398 userdata: 0x16e2e014

srf should be nil for this file, as it does not exist.

I'm pretty sure this would cause problems anywhere Tile:loadImage() is pointing to a bad path, so it should probably be fixed at the C level.

I may put in some defensive code in SetupWallpaperApplet as well, but I think a C fix is necessary here.
Comment 8 Ben Klaas 2010-02-22 13:39:39 UTC
In trying to fix this in SetupWallpaper, I'm trying to put in a check for an existing file path. On desktop, the only success I've had is by coding the relative path for the file like this:

 filepath = '../share/jive/' .. wallpaper

which is probably wrong on the device, and probably wrong for the case when it's looking for a stored image from ImageViewer.

there are three scenarios that need to work:
1. an old deprecated settings style file path, as in Mike's non-working settings
   e.g., "midnight.png"
2. a current settings style for a default wallpaper
   e.g, "applets/SetupWallpaper/wallpaper/fab4_harmony.png"
3. a path to a saved image from ImageViewer
   e.g., "???"

I have not worked out logic that will work for all three of these. Michael, if you have any thoughts on this, please share. Otherwise I think the only fix is to properly return nil from Tile:loadImage() when the file is not present, which is probably necessary anyway.


this patch I was testing with will work for #2 above, and does avert the horrible screen drawing issue, but for #3 scenario will probably result in not finding the image correctly.

=== SetupWallpaperApplet.lua
==================================================================
--- SetupWallpaperApplet.lua    (revision 38726)
+++ SetupWallpaperApplet.lua    (local)
@@ -389,12 +389,21 @@
        elseif wallpaper and string.match(wallpaper, "http://(.*)") then
                -- saved remote image for this player
                srf = Tile:loadImage(downloadPrefix .. playerId:gsub(":", "-"))
-       elseif wallpaper and string.match(wallpaper, "/") then
-               -- saved image from imageviewer
-               srf = Tile:loadImage(wallpaper)
        elseif wallpaper then
-               -- try firmware wallpaper
-               srf = Tile:loadImage(firmwarePrefix .. wallpaper)
+               local filepath
+               if string.match(wallpaper, "/") then
+                       filepath = '../share/jive/' .. wallpaper
+               else
+                       filepath = firmwarePrefix .. wallpaper
+               end
+               -- does filepath exist?
+               if lfs.attributes(filepath, "mode") == 'file' then
+                       log:warn('found: ', filepath)
+                       srf = Tile:loadImage(filepath)
+               else
+                       log:warn('not found: ', filepath)
+                       srf = nil
+               end
        end
        if srf ~= nil then
                Framework:setBackground(srf)
Comment 9 Mick 2010-02-22 23:19:42 UTC
Ben

I've reverted to 8524 which is working fine for me.

I think I'd gone back from 8540, so somewhere in those 16 checkins is the problem.
Comment 10 Alan Young 2010-02-23 00:46:18 UTC
Ben,

Yes, the code does not return NULL if no image can be loaded for a tile. I guess it should.
Comment 11 SVN Bot 2010-02-23 00:58:46 UTC
 == Auto-comment from SVN commit #8548 to the  repo by ayoung ==
 == https://svn.slimdevices.com/?view=revision&revision=8548 ==

bug 15753: Tile:loadImage() should return nil when the file path is non readable 
Return NULL if none of the images for a JiveTile can be found.
Does not test that they are actually loadable.
Comment 12 Alan Young 2010-02-23 00:59:32 UTC
Ben, does this change work for you? Alan.
Comment 13 SVN Bot 2010-02-23 02:34:51 UTC
 == Auto-comment from SVN commit #8552 to the  repo by mherger ==
 == https://svn.slimdevices.com/?view=revision&revision=8552 ==

Bug: 15753
Description: use System:findFile() to expand our partial file paths into valid absolute paths. 

Ben - this should be working with all three of your scenarios. I tested setting them manually in the SetupWallpaper.lua settings file.
Comment 14 Alan Young 2010-02-23 05:36:05 UTC
Ben, do you think that this was also the issue on your controller?
Comment 15 Ben Klaas 2010-02-23 07:37:56 UTC
Yes, I do think that the issue I saw on Controller was the same problem.

I've tested the changes by both Alan and Michael and both are working as desired. Thanks to both of you for the fixes.
Comment 16 Chris Owens 2010-04-08 17:26:38 UTC
This bug has been marked fixed in a released version of Squeezebox Server or the accompanying firmware or mysqueezebox.com release.

If you are still seeing this issue, please let us know!