Rockbox

Tasklist

FS#10258 - pictureflow: emptyslide is displayed if coverimage is based on filename

Attached to Project: Rockbox
Opened by Robert Kukla (roolku) - Thursday, 28 May 2009, 20:06 GMT
Last edited by Andrew Mahone (Unhelpful) - Friday, 29 May 2009, 11:02 GMT
Task Type Bugs
Category Plugins
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

The "emptyslide" image is displayed for coverart where the cover image name is based on filename. This is caused by r21111 and only shows after a pf cache rebuild.

In my opinion this commit is unwarranted as introduces an inconsistency. I have several tracks with filename based cover art which I don't want excluded from pf and will revert the commit for my personal build. Someone else may not be able to do it...
This task depends upon

Closed by  Andrew Mahone (Unhelpful)
Friday, 29 May 2009, 11:02 GMT
Reason for closing:  Fixed
Additional comments about closing:  As of r21126 track art is checked after album art instead, instead of being skipped entirely.
Comment by Thomas Martitz (kugel.) - Friday, 29 May 2009, 05:27 GMT
I think you mean track title based, not filename based.

a) It doesn't make sense for pf to seach by track title, since you can only browse by albums anyway.

b) (more importantly) pf only looked at the first track and took that track's album art. Having each track of an album with his own album art is just *broken* in pf anyway. This leads to more inconsistency IMO. The track title based album art is designed for the wps, where you show 1 track at a time. That just doesn't work for pf.

You should just have a fallback image for the whole album imo. if that's the same as your first track's album art, you'll be fine.
Comment by Steve Bavin (pondlife) - Friday, 29 May 2009, 07:58 GMT
I'd also argue for this to be reverted.

Whilst coverflow is currently a strict selection-by-album, it ought to be usable as a general database browser in the future (e.g. the database would have a <Covers> option slongside <All Tracks> and <Random> when you're at the bottom level.)
Comment by Thomas Martitz (kugel.) - Friday, 29 May 2009, 08:09 GMT
That doesn't work.

If you browse the database using pf, you still browse by albums (other sorting isn't possible in pf right now).

And even if you would browse by covers, pf isn't able to have a cover for every single track (and thus wouldn't index all covers). It just used to pick the <track> cover of the first track of the album and assumes this is the album art for the whole album. That's simply broken.

We could possibly revert it if/when pf is not album strict anymore. But as of now, it's just broken to even consider track specific covers.
Comment by Robert Kukla (roolku) - Friday, 29 May 2009, 08:26 GMT
>I think you mean track title based, not filename based.

"track title based" is not currently supported for albumart. The case that has been disabled in r21111 takes the filename of the audio file, not the title.

a) It doesn't make sense for pf to seach by track title, since you can only browse by albums anyway.

I disagree. I have several incomplete albums where I use filename-based albumart and would like those Albums to show up in pictureflow.

A specific case is where the track contains a whole album (and has a cue sheet). I don't see a reason to exclude those.

>b) (more importantly) pf only looked at the first track and took that track's album art. Having each track of an album with his own album art is just *broken* in pf anyway.

You are right. But the 'incorrect' premise here is that tracks with the same Album meta data have the same AA. To enforce this would be an unnecessary limitation.

> The track title based album art is designed for the wps, where you show 1 track at a time. That just doesn't work for pf.

I disagree. In PF it should show the cover and its associated titles. The problem is that arbitrary Album names are assigned to the cover.

>You should just have a fallback image for the whole album imo. if that's the same as your first track's album art, you'll be fine.

I have 600 tracks with filename based albumart. I think reverting the patch is the more sensible option (for me).

-------

Anyway, the actual bug is that "empty slide" is now displayed for tracks that have album art in the wps. I am sure that will confuse users.

Comment by Steve Bavin (pondlife) - Friday, 29 May 2009, 08:29 GMT
Sorry, I should have been clearer - I meant that would be a sensible way to go in the future.

Sounds like the real problem is the assumption that track 1 art = album art,

r21111 just moves the problem case from (a) somebody has different art for track 1 than for the album to (b) track-specific art is ignored. I remain unconvinced.
Comment by Steve Bavin (pondlife) - Friday, 29 May 2009, 08:31 GMT
Ah, reading Robert's comment (which came in while I was typing), perhaps it's a priority thing - i.e. the album art should be prioritised over track art when dealing with albums?
Comment by Thomas Martitz (kugel.) - Friday, 29 May 2009, 08:45 GMT
Robert, I think the expectations have you from your naming scheme is more broken than pictureflow. Why would you expect an arbitrary *track/file specific* cover art to work for the whole album?

