MediaPlayer doesn't remember previous volume setting

[scottmc]

I opened a wav file copied from a CD, and MediaPlayer opened up with the volume set to about 50%, this was very loud on my laptop so I reduced it to about 10%. I closed it and then opened a different wav file and it was at 50% again. I was going to reboot into BeOS to check how it was there but having issues getting BeOS to boot at the moment so couldn't check it. This might be app related or media prefs related, i'm not sure which keeps track of previous volume settings or if that's even an option, but it "should" be if it's not. I'm marking it as an enhancement for now.

[leavengood]

You are right that MediaPlayer does not save the volume setting.

Since there can be multiple MediaPlayer windows I think we need a per file volume setting, which I figure could be stored as an attribute of the files in question. So I did some research and indeed the R5 media player stores the volume setting (and quite a few other settings) as attributes of the media file:

Int-32 4 be:mediaPlayer_miniMode

0x52454354 16 be:mediaPlayer_frame

Float 4 be:mediaPlayer_volume

Int-64 8 be:mediaPlayer_inpoint Int-64 8 be:mediaPlayer_outpoint

Bool 1 be:mediaPlayer_dropFrames

So I think we need to do something similar.

[engleek]

After a bit of reading through the Media Kit section in the Be Book, I reckon that:

• PlaylistItem could be changed to add volume as an available attribute.
• Controler could be changed to check PlaylistItem for stored volume when FileChanged is called, and store volume when FileFinished is called.

This is only a first idea, I wonder if this is the most elegant solution.

I worry about the fact that the volume is a float value, but that PlaylistItem stores numbers only as int64, and that a PlaylistItem isn't necessarily a file.

[stippi]

Your observations are correct. The PlaylistItem should provide a virtual interface for retrieving and setting a persistent volume. The default implementation would be empty. Only the FilePlaylistItem would then implement it via node attributes.

[stippi]

Forgot to mention that the type of the value, float or integer is free to chose.

[engleek]

First efforts at addressing this enhancement.

[engleek]

The global structure seems okay to me (pay no attention to the printf's).

I thought it more strategic to act when the PlaylistItem changed.

This is non-functional, which I think is due to the actual attribute write (it seemed like it should have worked, no luck).

Also, I should probably use the Attribute structure for the actual attribute string.

My apologies for not following a couple of the style rules (80 chars again)

[stippi]

I believe the Controller has the volume as float between 0 and 1. So using int64 as attribute type and converting it via (int) will not work. I don't understand the first two hunks of the patch. Why do you need fLastPlaylistItem at all?

Restoring the volume from the attribute when the playlist item is opened is correct. However, you also need to actually store the volume when it changes. Here is what I think about volume on items: The user has a very easy way to change the global volume of the system. Changing the volume in MediaPlayer could work as an adjustment, i.e. when the volume is not properly normalized in the file. Or with quite loud files where the waveform has a lot of energy across many frequency bands. With these things in mind, I believe the MediaPlayer should apply the volume of the previous song to the next song, but only as long as it's still the same album (i.e. within the same folder on disk). The right place to add this knowledge would be the Playlist, IMHO.

One last remark... if you indeed need to reference a PlaylistItem pointer, it needs to be a reference. The code directly above your first hunk shows how to do that. This is important, since PlaylistItems are refernce counted, across different threads even. (PlaylistWindow and MainWin are two threads messing with the Playlist.)

[engleek]

I agree with storing volume as a float attribute, when I started I followed the methods that were available in PlaylistItem for some reason.

As for fLastPlaylistItem, my solution involved doing the loading and storing at the same moment, so when an item is loaded, it stores the volume in the last item, then restores the volume from the new item. I needed a place in the code from which could access the player volume and the playlist, and which could react to item change.

At the point I chose, when the Message was received, the item had already changed, so in order to save the volume to the last played item I chose to store reference. A more elegant solution would be to let the Playlist deal with it's own volume data, but I don't presently know how I it could nicely access the Controller volume in both directions.

If I understand your thoughts correctly, the stored value wouldn't really be a volume, rather a factor to reduce volume on loud items and raise it for quiet items. Also if media player doesn't find an volume info, it first looks for nearby items with the same album data, logically starting with the last item (for example).

Thanks for the reference remark, I'll keep that in mind :)

[axeld]

Ryan, are you still working on this one? ;-)

