Created attachment 187 [details]
merge cacheEntries from CUE sheets

Here is the patch I spoke of.

adding cc to michael, because I recall he was the man behind the CUE sheet code.

well, when I apply the patch attached above to a clean cvs checkout, slimserver
no longer starts.

$ ./slimserver.pl 
Global symbol "%tempCacheEntry" requires explicit package name at
/usr/src/tmp/slim/server/Slim/Music/Info.pm line 2084.
BEGIN not safe after errors--compilation aborted at
/usr/src/tmp/slim/server/Slim/Music/Info.pm line 2290.
Compilation failed in require at /usr/src/tmp/slim/server/Slim/Utils/Misc.pm
line 14.
BEGIN failed--compilation aborted at /usr/src/tmp/slim/server/Slim/Utils/Misc.pm
line 14.
Compilation failed in require at ./slimserver.pl line 105.
BEGIN failed--compilation aborted at ./slimserver.pl line 105.

Created attachment 189 [details]
uses proper hash reference

sorry, wrong version.  this should be less crash-prone

well, the new patch doesn't seem to break anything new. Unfortunately, it
doesn't seem to fix "Browse Artwork" either. That is, even with this patch in
place, Browse Artwork doesn't show the pictures that "Browse Albums" does.

unfortunately, I cannot reproduce the setup to really test this. nor can I
reproduce the problem with what I do have.  I made one test case, and it did
work.  if you can find some time to create a small sample of flac/cue/art files
and do a wipe cache with d_artwork and/or d_info enabled, that might help
isolate what is going on.  I think the proper way forward is certainly along the
lines of this patch.  The updateCache calls should be made from as few locations
as possible, at least until the SQL backend can be brought in.

I'm seeing a number of issues with displaying artwork (both with or without this
patch). I'm going to throw some more variety into my test set to see if I can't
narrow down cuesheet related issues from general ones, and post a summary of
what I find.

sounds good what this patch was trying to do was to allow Info.pm to handle the
updateCache, and readCoverArt.  The parseCUE should send its cache info to
readTags which can then merge the data in the proper way. 

Created attachment 204 [details]
this is better

that last patch was not referencing the hashes quite correctly so they didn't
merge.	I was using the wrong folder for testing so it looked lke it worked
when it didn't.  I looked at the contents of the cache now and this does seem
to pull in the relevant elements as provided by the cue sheet and allows
artwork to show.  Please give this a shot and see if this helps with any of
your flac w/cue variations.  The only one i have is a single flac with an
external sheet.

still no luck with Browse Artwork.
I see the artwork if I navigate to the album, I see the art, but Browse Artwork
still shows only the placeholder (or nothing at all if I have placeholders
turned off). 

this should work.  disregard anything with the browse album Those are read on
the fly.  only the browse by artwork is cached, thus you also need to wipe the
cache.   please look at the output of d_artwork if you get time because I'd
really like to see a snippit of that output on a track that should have artwork
show up. the curiority is killing me :)

ok, here's some more data.

