Rockbox

Tasklist

FS#6814 - Mpegplayer volume changer

Attached to Project: Rockbox
Opened by Jacob Gardner (cmptrgy412) - Wednesday, 14 March 2007, 00:28 GMT
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

The mpegplayer plugin did not have the functionality for volume changing. I fixed that by putting the following code in the button switch(...):

case BUTTON_SCROLL_UP:
case BUTTON_SCROLL_DOWN:


vol = rb->global_settings->volume;
minvol = rb->sound_min(SOUND_VOLUME);
maxvol = rb->sound_max(SOUND_VOLUME);

if(button == BUTTON_SCROLL_UP)
vol -= vol_interval; // This seems backwards, but it works... could someone explain.
else
vol += vol_interval;

if( vol > maxvol )
vol = maxvol;
else if( vol < minvol )
vol = minvol;

if( vol != rb->global_settings->volume ) {
rb->sound_set( SOUND_VOLUME, vol );
rb->global_settings->volume = vol;

}

break;

This task depends upon

Closed by  Dave Chapman (linuxstb)
Sunday, 18 March 2007, 17:07 GMT
Reason for closing:  Accepted
Additional comments about closing:  A modified version of mpegplayer-volume_control-02.diff committed - thanks to both Jacob and Pascal.
Comment by Jacob Gardner (cmptrgy412) - Wednesday, 14 March 2007, 00:41 GMT
I guess I should add that this was only tested on my Sansa e200.
Comment by Hepdog (007quick) - Wednesday, 14 March 2007, 04:08 GMT
Hmm, tried to patch it but after going through all the-p0 -p1... none of them worked! I do have other patches but none of them that envole mmpeg and it didn't even get to the HUNK FAILED #1 of 2 or w/e it is. are you sure that it patches?
Comment by Jacob Gardner (cmptrgy412) - Wednesday, 14 March 2007, 11:00 GMT
Yes, I just checked, but using the tortoise svn patch program...
I don't know if that's different from the patch program in cygwin. If so, I can create one in there, it just seemed easier at them time to use tortoise.
Comment by Nils Wallménius (nls) - Wednesday, 14 March 2007, 11:29 GMT
the patch is made in the mpegplayer directory, you'll need to place it there to apply it or point patch to apps/plugins/mpegplayer/mpegplayer.c
Comment by Hepdog (007quick) - Wednesday, 14 March 2007, 15:09 GMT
Ok, so it patches fine now when you put it in that specific directory, but next time one should make it so that you can leave it in the root of the source. But anywyas. I have a H10 5g and the volume works fine except that it is backwards. having volume capabilities is great but for some reason it is backwards. I will be watching this and will gladly test for you.
Comment by Geoff Stokes (iwantanimac) - Thursday, 15 March 2007, 07:29 GMT
Thanks for this. I can't believe they didn't bother with it in the first place...
Comment by Sacha (Angyman) - Thursday, 15 March 2007, 09:48 GMT
if(button == BUTTON_SCROLL_UP)
vol -= vol_interval;
else
vol += vol_interval;

Shouldnt it be like that:?
vol += vol_interval;
else
vol -= vol_interval;


Comment by Jacob Gardner (cmptrgy412) - Friday, 16 March 2007, 01:15 GMT
Sacha and Hepdog: Meh...
On the Sansa E200 series it worked the other way, thus the comment of mine about it being backwards. When I get home on Sunday, I'll try to add compatibility for more devices.

nls: Yes, sorry, my first patch, I'll use the patch program in cygwin next time instead of tortoise svn.
Comment by Pascal Briehl (ColdSphinX) - Friday, 16 March 2007, 17:10 GMT
How about this:
[code]/* button definitions */
#if (CONFIG_KEYPAD == IRIVER_H100_PAD) || (CONFIG_KEYPAD == IRIVER_H300_PAD)
#define MPEG_MENU BUTTON_MODE
#define MPEG_STOP BUTTON_OFF
#define MPEG_PAUSE BUTTON_ON
#define MPEG_VOL_DOWN BUTTON_DOWN
#define MPEG_VOL_UP BUTTON_UP

#elif (CONFIG_KEYPAD == IPOD_3G_PAD) || (CONFIG_KEYPAD == IPOD_4G_PAD)
#define MPEG_MENU BUTTON_MENU
#define MPEG_PAUSE (BUTTON_PLAY | BUTTON_REL)
#define MPEG_STOP (BUTTON_PLAY | BUTTON_REPEAT)
//#define MPEG_VOL_DOWN BUTTON_DOWN
//#define MPEG_VOL_UP BUTTON_UP

#elif CONFIG_KEYPAD == IAUDIO_X5_PAD
#define MPEG_MENU (BUTTON_REC | BUTTON_REL)
#define MPEG_STOP BUTTON_POWER
#define MPEG_PAUSE BUTTON_PLAY
#define MPEG_VOL_DOWN BUTTON_DOWN
#define MPEG_VOL_UP BUTTON_UP