[leavengood]

Actually it is funny you ask since I was about to start auditing my older tickets and removing the ones I'm not working on (since I've recently added a bunch of new ones I have more hope of working on.)

The short answer is no :)

I just never thought there was a very elegant fix to this bug, though I'll admit not looking very closely at engleek's patch.

Assigning to nobody.

[halilpk]

Haiku revision: hrev42211 still a bug. When you start a media file volume is always 50%. And there was no settings about it. System: Haiku hrev1-alpha3 on VMware workstation 8 on windows 7 32 bit

[harvard]

Problem is valid.MediaPlayer doesn't remember previous volume setting.

[harvard]

Problem is valid.MediaPlayer doesn't remember previous volume setting.

[frizer23]

This patch is functionnal here, but I would appreciate some feedback.

Mediaplayer now remembers volume setting, but keeps current volume when play files from the playlist. I also implemented a resume feature and associated options. Again, it starts from the begining of the file if it is loaded from the playlist.

[modeenf]

anything we can comit?

[pulkomandy]

Style violations:

• 2 blank lines between methods
• Missing space between if and (
• Missing space after =

Functionality:

• The BAlert for resuming isn't locale-safe. Use BString.SetToFormat() and B_TRANSLATE().
• I don't understand why the 50* and /50 on volume. Wouldn't it be simpler if the volume was stored as a float between 0 and 1 everywhere?

[waddlesplash]

This looks easy enough; if the original author can't do it I'll be happy to.

[stippi]

Thanks for looking into it! I believe the frame property for the resume feature (which would be much appreciated) needs to be stored as int64. Otherwise, the patch just has some minor coding style issues, but I haven't looked very closely.

[frizer23]

There you go ;) Please tell me if there is any problem with this (it compiles but I couldn't test the changes since last patch).

[korli]

Thanks for the patch! Here are some coding issues:

• two blank lines between methods.
• space before and after operators (/ % = ...), line 704, 714, 716 in Controller.cpp
• line 692 in Controller.cpp: two tabs instead of one.
• line 717 in Controller.cpp: 80 characters length.
• FilePlaylistItem.cpp: multiline blocks require braces.

[frizer23]

I fixed coding issues in the last patch. It still applies nicely.

[Barrett]

Is there some reason for not applying this? The patch looks like OK to me.

[pulkomandy]

Probably because no one reviewed the last version yet?

I see room for improvement (like using BDurationFormat instead of custom code to format the duration), but maybe we should apply it first and see about that later.

[Barrett]

I would apply it then when time permits (if someone don't do it).

[Barrett]

Ok, then I tested the patch. The functionality is here but I'm not convinced by the pop-up appearing and asking to resume the previous play time, so this behavior should be disabled by default to avoid annoying the user. Another problem is that when the file is played from playlist, those settings should be ignored.

[digib0y]

The frame property for the resume feature has already been implemented. Am I wrong?

[axeld]

I don't think so, why do you? :-)

[axeld]

The MediaPlayer only stores the position of the current playlist for audio files; but that's part of the playlist, not the audio file.

[pulkomandy]

Patch migrated to Gerrit: ​https://review.haiku-os.org/#/c/haiku/+/327

[waddlesplash]

Merged in hrev ... or, well, looks like the post-commit hook that adds hrevs broke. It's merged in ​https://git.haiku-os.org/haiku/commit/?id=3248de3de47011137e0c667a7247dd424c827bd7 though.