I'm testing this with a recent cvs update that has the above patch applied. For
testing purposes, I have it set to "Don't Cache" the "Music Library Database"
but I also hit "Wipe Cache" just for good measure. I have the "Artwork" variable
in Interface settings set to "%ARTIST - ALBUM" since that's how I usually set it
on the real server, but I've tested this patch with that variable cleared (it's
default) as well just for throughness (no effect). I'm testing with two albums,
one using the ARTIST - ALBUM.jpg picture, and one using cover.jpg (the second is
a various artists album, so the ARTIST - ALBUM.jpg style doesn't work anyway.)

The attachment was generated by using -d_artwork. After the initial directory
scan, I selected "Browse Artwork" and saw no pictures. I then checked each of
the albums, which do show artwork individually. (I know you said it was a
different mechinism, but I mention it here because it's relevent to the output
attached.) I went back to "Browse Artwork" and still no pictures appear.

One interesting thing I noted while using -d_artwork, is that output is
generated when the initial scan happens, and when browsing each album, but when
"browse artwork" is selected, no output from -d_artwork occurs. I don't know if
that's normal or not, but it seemed suspicious.

Anyway, here's the output you requested. Let me know if I can help further.

Created attachment 211 [details]
output using -d_artwork and the above patch

Thanks Michael!  I think there is one thing to try, based on what I can see in
teh log.  The log shows that the server is finding the 'variable cover'
(Marillion - Clutching at Straws.jpg for eg).  however, it then does not find a
thumb image. If you do use this for cover art, and want those to appear in
browse artwork, you need to put the variable name in both Artowrk, and Artwork
Thumbnail settings.  it is the thumbnail that appears in the browse by artwork.

Blair Witch project should be showing an image, as teh debug says: "Found thumb
file: /var/audio/art-test/The Blair Witch Project/cover.jpg"

You likely dont see d_artwork output during the browse by artwork listing
because the process is drawn from the cache.  The cache stores each album name,
matched with the filepath to the artwork file found during scan. the cache
stores 1 if the images is embedded.  d_info should show the request for artwork
in that case, as we grab the thumb image from the infocache. d_info is a bit
chatty so I didn't want to run that one just yet.

Ok, I miss understood how thumbnails were handled. I was under the impression
that a missing thumbnail would cause the "full size" image to be used, and thus
only needed to be specified if you had a seperate, pre-scalled thumbnail image
available.

So, adding %ARTIST - ALBUM to the thumbnail artwork setting caused an
interesting effect. The art is recognized by slimserver as a thumbnail entry, as
seen from -d_artwork...

2004-11-29 15:04:42.9718 Variable Thumbnail: Marillion - Clutching at Straws.jpg
from ARTIST - ALBUM

2004-11-29 15:04:42.9723 Found thumb file:
/var/audio/art-test/Marillion/Marillion - Clutching at Straws.jpg

and when I "Browse by Artwork" I still don't see any covers, but the placeholder
link for Marillion is gone. In it's place is a blank space that has the alt text
of the album name. Peering at the frame source, I see it's trying to link an
image that doesn't exist.

img
src="/music/%2Fvar%2Faudio%2Fart-test%2FMarillion%2FMarillion%20-%20Clutching%20at%20Straws.cue.flac/thumb.jpg"
border="0" height="100" width="100" alt="Clutching at Straws" title="Clutching
at Straws" 

so, it recognizes that it now has a thumbnail image, but doesn't seem to be able
to link it into the rendered page correctly.

On the other hand, adding thumb.jpg to the Blair Witch Project (in addition to
the existing cover.jpg) ...

2004-11-29 15:04:43.7210 Found thumb file: /var/audio/art-test/The Blair Witch
Project/thumb.jpg

didn't seem to have any effect at all. I still get the placeholder link for
Blair Witch in the Browse Artwork page.

The artwork settings, perhaps, are a bit confusing.  cover.jpg, thumb.jpg,
ambumartsmall.jpg, and folder.jpg are all valid filenames, automatically
searched, for thumbnail AND cover art.  This is also valid as gif files. The
only time you need the Artwork and Artwork Thumbnail settings is if you use a
filename that isn't automatic, or you wish to use the variables.  In this case,
having %ARTIST - ALBUM for both woudl allow the variable name to be found. 
cover.jpg shoudl be found for blair witch for both artwork and thumbnail.  The
difference in teh automatic files is that for cover art, the large files are
looked for first, and for thumbnail the smaller files are looked for first. 
Thus you could have a cover.jpg of 300x300 and a thumb.jpg of 100x100 and you'd
see the cover.jpb when browsing album, and the thumb.jpg when browsing artwork.

When you get the blank image, what happens if you right click and choose view
image? Sometimes when artwork is new vs not found, the browser seems to cache a
blank. Well, that's been by guess.  They always turn up later.  I think we've
got the search part working.  The actual rendering seems to be the current hurdle.

Proposed new string for Artwork Thumbnail description:

	EN	For Browse By Artwork thumbnails, the server will search the same default
image files as above, and in ID3 Tags.  However, if you wish to use the variable
filename feature, or specify your own filename, enter that here.

ahh, I had the standard file algorithym down but didn't realize it changed for
the variable name version. that makes sense.

right clicking and selecting "view image" shows me a blank page with the url of
the non-existant image file. the problem seems to be that the wrong file name is
being inserted into the img tag. In the example above, for the music file it
should be looking for "Marillion - Clutching at Straws.jpg"
instead it's taking the name of the flac file and appending "/thumb.jpg" to the
end, and attempting to render "Marillion - Clutching at Straws.cue.flac/thumb.jpg"
which of course doesn't exist.

I've thrown an mp3 in there as well, with it's own cover.jpg, just to make sure
that "Browse Artwork" in general is working. It's picked up just fine, and used
as a thumbnail on the browse artwork page, right next to the placeholder link
for the Blair Witch Project, whose own cover.jpg (or thumb.jpg) still isn't
being recognized by the "Browse Artwork" function. 

after a bit more fiddling...
dropping the artwork variable and going back to just using cover.jpg doesn't
make the Marillion artwork drop back to placeholder style in "Browse Artwork",
instead it keeps the blank-space-linked-to-wrong-image display.

dragging in a couple other sample files, I've found that the general behavour
most closely resembles the behaviour of Marillion in the previous tests, and not
The Blair Witch Project. The latter behaves as if it's thumbnail is never found,
regardless of artwork variable or the existance of cover.jpg and/or thumb.jpg
(browse album shows art fine, just browse artwork doesn't). for now I'm content
that it's a problem with this particular album, and probably comes down to a
differnt bug than we're chasing here. (I suspect that perhaps the apostrophe in
the album name might not be getting escaped correctly/consistantly, but that's
just a guess at this point.)

which means that when combined with the above revision of the text for the
thumbnail artwork variable description and the patch that started this, the only
remaining issues is that for cuesheet flacs, the url of the thumbnail image is
incorrectly constructed for the "browse artwork" page.

ok, looking at the working mp3 image, I see that filename/thumb.jpg is some sort
of virtual url that gets replied with the actual file by slimserver. In which
case, the url constructions for the flac files is correct, but what ever bits of
slimserver that lookup as valid file from it aren't working as expected.

i was about to mention that :)

