Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#7430 closed enhancement (fixed)

MediaPlayer: Cover image / artwork

Reported by: shinta Owned by: stippi
Priority: normal Milestone: R1
Component: Applications/MediaPlayer Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

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)

Attachments (4)

BL_DI_20110409.png (41.9 KB ) - added by shinta 9 years ago.
Artwork implemented
mediaplayer_artwork.diff (26.2 KB ) - added by shinta 9 years ago.
This is the patch to implement artwork feature.
ImageTrackVideoSupplier.cpp (2.8 KB ) - added by shinta 9 years ago.
ImageTrackVideoSupplier.h (1.6 KB ) - added by shinta 9 years ago.

Download all attachments as: .zip

Change History (22)

by shinta, 9 years ago

Attachment: BL_DI_20110409.png added

Artwork implemented

comment:1 by shinta, 9 years ago

Above screenshot is MediaPlayer with artwork feature.

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

by shinta, 9 years ago

Attachment: mediaplayer_artwork.diff added

This is the patch to implement artwork feature.

comment:2 by shinta, 9 years ago

Has a Patch: set

by shinta, 9 years ago

Attachment: ImageTrackVideoSupplier.cpp added

by shinta, 9 years ago

Attachment: ImageTrackVideoSupplier.h added

comment:3 by shinta, 9 years ago

Sorry, new files are lacked. I added.

comment:4 by kallisti5, 9 years ago

Version: R1/alpha2R1/Development

Thanks for this!

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

comment:5 by phoudoin, 9 years ago

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.

comment:6 by shinta, 9 years ago

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.

comment:7 by stippi, 9 years ago

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.

in reply to:  6 comment:8 by phoudoin, 9 years ago

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 ;-)

comment:9 by phoudoin, 9 years ago

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

comment:10 by phoudoin, 9 years ago

Resolution: fixed
Status: newclosed

comment:11 by shinta, 9 years ago

Thank you for applying!

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

comment:12 by humdinger, 9 years ago

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?

comment:13 by stippi, 9 years ago

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.

in reply to:  13 ; comment:14 by shinta, 9 years ago

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.

in reply to:  14 ; comment:15 by humdinger, 9 years ago

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.

in reply to:  13 comment:16 by shinta, 9 years ago

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.

in reply to:  13 comment:17 by axeld, 9 years ago

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 ;-)

in reply to:  15 comment:18 by axeld, 9 years ago

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.

Note: See TracTickets for help on using tickets.