#elif CONFIG_KEYPAD == GIGABEAT_PAD
#define MPEG_MENU BUTTON_MENU
#define MPEG_STOP BUTTON_A
#define MPEG_PAUSE BUTTON_SELECT
#define MPEG_VOL_DOWN BUTTON_DOWN
#define MPEG_VOL_UP BUTTON_UP

#elif CONFIG_KEYPAD == IRIVER_H10_PAD
#define MPEG_MENU (BUTTON_REW | BUTTON_REL)
#define MPEG_STOP BUTTON_POWER
#define MPEG_PAUSE BUTTON_PLAY
#define MPEG_VOL_DOWN BUTTON_SCROLL_DOWN
#define MPEG_VOL_UP BUTTON_SCROLL_UP

#elif CONFIG_KEYPAD == SANSA_E200_PAD
#define MPEG_MENU BUTTON_SELECT
#define MPEG_STOP BUTTON_POWER
#define MPEG_PAUSE BUTTON_UP
#define MPEG_VOL_DOWN BUTTON_SCROLL_UP
#define MPEG_VOL_UP BUTTON_SCROLL_DOWN

#else
#error MPEGPLAYER: Unsupported keypad
#endif[/code]
[...]
[code] switch (button)
{
#if defined(MPEG_VOL_UP) && defined(MPEG_VOL_DOWN)
case MPEG_VOL_UP:
case MPEG_VOL_DOWN:
vol = rb->global_settings->volume;
minvol = rb->sound_min(SOUND_VOLUME);
maxvol = rb->sound_max(SOUND_VOLUME);

if (button == MPEG_VOL_DOWN)
vol -= vol_interval;
else
vol += vol_interval;

if ( vol > maxvol )
vol = maxvol;
else if( vol < minvol )
vol = minvol;

if ( vol != rb->global_settings->volume ) {
rb->sound_set( SOUND_VOLUME, vol );
rb->global_settings->volume = vol;
}
break;
#endif
case MPEG_MENU:
[/code]
Comment by Hepdog (007quick) - Friday, 16 March 2007, 23:00 GMT
Just for the record the patch is out of sync!
Comment by Jacob Gardner (cmptrgy412) - Saturday, 17 March 2007, 02:30 GMT
Pacal: Thanks, I'll put it in on sunday, when I get home, or you can do it if you like

Hepdog: It's my first patch.
Comment by Hepdog (007quick) - Saturday, 17 March 2007, 05:34 GMT
Oh ya. No problem. I actually reported 4 different patches that went out of sync all at the same time!
Comment by Pascal Briehl (ColdSphinX) - Saturday, 17 March 2007, 11:26 GMT
I only have tested it on my x5, but the h10 and the sansa part should work.
The keymapping for the other players should work, but for the IPOD_3G_PAD and IPOD_4G_PAD I have no idea.
If the keymapping is ready the "#if defined(MPEG_VOL_UP) && defined(MPEG_VOL_DOWN)" can be removed.
Comment by Pascal Briehl (ColdSphinX) - Saturday, 17 March 2007, 13:53 GMT
latest SVN:
IAUDIO_X5_PAD changed to IAUDIO_X5M5_PAD
Comment by Dave Chapman (linuxstb) - Sunday, 18 March 2007, 12:13 GMT
If you want this committed to SVN, please post a proper patch. If you checked out the code from svn, you just need to do:

svn diff apps/plugins/mpegplayer/mpegplayer.c > mpegplayer-volcontrol.diff

and then attach that .diff file to a comment.

For more information, see: http://www.rockbox.org/twiki/bin/view/Main/WorkingWithPatches
Comment by Pascal Briehl (ColdSphinX) - Sunday, 18 March 2007, 12:59 GMT
I had to remove some other patches, but there it is.
Comment by Dave Chapman (linuxstb) - Sunday, 18 March 2007, 14:29 GMT
Thanks for the patch. A few comments:

1) Does it not work for ipods? The appropriate key mappings should be BUTTON_SCROLL_FWD to increase the volume, and BUTTON_SCROLL_BACK to decrease it (see the WPS definitions in keymap-ipod.c)

2) The patch doesn't appear to handle repeat events - it should check for MPEG_VOL_UP|BUTTON_REPEAT as a well as simply MPEG_VOL_UP

3) Looking at keymap-gigabeat.c, there are two sets of buttons mapped to volume changing in the WPS screen (which is what I think mpegplayer should mimic) - the normal UP/DOWN buttons, plus the dedicated VOL_UP and VOL_DOWN buttons.

Comment by Pascal Briehl (ColdSphinX) - Sunday, 18 March 2007, 15:47 GMT
now iPod 3G & iPod 4G are included.
I only have an iAudio x5, so please test.
Comment by Dave Chapman (linuxstb) - Sunday, 18 March 2007, 16:34 GMT
OK, thanks. I'll fix up all the other issues and commit.
Comment by Pascal Briehl (ColdSphinX) - Sunday, 18 March 2007, 16:52 GMT
now it also displays the Volume and botton_repeat for every target.

Loading...