Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#8836 - Short pcmbuf_beep() (AKA Keyclick) doesn't work properly on PortalPlayer targets

Attached to Project: Rockbox
Opened by Steve Bavin (pondlife) - Tuesday, 01 April 2008, 12:51 GMT+2
Last edited by Steve Bavin (pondlife) - Thursday, 11 December 2008, 09:10 GMT+2
Task Type Bugs
Category Music playback
Status Closed
Assigned To No-one
Player Type PortalPlayer-based
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Version 3.1
Due Date Undecided
Percent Complete 100%
Private No

Details

The attached patch revises pcmbuf_beep() in an attempt to kill the beeeeep of death that occurs on PortalPlayer targets when keyclick is enabled.

With this patch, on c200, the unpleasent beep seems to have gone, but the keyclick isn't very reliable:
- Before playback is started, the keyclick has an extra click (probably when pcm playback begins)
- While playback is in progress, the keyclick sounds correct, but is delayed (could the 1/8th second delay be removed?)
- After playback then STOP, the very first keyclick works, but subsequent ones are almost inaudible.

I've no idea why, but if the un-needed "memset(bufstart, 0, samples * 4);" is removed, then the keyclick after playback rarely sounds at all. I guess it's something to do with memory caching, but am not a low-level expert.

Also, it would be good if the pcmbuf static variable audiobuffer could be renamed, to avoid potential confusion with the audiobuffer declared in buffer.c.
   pcmbufbeep_simpler.patch (5.7 KiB) 
 apps/pcmbuf.c |  114 +++++++++++++++++++++++++++-------------------------------
 1 file changed, 54 insertions(+), 60 deletions(-)

This task depends upon

Closed by  Steve Bavin (pondlife)
Thursday, 11 December 2008, 09:10 GMT+2
Reason for closing:  Fixed
Additional comments about closing:  No need to disable the option now. Thanks, jhMikeS!
Comment by Steve Bavin (pondlife) - Wednesday, 02 April 2008, 08:14 GMT+2
Of course, the bug itself probably lies in the lower level PCM/DMA code. If it were in pcmbuf.c then I'd expect similar problems on all SWCODEC targets.
Comment by Steve Bavin (pondlife) - Thursday, 03 April 2008, 16:26 GMT+2
I've committed the above patch, minus the un-needed memset (and corresponding comment). Seems to kill the problem on PP, but sadly also the keyclick. H300 works fine with the simpler code.

I'll change this back to a bug report for some PP guru to look at.
Comment by Marc Guay (Marc_Guay) - Monday, 07 July 2008, 22:01 GMT+2
Enabling voice and keyclick and then jumping around the menus very quickly will make the beep of death happen eventually.
Comment by Nathan Myers (ncm) - Tuesday, 30 September 2008, 04:39 GMT+2
Duplicate #9430 - crash when keyclick on, multiple button presses
Comment by Nathan Myers (ncm) - Tuesday, 30 September 2008, 04:45 GMT+2
Shouldn't the keyclick setting option be disabled on players where it doesn't work?
Comment by Marc Guay (Marc_Guay) - Tuesday, 30 September 2008, 13:01 GMT+2
There were plans to disable it in v3.0 (for PP players) but apparently it was forgotten.
Comment by Nathan Myers (ncm) - Wednesday, 01 October 2008, 01:58 GMT+2
Isn't that what a tracking system is for? I have entered #9432.
Comment by Nathan Myers (ncm) - Wednesday, 01 October 2008, 05:22 GMT+2
That was quick; Llorean up and closed #9432. So much for using the tracking system so plans wouldn't be forgotten. Me, I would have suggested putting it on the "Roadmap" instead of closing it.
Comment by Nathan Myers (ncm) - Wednesday, 01 October 2008, 09:17 GMT+2
I have attached a patch to make the bug self-documenting.
   ppkeyclickbusted.diff (1.5 KiB) 
 apps/settings_list.c |   19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comment by Steve Bavin (pondlife) - Wednesday, 01 October 2008, 09:47 GMT+2
That's not the right solution (and it breaks translation). Better just to disable the feature on PP (as per the attached patch), although if that's committed the underlying problem will probably never be fixed.

The correct solution is, of course, to fix the PP implementation of pcmbuf_beep()...
   noppkeyclick.patch (3 KiB) 
 apps/action.c              |    4 +++-
 apps/settings.h            |    4 +++-
 apps/menus/settings_menu.c |    8 ++++++--
 apps/settings_list.c       |    2 ++
 4 files changed, 14 insertions(+), 4 deletions(-)

Comment by Nathan Myers (ncm) - Wednesday, 01 October 2008, 21:00 GMT+2
Here's another try at fixing the keyclick settings without, this time, breaking translation. It seems less intrusive than noppkeyclick.patch, and leaves open the possibility that the underlying bug might someday be fixed.
Comment by Nathan Myers (ncm) - Wednesday, 01 October 2008, 21:01 GMT+2
Sorry, that patch:
   ppkeyclickbusted2.diff (1.4 KiB) 
 apps/settings_list.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Comment by Nathan Myers (ncm) - Wednesday, 01 October 2008, 21:43 GMT+2
Sorry, I suppose that should be:
   ppkeyclickbusted3.diff (1.4 KiB) 
 apps/settings_list.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Comment by Steve Bavin (pondlife) - Thursday, 02 October 2008, 09:53 GMT+2
Erm, that patch just removes the options (apart from "off"), right? So the keyclick still can't be anabled, but there's a useless menu option left behind.

I maintain that the correct solution is to find out why the problem occurs in PP pcm playback, and then fix it. Could it be that the buffer length has to be a minimum number of samples, or a multiple of 2^n bytes? If so, a workaround may be just to mix appropriate silence after the beep. I don't have a working PP target, so can't test this here.

And if we can't fix it, then the keyclick should just be removed as per my patch (IMHO).
Comment by Nathan Myers (ncm) - Thursday, 02 October 2008, 11:38 GMT+2
The point is that, until it's fixed, the present menu options are a nasty trap for anybody who doesn't know that turning them on will make playback crash on random button presses. Of course the underlying bug should be fixed, but in the meantime we shouldn't be tricking people into turning on something that has never worked correctly.

We leave the menu option in place because the manual mentions it. Anybody curious why the other options are missing will find this FS task and be enlightened, and maybe motivated to fix the underlying bug. When the underlying bug gets fixed, then we turn the menu options back on just by removing the #if/#else.
Comment by Steve Bavin (pondlife) - Thursday, 02 October 2008, 12:06 GMT+2
The manual should be kept in sync, and also suppress keyclick for PP targets. That should be doable, but I'll need to ask a manual guru on how to do it.
Comment by Nathan Myers (ncm) - Thursday, 02 October 2008, 23:31 GMT+2
All it need is a one-line note that keyclick is not yet supported on PP targets. It doesn't need anything tricky.
Comment by Michael Sevakis (MikeS) - Thursday, 11 December 2008, 02:52 GMT+2
The PCM bug has been fixed but are all issues addressed here now? If so, please close.

Loading...