If you have only 1 of 10 track in an album supplied with a album art, then the WPS won't show album art for the other 9 tracks. That makes sense. Pictureflow however showed the art for all files of the album (wps and pf behave differently anyway).
Even worse, if that cover was not for the first track, but for the 2nd or so, then even the old pictureflow wouldn't show anything.
-- That's what you call consistent?

Filename based album art is just not designed for albums. Pitctureflow now respects that.

Singlefile albums with cue sheets are surely a special case. But the other cover possibilities remain, such as <album>.<ext> which would make just sense if you really want to have the cover for the whole file.


>> "track title based" is not currently supported for albumart. The case that has been disabled in r21111 takes the filename of the audio file, not the title.
Sorry, I was under the (wrong) impression that it was based on the track metadata, not the filename.
Comment by Robert Kukla (roolku) - Friday, 29 May 2009, 08:51 GMT
>Sounds like the real problem is the assumption that track 1 art = album art,

Yes. The user can enforce it (this is what I do) but doesn't have to. It will just an arbitrary (the first?) cover in pf which I find acceptable.

>r21111 just moves the problem case from (a) somebody has different art for track 1 than for the album to (b) track-specific art is ignored.

Yes and there are still various other ways to have different cover images for tracks from one album. Removing just one of these isn't a solution in my opinion.

>Ah, reading Robert's comment (which came in while I was typing), perhaps it's a priority thing - i.e. the album art should be prioritised over track art when dealing with albums?

I don't think this would work due to the many possible ways album art can be assigned. Remember the tracks of an album don't need to be in a single directory.

---
If anybody wants to continue with http://www.rockbox.org/tracker/task/8295 for a proper solution: I have an updated patch that doesn't have the memory overhead by using dircache (as suggested by slasheri). Needs to be synced and debugged... Not sure I can find the time myself in the near future.


Comment by Thomas Martitz (kugel.) - Friday, 29 May 2009, 09:01 GMT
>>ii.e. the album art should be prioritised over track art when dealing with albums?

That's what I actually had in mind when making the suggestion to Unhelpful. He committed it with just ignoring filename. But that's fine too, IMO.
Comment by Robert Kukla (roolku) - Friday, 29 May 2009, 09:40 GMT
>Robert, I think the expectations have you from your naming scheme is more broken than pictureflow.

Why? I have a tool that extracts embedded album art and saves it as a jpg with the same base name as the audio file. I don't have a tool that does the same and saves it with the album (taking to into account the translations required for rockbox). I don't always keep the whole album on my player and do store tracks from different albums in the same directory.

Yes, you can argue I shouldn't but in my opinion rockbox should give flexibility and not impose restrictions.

>Why would you expect an arbitrary *track/file specific* cover art to work for the whole album?

Because the arbitrary image looks the same as all the other images (this is for me, the user to enforce). And in several cases there is only one track per album so there is no problem for me.

>If you have only 1 of 10 track in an album supplied with a album art, then the WPS won't show album art for the other 9 tracks. That makes sense. Pictureflow however showed the art for all files of the album (wps and pf behave differently anyway).
>Even worse, if that cover was not for the first track, but for the 2nd or so, then even the old pictureflow wouldn't show anything.

You can cause the same problem (1 track with, 9tracks without) by having tracks of the album in different directories and using album-based cover art. Are you going to stop searching for album based cover art as well?

>-- That's what you call consistent?

Consistent with what? With my understanding on how picture flow works - yes.

>Filename based album art is just not designed for albums. Pitctureflow now respects that.

Unless you enforce a 1:1 relationship between album meta data and album art image you will only be able to work around special cases and while doing so making it harder to understand how it works and giving the user less flexibility to achieve their goal.

>Singlefile albums with cue sheets are surely a special case. But the other cover possibilities remain, such as <album>.<ext> which would make just sense if you really want to have the cover for the whole file.

Yes, at the price of additional effort - i.e. less flexible.
It also makes it harder to find the associated image for file management - e.g. when I delete an audiobook I have finished, I want to delete the cover as well and it is handy to have the image sorted next to the audio file.

I believe the problematic issue is that picture flow assumes 1:1 relationship between album meta data and album art image. Solution would be to drop the browse by album title and purely browse by cover art (FS#8295) or to enforce this 1:1 relationship. Your solution only singles out one issue that "in certain cases may be" problematic and disables it.

As a 'side effect' it leads to the display of the "empty slide" image in pictureflow for tracks that have (filename based) album art in the wps which in my mind is the real "bug" of that report.

Loading...