D21076: ICC Color Correction Effect

zzag requested changes to this revision.
zzag added a subscriber: graesslin.
zzag added a comment.
This revision now requires changes to proceed.


  I'm not sure about inclusion because:
  
  (a) (currently) this effect doesn't work correctly on multi screen setups
  (b) there is a policy about inclusion of new effects. Maybe @graesslin could weigh in (it may take some time to get response though)
  
  I don't see any problems with shipping this effect as a third party binary effect (out-of-source tree).

INLINE COMMENTS

> icc.cpp:21-31
> +#include "icc.h"
> +#include "iccconfig.h"
> +#include "kwinglplatform.h"
> +
> +#include "lcms2.h"
> +
> +#include <QAction>

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Includes

> icc.cpp:33
> +
> +static const int LUT_POINTS = 64;
> +

Is it a good idea to hardcode this value though?

> icc.cpp:39-41
> +    :   m_valid(false),
> +        m_shader(NULL),
> +        m_texture(0)

- Use `nullptr` instead `NULL`
- Prefer default member initialization

> icc.cpp:49-52
> +    if (m_shader)
> +        delete m_shader;
> +    if (m_texture != 0)
> +        glDeleteTextures(1, &m_texture);

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Braces

> icc.cpp:110
> +
> +std::vector<uint8_t> ICCEffect::makeCLUT(const char* source_icc, const char* target_icc)
> +{

- Use QVector.
- Use camelCase not snake_case :-)

> icc.cpp:112-113
> +{
> +    std::vector<uint8_t> clut, clut_source(LUT_POINTS*LUT_POINTS*LUT_POINTS*3);
> +    cmsHPROFILE source, target;
> +    cmsHTRANSFORM transform;

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Variable_Declarations

> icc.cpp:127
> +        goto free_target;
> +    for (int b = 0, addr = 0; b < LUT_POINTS; b++) {
> +        for (int g = 0; g < LUT_POINTS; g++) {

Post increment in a for loop is a bug, even if it's an int.

> icc.cpp:130-132
> +                clut_source[addr] = 255*r/(LUT_POINTS-1);
> +                clut_source[addr+1] = 255*g/(LUT_POINTS-1);
> +                clut_source[addr+2] = 255*b/(LUT_POINTS-1);

Just do

  clut_source[addr++] = 255 * r / (LUT_POINTS - 1);
  clut_source[addr++] = 255 * g / (LUT_POINTS - 1);
  clut_source[addr++] = 255 * b / (LUT_POINTS - 1);

and remove addr += 3 in the for loop.

(also, I wonder whether we could use reciprocals here, just a thought)

> icc.cpp:139-144
> +free_target:
> +    cmsCloseProfile(target);
> +free_source:
> +    cmsCloseProfile(source);
> +free_nothing:
> +    return std::move(clut);

Please don't do this in C++. It would be perfectly fine C code, but not C++. Use a QScopedPointer with custom deleter instead.

> icc.cpp:176
> +{
> +    if (m_valid) {
> +        glActiveTexture(GL_TEXTURE3);

You don't need to check whether m_valid is true. drawWindow() won't be called if isActive() returns false.

> icc.cpp:211
> +
> +} // namespace

`// namespace KWin`

(though namespace comments is a bit controversial topic in KWin)

> icc.h:24-25
> +
> +#include <kwineffects.h>
> +#include "kwinglutils.h"
> +

Why one is <filename> and the other is "filename"?

> icc.h:35
> + * cLUT is generated from a pair of ICC profiles using LCMS2
> + */
> +class ICCEffect

In KWin, doxygen comments have to terminate with `**/`.

> icc.h:36-37
> + */
> +class ICCEffect
> +    : public Effect
> +{

You're following the old coding style. Instead it should look like

  class ICCEffect : public Effect
  {

> icc.h:42
> +    ICCEffect();
> +    ~ICCEffect();
> +

~ICCEffect() override;

> icc.h:45
> +    void reconfigure(ReconfigureFlags flags) override;
> +    void drawWindow(EffectWindow* w, int mask, QRegion region, WindowPaintData& data) override;
> +    void paintEffectFrame(KWin::EffectFrame* frame, QRegion region, double opacity, double frameOpacity) override;

https://techbase.kde.org/Policies/Frameworks_Coding_Style#Whitespace

> icc.h:47
> +    void paintEffectFrame(KWin::EffectFrame* frame, QRegion region, double opacity, double frameOpacity) override;
> +    bool isActive() const;
> +

Missing override keyword.

> icc.h:53
> +
> +public Q_SLOTS:
> +

Delete it.

> icc.h:55
> +
> +protected:
> +    bool loadData();

Huh?

> icc.h:65
> +    GLuint m_texture;
> +    std::vector<uint8_t> m_clut;
> +

I don't see why we need it. This field is used only in makeclut.

> icc.h:67-68
> +
> +    std::vector<uint8_t> makeCLUT(const char* source_icc, const char* target_icc);
> +    GLuint setupCCTexture(uint8_t *clut);
> +};

I don't see why these two have to be methods. Both can be static functions in the cpp file.

> icc_config.cpp:53
> +{
> +    // Undo unsaved changes
> +}

Huh?

> icc_config.cpp:78-79
> +}
> +
> +
> +} // namespace

Too many empty lines.

> icc_config.h:34
> +public:
> +    explicit ICCEffectConfig(QWidget* parent = 0, const QVariantList& args = QVariantList());
> +    ~ICCEffectConfig();

Use nullptr.

REPOSITORY
  R108 KWin

REVISION DETAIL
  https://phabricator.kde.org/D21076

To: vitaliyf, zzag
Cc: graesslin, davidedmundson, anthonyfieroni, zzag, ngraham, kwin, jraleigh, GB_2, mkulinski, ragreen, jackyalcine, Pitel, iodelay, bwowk, ZrenBot, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, jensreuterberg, abetts, sebas, apol, mart