I think this patch at least solves the artwork scan, but I agree something else
is going on here.  I have this sample working with my setup:
http://192.168.1.22:9000/music/%2Fmnt%2Fmp3%2FAlternate%2FFLAC%2FCUE%2FU2_How_To_Dismantle_An_Atomic_Bomb.flac/thumb.jpg

and I get the image, which uses cover.jpg as the real file.  I'll try a variable
name and see how it goes in case that isn't working.  Otherwise, there must be
something mismatched in the infocache.  In cases where the album name is the
same as artist name, you might end up with a placeholder if you use different
case formatting.  This is a known bug that affects browse by album as well.  It
may be that we have something similar for punctuation. 

thanks for taking all this time to run tests.

I tried renaming the jpg to ARTIST - ALBUM.jpg for a quick test.  I got the
right artwork for browse artwork, but also got a placeholder for 'no album'
which linked to the whole flac file.  the link under the artwork went to the
list of songs marked by the external cue sheet.  is yours an internal CUE?  is
it normal for CUE files to be listed as well?

michael, if you are using MusicMagic, please turn this off and wipe cache.  I
saw a case of the blank image from a flac w/ cue sheet once I had enabled
musicmagic.  The musicmagic support does not currently support artwork so I
believe it may be interfering.

> if you are using MusicMagic, please turn this off and wipe cache. 

nope, I only discovered MusicMagic yesterday. I haven't played with it enough
yet to consider bringing it anywhere near my slimserver.

> is yours an internal CUE?  is it normal for CUE files to be listed as well?

slimserver still has a fair ammount of trouble dealing multiple songs in a
single file, or multiple files that refer to the same piece of music. Here we're
dealing with both.
I typically use Flac files with embedded (internal) cuesheets. I do keep a flac
file with an external cuesheet around for testing, though I've never tried it
specifically with artwork before.

whipping out my external cuesheet flac test and throwing artwork at it, I
discovered the same thing you did, that slimserver includes both the cuesheet
results (as the correct album entry) and a placeholder for the raw flac as well
(as "No Album"). This is consistant with or without the patch that started this
thread, so I'd say leave that as a seperate issue.

I also discovered one more thing while testing the external cuesheet flac. 
Like some people on the internet, I tend to differentiate my flac files with
embedded cuesheets by naming them title.cue.flac, (in the spirit of file.tar.gz)
and those that have external cuesheets or represent a single song as title.flac.
When I was messing around with the external cuesheet flac, I got it to display
artwork. Then just for the sake of thourough testing, I took the same file and
made both an external cuesheet version and an embedded cuesheet version.
I stumbled across this ...
testfile.flac (with external cuesheet) will show artwork.
testfile.cue.flac (with external cuesheet, despite the name) will show up blank
on the browse artwork page. (not placeholder, just no image like I mentioned in
a previous comment.)
Unfortunately, the flac files with embedded cuesheets still won't show artwork
no matter how I name them. (they still show the blank entry either way.)

