Opened 11 years ago

Closed 4 months ago

#1503 closed enhancement (fixed)

MediaPlayer doesn't remember previous volume setting

Reported by: scottmc Owned by: stippi
Priority: normal Milestone: R1
Component: Applications/MediaPlayer Version: R1/Development
Keywords: Cc: redeye4@…
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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.

Attachments (4)

mediaplayer-volume-memory.diff (4.3 KB) - added by engleek 9 years ago.
First efforts at addressing this enhancement.
0001-Mediaplayer-resume-at-latest-position-and-restore-vo.patch (12.9 KB) - added by frizer23 5 years ago.
0001-MediaPlayer-remember-position-and-volume.patch (14.2 KB) - added by frizer23 4 years ago.
0001-Mediaplayer-remember-position-and-volume.patch (17.9 KB) - added by frizer23 4 years ago.

Download all attachments as: .zip

Change History (37)

comment:1 Changed 11 years ago by leavengood

Owner: changed from marcusoverhagen to leavengood
Status: newassigned

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.

comment:2 in reply to:  1 Changed 9 years ago by engleek

Cc: redeye4@… added

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.

comment:3 Changed 9 years ago by 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.

comment:4 Changed 9 years ago by stippi

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

Changed 9 years ago by engleek

First efforts at addressing this enhancement.

comment:5 Changed 9 years ago by 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)

comment:6 Changed 9 years ago by 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.)

comment:7 in reply to:  6 Changed 9 years ago by 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 :)

comment:8 Changed 7 years ago by axeld

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

comment:9 Changed 7 years ago by leavengood

Owner: changed from leavengood to nobody
Status: in-progressassigned

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.

comment:10 Changed 7 years ago by 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

comment:11 Changed 6 years ago by harvard

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

comment:12 Changed 6 years ago by harvard

Problem is valid.MediaPlayer doesn't remember previous volume setting in haikuR1/Alpha 4.1.

Last edited 6 years ago by harvard (previous) (diff)

comment:13 Changed 5 years ago by frizer23

Has a Patch: set

comment:14 Changed 5 years ago by 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.

comment:15 Changed 5 years ago by anevilyak

Owner: changed from nobody to stippi

comment:16 Changed 4 years ago by modeenf

anything we can comit?

comment:17 Changed 4 years ago by 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?

comment:18 Changed 4 years ago by waddlesplash

Owner: changed from stippi to waddlesplash
Version: R1/pre-alpha1R1/Development

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

comment:19 Changed 4 years ago by 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.

comment:20 Changed 4 years ago by 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).

comment:21 Changed 4 years ago by 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.

comment:22 Changed 4 years ago by frizer23

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

comment:23 Changed 4 years ago by pulkomandy

Owner: changed from waddlesplash to stippi

comment:24 Changed 3 years ago by Barrett

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

comment:25 Changed 3 years ago by 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.

comment:26 Changed 3 years ago by Barrett

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

comment:27 Changed 3 years ago by 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.

comment:28 Changed 21 months ago by digib0y

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

comment:29 Changed 20 months ago by axeld

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

comment:30 Changed 20 months ago by 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.

comment:31 Changed 4 months ago by pulkomandy

comment:32 Changed 4 months ago by pulkomandy

Has a Patch: unset

comment:33 Changed 4 months ago by waddlesplash

Resolution: fixed
Status: assignedclosed

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.

Note: See TracTickets for help on using tickets.