MediaPlayer: Cover image / artwork

[shinta]

Like iPod or Walkman, I want MediaPlayer show thumbnail image (artwork) on its display view when it plays music file.

Music format independent mechanism is to show foo.jpg/png when MediaPlayer plays foo.mp3/wav.

Additionally, artwork is embedded in mp3's ID3V2 tag.

---

​SHINTA (Enhancement No.9)

[shinta]

Artwork implemented

[shinta]

Above screenshot is MediaPlayer with artwork feature.

MediaPlayer is playing SnowTears.mp3 with artwork (SnowTears.jpg) shown.

[shinta]

This is the patch to implement artwork feature.

[shinta]

Sorry, new files are lacked. I added.

[kallisti5]

Thanks for this!

The patch looks clean after a quick scan. If no one applies this in a few days i'll review / apply.

[phoudoin]

The feature looks fine, indeed.

I didn't have time to review deeper the patch yet, but I don't like the introduction of std::vector along BList. It should be consistent, and as BList was used for others lists of stuffs by FilePlayListItem and MediaFileTrackSupplier classes, I would rather like to use it too for new lists introduced by this feature.

[shinta]

Thanks for comments.

Please teach me why Haiku's apps don't use STL std::vector. BList retains void pointer, so,

• BList needs elements' new/delete: maybe cause memory leaks.
• BList needs reinterpret_cast: maybe cause unexpected error that can't be checked by compiler.

I think std::vector is easy to use and safety.

[stippi]

The patch looks good to me. Some small coding style violations but nothing too serious. I think the playlist item binding turned out nicely. Thanks a lot for your work on media player! Using std vector is fine, I think.

[phoudoin]

Replying to shinta:

I think std::vector is easy to use and safety.

I agree, and I'm not against using std:vector in itself. I was just pointing that mixing in same class both BList and STL's vector containers seems a bit weird and unconsistent. Nothing more, nothing less.

I may have express my point in a bit harsh way, though. Sorry.

To amend myself, I'll commit the patch soon ;-)

[phoudoin]

Applied in hrev41271, with some small code style changes and gcc4 support (using std::vector; added).

[shinta]

Thank you for applying!

Thanks to playlist idea, many media can be treated in same framework. Thank you!

[humdinger]

Nice additions! I was wondering... Often there are not coverart jpegs corresponding to the filename of every track, but one jpeg corresponding to the album title. Would it make sense to fall back to the ID3 or attribute album title named jpeg if there isn't a 100% match filename.mp3<->filename.jpg?

[stippi]

BTW, I just realized that some handling of music versus video clips in MediaPlayer should be broken now. When album art is present, MediaPlayer will consider the clip as video now. For example, this will break the window placement and automatic playlist saving features which depends on the clip type.

[shinta]

I confirmed applying my patch by haiku-nightly-hrev41272-x86gcc2hybrid-vmware. Thanks again.

Replying to humdinger:

Would it make sense to fall back to the ID3 or attribute album title named jpeg if there isn't a 100% match filename.mp3<->filename.jpg?

Do you mean below?

When there is a album titled "AAAAA", which has songs "001.mp3", "002.mp3"...

When I play 001.mp3, MediaPlayer usually show 001.jpg. But, if there isn't 001.jpg, MediaPlayer shows AAAAA.jpg.

I think it is a good idea.

Replying to stippi:

BTW, I just realized that some handling of music versus video clips in MediaPlayer should be broken now. When album art is present, MediaPlayer will consider the clip as video now. For example, this will break the window placement and automatic playlist saving features which depends on the clip type.

I can't understand English 100%, but, if MediaPlayer's behavior is different from music and video, there may be some things to be solved.

[humdinger]

Replying to shinta:

When there is a album titled "AAAAA", which has songs "001.mp3", "002.mp3"... When I play 001.mp3, MediaPlayer usually show 001.jpg. But, if there isn't 001.jpg, MediaPlayer shows AAAAA.jpg.

Exactly.

[shinta]

Replying to stippi:

BTW, I just realized that some handling of music versus video clips in MediaPlayer should be broken now. When album art is present, MediaPlayer will consider the clip as video now. For example, this will break the window placement and automatic playlist saving features which depends on the clip type.

Is your mention related to MainWin::GetQuitMessage()?

	if (!fHasVideo && fHasAudio) {
		// store playlist, current index and position if this is audio
		BMessage playlistArchive;

		BAutolock controllerLocker(fController);
		playlistArchive.AddInt64("position", fController->TimePosition());
		controllerLocker.Unlock();

		if (!fPlaylist)
			return;

		BAutolock playlistLocker(fPlaylist);
		if (fPlaylist->Archive(&playlistArchive) != B_OK
			|| playlistArchive.AddInt32("index",
				fPlaylist->CurrentItemIndex()) != B_OK
			|| message->AddMessage("playlist", &playlistArchive) != B_OK) {
			fprintf(stderr, "Failed to store current playlist.\n");
		}
	}

I wonder why current index and position is saved only if audio.

[axeld]

Replying to stippi:

BTW, I just realized that some handling of music versus video clips in MediaPlayer should be broken now. When album art is present, MediaPlayer will consider the clip as video now. For example, this will break the window placement and automatic playlist saving features which depends on the clip type.

FWIW, that one only rarely worked for me, anyway, but I never bothered to look into it. So it won't be a huge loss ;-)

[axeld]

Replying to humdinger:

Replying to shinta:

When there is a album titled "AAAAA", which has songs "001.mp3", "002.mp3"... When I play 001.mp3, MediaPlayer usually show 001.jpg. But, if there isn't 001.jpg, MediaPlayer shows AAAAA.jpg.

Exactly.

Furthermore, it might also be beneficial to detect albums, and add eventual images in that folder as well - and this can be named almost arbitrarily, like "cover.jpg", or "Album name - front.jpg" or something like that.

In any case, I hope the feature is optional.