I'm not sure if this brings us any closer to an answer, or just confuses things
more. 

oddly, I think it does make some sense.  The artwork cache stores the path of
the artwork file (or 1 for embedded) in a hash, keyed by album title.  These two
flac woudl have same album title, but different locations, just the artwork link
might not be valid for one of them. Are they both stored in the same directory?
Perhaps, the virtual path that the server is creating for all these variants
might also be affecting things.  I'd need to figure out how to make an internal
cue sheet to test.

> Are they both stored in the same directory?

if you mean the working (by that I mean displays artwork on the browse artwork
page) testfile.flac and the non-working testfile.cue.flac, it's actually the
same file, so they're not both active at once (and thus not conflicting).
something like this...

mv testfile.flac testfile.cue.flac
(edit testfile.cue so it's pointing to the right filename)
(start slimserver, wipe cache, note that it's not displaying art for that album)
mv testfile.cue.flac testfile.flac
(edit testfile.cue so it's pointing to the right filename)
(start slimserver, wipe cache, note that art is again displayed)

> I'd need to figure out how to make an internal cue sheet to test.

sucking the cuesheet into the flac file is the easy part...
metaflac --import-cuesheet-from=testfile.cue testfile.flac
mv testfile.cue /some/where/slim/wont/find/it/

now you just need to make sure your tags are in a form slim will understand.

That sample file you let me download was the trick!

I have the answer...well, at least the problem.  test.flac seems to get cached
with 'fec' as content type.  as such, it returns nothing from Slim::Info::isSong
so no artwork can be retrieved as the artwork only returns when isSong returns a
type (ie, types.conf says its audio).

is there a problem with marking fec as 'audio' type in types.conf?

Created attachment 212 [details]
make fec 'audiolist' type

this modifies the types for fec to audiolist, and I've changed isSong to work
when the type matches /audio/ instead of eq 'audio' so that fec will qualify as
audio AND list.  I dont use these types, so I'm hoping this has no ill effects.
 However, it DOES now show artwork for test.flac and test.cue.flac when using
an artwork file in the same folder. in my case, I had to clear the browser
cache as well as wipe cache after applying the fix. This must be applied on top
of the previous patch ( attachment 204 [details])

another step in the right direction.

applying the patch makes all of my cover.jpg entries show up (after a browser
refresh), regardless of where the cuesheet lives. unfortunately, the 
%ARTIST - ALBUM style covers are still showing up blank.

I do like the audiolist idea. 
The first issue with it that I noticed is that the "no album" entry went from
listing the cuesheet (for the test album with the external cuesheet) to listing
all the flacs.  I tried making a change to isPlaylist similar to what you did
for isSong, but without much success. (using  =~ "list" doesn't work as it will
match things we don't intend. using =~ "playlist" and changing the fec type to
"audio-and-playlist" didn't work either, the flac files still appeared under "no
album". Haven't had much chance yet to dig further than that.) 

anyway, it's certainly a step in the right direction.

Created attachment 219 [details]
hopefully the next step

I found out that the raw .flac file was being stored to the artworkcache. This
was  a two-stage problem, because of the initial scan, then a call to
reBuildCaches erases the artworkCache and rewrites.  I'm not sure we should be
doing that for the artwork cache but I'd like to know what results we got from
this latest patch before I go after that.  This patch is in addition to the
previous ones.

on recent inspection, it looks like the modification to types.conf is not needed
once teh most recent patch is done.  

I think you've got it.

This works for nearly anythnig I can think to throw at it.

I still like the idea of the audiolist type, but for now it doesn't seem
necessary for handling artwork, and without it the "no album" entry only
contains external cuesheets. Probably better to put that change aside for the
moment.

I say commit this and call it fixed.

Phew, that's good to know!

attachment 204 [details] is already committed.  212 is not needed.  Vidur, could 219 be
considered for 5.4.1?  

marked for 6.0.0 only.

Routine bug db maintenance; removing old versions which cause confusion.  I apologize for the inconvenience.