Bug 301311 - JJ: Pre-amplifier in equalizer doesn't work
Summary: JJ: Pre-amplifier in equalizer doesn't work
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Unclassified
Component: Tools/Equalizer (show other bugs)
Version: 2.6.90 (2.7 beta)
Platform: Archlinux Packages Linux
: NOR normal with 21 votes (vote)
Target Milestone: 2.8
Assignee: Amarok Developers
URL:
Keywords: junior-jobs
Depends on:
Blocks:
 
Reported: 2012-06-06 20:45 UTC by bartek
Modified: 2013-04-15 20:09 UTC (History)
6 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.8


Attachments
Equalizer snapshot (56.56 KB, image/png)
2013-02-16 06:53 UTC, aitch_nu
Details

Note You need to log in before you can comment on or make changes to this bug.
Description bartek 2012-06-06 20:45:48 UTC
In amarok in equalizer changing position of the first slider doesn't change the sound. I believe it should be pre-amplifier, but taking in consideration this bug https://bugs.kde.org/show_bug.cgi?id=280318 it is not clear.

When I used xine-backend for phonon (with I don't know why is unmaintained) I didn't had this problem. Also in other backends for phonon there no such  problem, because there's no equalizer at all... That's why I believe the gstreamer-backend is to blame.

Reproducible: Always

Steps to Reproduce:
1. Run amarok
2. Play some file
3. Open equalizer
4. Change position of first slider. No change in sound should be noticed.
Comment 1 Torrie Fischer 2012-09-23 09:30:47 UTC
The GStreamer equalizer actually does not have a preamp. Amarok shouldn't be displaying one as phonon-gstreamer does not report the existance of one.

Reassigning.
Comment 2 bartek 2012-09-25 05:53:53 UTC
If  GStreamer does not have a preamp, so how it works in Clementine? It uses gstreamer and it has good working preamp.
Comment 3 Matěj Laitl 2012-11-16 17:20:40 UTC
Correct component.
Comment 4 Myriam Schweingruber 2012-11-27 15:20:01 UTC
(In reply to comment #2)
> If  GStreamer does not have a preamp, so how it works in Clementine? It uses
> gstreamer and it has good working preamp.

Because Clementine uses gstreamer directly, not Phonon.

Which exact Amarok version is this about? Please make sure you sue Amarok 2.6 and report back.
Comment 5 Myriam Schweingruber 2013-01-11 12:12:37 UTC
Closing for lack of feedback.
Comment 6 Matěj Laitl 2013-01-11 12:24:51 UTC
Oh, I can confirm and reproduce this bug, reopening.

The problem is interface between EngineController and Equalizer, which is so silly that it cannot communicate non-presence of the preamp, also the Equalizer GUI must be adapted to be able to disable gray-out preamp. Shouldn't be hard to do, marking as JJ.
Comment 7 aitch_nu 2013-02-03 16:38:13 UTC
how should I check which Effect Parameter is supported by phonon or should I just hard code it for preamp ?
Comment 8 aitch_nu 2013-02-15 15:33:56 UTC
Can I assume that if at all preamp is present then it will the first element of Effect Parameter list ??
Comment 9 Matěj Laitl 2013-02-15 15:37:53 UTC
(In reply to comment #8)
> Can I assume that if at all preamp is present then it will the first element
> of Effect Parameter list ??

Yes, in the first version of the patch.
Comment 10 aitch_nu 2013-02-16 06:53:43 UTC
Created attachment 77354 [details]
Equalizer snapshot

Is this the desired output ? ( Preamp is disabled in the image though the effect is not porperly visible )
Additional Change :  Fixed top and bottom labels of first slider. Earlier band name label was at the top and slider value label was at the bottom for first slider ( assuming it was a bug ).
Comment 11 Matěj Laitl 2013-02-16 17:37:42 UTC
(In reply to comment #10)
> Created attachment 77354 [details]
> Equalizer snapshot
> 
> Is this the desired output ? ( Preamp is disabled in the image though the
> effect is not porperly visible )

Yep, looks good.

> Additional Change :  Fixed top and bottom labels of first slider. Earlier
> band name label was at the top and slider value label was at the bottom for
> first slider ( assuming it was a bug ).

I think it was intended, but confusing, so this is a good change.

Now it's time for http://reviewboard.kde.org/ - please do attach the screenshot there too.
Comment 12 Matěj Laitl 2013-04-15 20:09:00 UTC
Git commit e344f608faf236e8faa455e0516f55d4ffeee80c by Matěj Laitl, on behalf of Harsh Gupta.
Committed on 15/04/2013 at 15:55.
Pushed by laitl into branch 'master'.

EqualizerDialog: disable pre-amplifier slider if it isn't supported

BUGFIXES:
 * Pre-apmlifier in equalizer is now only enabled if it is actually
   supported; patch by Harsh Gupta.
FIXED-IN: 2.8
REVIEW: 108995
DIGEST: Amarok now disables pre-amp slider if it isn't available

M  +2    -0    ChangeLog
M  +13   -7    src/EngineController.cpp
M  +2    -0    src/EngineController.h
M  +7    -3    src/dialogs/EqualizerDialog.cpp
M  +4    -4    src/dialogs/EqualizerDialog.ui

http://commits.kde.org/amarok/e344f608faf236e8faa455e0516f55d4ffeee80c