From 14e7afcb4ea930839931cfa4aeb1b1b1b9f658b6 Mon Sep 17 00:00:00 2001 From: Xaver Hugl Date: Tue, 26 Apr 2022 12:32:20 +0200 Subject: [PATCH] color management: refactor and move to its own directory The pipeline stages are also now properly managed, which should prevent use-after-free errors. BUG: 453026 --- CMakeLists.txt | 1 + src/CMakeLists.txt | 8 +-- src/backends/drm/drm_output.h | 1 - src/backends/drm/drm_pipeline.h | 2 +- src/backends/x11/standalone/x11_output.cpp | 2 +- src/{ => colors}/colordevice.cpp | 55 ++++--------------- src/{ => colors}/colordevice.h | 0 src/{colors.cpp => colors/colorlut.cpp} | 26 +-------- src/{colors.h => colors/colorlut.h} | 15 +----- src/{ => colors}/colormanager.cpp | 0 src/{ => colors}/colormanager.h | 0 src/colors/colorpipelinestage.cpp | 49 +++++++++++++++++ src/colors/colorpipelinestage.h | 33 ++++++++++++ src/colors/colortransformation.cpp | 61 ++++++++++++++++++++++ src/colors/colortransformation.h | 41 +++++++++++++++ 15 files changed, 206 insertions(+), 88 deletions(-) rename src/{ => colors}/colordevice.cpp (83%) rename src/{ => colors}/colordevice.h (100%) rename src/{colors.cpp => colors/colorlut.cpp} (63%) rename src/{colors.h => colors/colorlut.h} (67%) rename src/{ => colors}/colormanager.cpp (100%) rename src/{ => colors}/colormanager.h (100%) create mode 100644 src/colors/colorpipelinestage.cpp create mode 100644 src/colors/colorpipelinestage.h create mode 100644 src/colors/colortransformation.cpp create mode 100644 src/colors/colortransformation.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 70d1387882..0980510ba9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -417,6 +417,7 @@ include_directories(BEFORE ${CMAKE_CURRENT_SOURCE_DIR}/src/effects ${CMAKE_CURRENT_SOURCE_DIR}/src/tabbox ${CMAKE_CURRENT_SOURCE_DIR}/src/platformsupport + ${CMAKE_CURRENT_SOURCE_DIR}/src/colors ) if (KF5DocTools_FOUND) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 565a64a99a..704324af91 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -31,9 +31,11 @@ target_sources(kwin PRIVATE appmenu.cpp atoms.cpp client_machine.cpp - colordevice.cpp - colormanager.cpp - colors.cpp + colors/colordevice.cpp + colors/colorlut.cpp + colors/colormanager.cpp + colors/colorpipelinestage.cpp + colors/colortransformation.cpp composite.cpp cursor.cpp cursordelegate_opengl.cpp diff --git a/src/backends/drm/drm_output.h b/src/backends/drm/drm_output.h index 031ce05a9c..885581d0af 100644 --- a/src/backends/drm/drm_output.h +++ b/src/backends/drm/drm_output.h @@ -9,7 +9,6 @@ #ifndef KWIN_DRM_OUTPUT_H #define KWIN_DRM_OUTPUT_H -#include "colors.h" #include "drm_abstract_output.h" #include "drm_object.h" #include "drm_object_plane.h" diff --git a/src/backends/drm/drm_pipeline.h b/src/backends/drm/drm_pipeline.h index a2d27f9763..d4e8e55876 100644 --- a/src/backends/drm/drm_pipeline.h +++ b/src/backends/drm/drm_pipeline.h @@ -17,7 +17,7 @@ #include #include -#include "colors.h" +#include "colorlut.h" #include "drm_object_plane.h" #include "output.h" #include "renderloop_p.h" diff --git a/src/backends/x11/standalone/x11_output.cpp b/src/backends/x11/standalone/x11_output.cpp index 4669a2faa4..0c1c68d760 100644 --- a/src/backends/x11/standalone/x11_output.cpp +++ b/src/backends/x11/standalone/x11_output.cpp @@ -7,7 +7,7 @@ SPDX-License-Identifier: GPL-2.0-or-later */ #include "x11_output.h" -#include "colors.h" +#include "colorlut.h" #include "main.h" namespace KWin diff --git a/src/colordevice.cpp b/src/colors/colordevice.cpp similarity index 83% rename from src/colordevice.cpp rename to src/colors/colordevice.cpp index ca929b59a8..9190483105 100644 --- a/src/colordevice.cpp +++ b/src/colors/colordevice.cpp @@ -5,7 +5,8 @@ */ #include "colordevice.h" -#include "colors.h" +#include "colorpipelinestage.h" +#include "colortransformation.h" #include "output.h" #include "utils/common.h" @@ -24,17 +25,6 @@ struct CmsDeleter; template using CmsScopedPointer = QScopedPointer>; -template<> -struct CmsDeleter -{ - static inline void cleanup(cmsStage *stage) - { - if (stage) { - cmsStageFree(stage); - } - } -}; - template<> struct CmsDeleter { @@ -69,21 +59,15 @@ public: uint brightness = 100; uint temperature = 6500; - CmsScopedPointer temperatureStage; - CmsScopedPointer brightnessStage; - CmsScopedPointer calibrationStage; + QSharedPointer temperatureStage; + QSharedPointer brightnessStage; + QSharedPointer calibrationStage; QSharedPointer transformation; }; void ColorDevicePrivate::rebuildPipeline() { - cmsPipeline *pipeline = cmsPipelineAlloc(nullptr, 3, 3); - if (!pipeline) { - qCWarning(KWIN_CORE) << "Failed to allocate cmsPipeline!"; - return; - } - if (dirtyCurves & DirtyCalibrationToneCurve) { updateCalibrationToneCurves(); } @@ -93,25 +77,12 @@ void ColorDevicePrivate::rebuildPipeline() if (dirtyCurves & DirtyTemperatureToneCurve) { updateTemperatureToneCurves(); } - dirtyCurves = DirtyToneCurves(); - if (calibrationStage) { - if (!cmsPipelineInsertStage(pipeline, cmsAT_END, calibrationStage.data())) { - qCWarning(KWIN_CORE) << "Failed to insert the color calibration pipeline stage"; - } + const auto tmp = QSharedPointer::create(QVector{calibrationStage, brightnessStage, temperatureStage}); + if (tmp->valid()) { + transformation = tmp; } - if (temperatureStage) { - if (!cmsPipelineInsertStage(pipeline, cmsAT_END, temperatureStage.data())) { - qCWarning(KWIN_CORE) << "Failed to insert the color temperature pipeline stage"; - } - } - if (brightnessStage) { - if (!cmsPipelineInsertStage(pipeline, cmsAT_END, brightnessStage.data())) { - qCWarning(KWIN_CORE) << "Failed to insert the color brightness pipeline stage"; - } - } - transformation = QSharedPointer::create(pipeline); } static qreal interpolate(qreal a, qreal b, qreal blendFactor) @@ -164,7 +135,7 @@ void ColorDevicePrivate::updateTemperatureToneCurves() // The ownership of the tone curves will be moved to the pipeline stage. cmsToneCurve *toneCurves[] = {redCurve.take(), greenCurve.take(), blueCurve.take()}; - temperatureStage.reset(cmsStageAllocToneCurves(nullptr, 3, toneCurves)); + temperatureStage = QSharedPointer::create(cmsStageAllocToneCurves(nullptr, 3, toneCurves)); if (!temperatureStage) { qCWarning(KWIN_CORE) << "Failed to create the color temperature pipeline stage"; } @@ -201,7 +172,7 @@ void ColorDevicePrivate::updateBrightnessToneCurves() // The ownership of the tone curves will be moved to the pipeline stage. cmsToneCurve *toneCurves[] = {redCurve.take(), greenCurve.take(), blueCurve.take()}; - brightnessStage.reset(cmsStageAllocToneCurves(nullptr, 3, toneCurves)); + brightnessStage = QSharedPointer::create(cmsStageAllocToneCurves(nullptr, 3, toneCurves)); if (!brightnessStage) { qCWarning(KWIN_CORE) << "Failed to create the color brightness pipeline stage"; } @@ -231,11 +202,7 @@ void ColorDevicePrivate::updateCalibrationToneCurves() cmsDupToneCurve(vcgt[1]), cmsDupToneCurve(vcgt[2]), }; - - calibrationStage.reset(cmsStageAllocToneCurves(nullptr, 3, toneCurves)); - if (!calibrationStage) { - qCWarning(KWIN_CORE) << "Failed to create the color calibration pipeline stage"; - } + calibrationStage = QSharedPointer::create(cmsStageAllocToneCurves(nullptr, 3, toneCurves)); } cmsCloseProfile(handle); diff --git a/src/colordevice.h b/src/colors/colordevice.h similarity index 100% rename from src/colordevice.h rename to src/colors/colordevice.h diff --git a/src/colors.cpp b/src/colors/colorlut.cpp similarity index 63% rename from src/colors.cpp rename to src/colors/colorlut.cpp index e9df496e18..607c4262bc 100644 --- a/src/colors.cpp +++ b/src/colors/colorlut.cpp @@ -6,35 +6,13 @@ SPDX-License-Identifier: GPL-2.0-or-later */ -#include "colors.h" +#include "colorlut.h" -#include +#include "colortransformation.h" namespace KWin { -ColorTransformation::ColorTransformation(cmsPipeline *pipeline) - : m_pipeline(pipeline) -{ -} - -ColorTransformation::~ColorTransformation() -{ - cmsStage *last = nullptr; - do { - cmsPipelineUnlinkStage(m_pipeline, cmsAT_END, &last); - } while (last); - cmsPipelineFree(m_pipeline); -} - -std::tuple ColorTransformation::transform(uint16_t r, uint16_t g, uint16_t b) const -{ - const uint16_t in[3] = {r, g, b}; - uint16_t out[3] = {0, 0, 0}; - cmsPipelineEval16(in, out, m_pipeline); - return {out[0], out[1], out[2]}; -} - ColorLUT::ColorLUT(const QSharedPointer &transformation, size_t size) : m_transformation(transformation) { diff --git a/src/colors.h b/src/colors/colorlut.h similarity index 67% rename from src/colors.h rename to src/colors/colorlut.h index 15edb4b07e..3354a1fcb1 100644 --- a/src/colors.h +++ b/src/colors/colorlut.h @@ -9,26 +9,13 @@ #pragma once #include -#include #include "kwin_export.h" -typedef struct _cmsPipeline_struct cmsPipeline; - namespace KWin { -class KWIN_EXPORT ColorTransformation -{ -public: - ColorTransformation(cmsPipeline *pipeline); - ~ColorTransformation(); - - std::tuple transform(uint16_t r, uint16_t g, uint16_t b) const; - -private: - cmsPipeline *const m_pipeline; -}; +class ColorTransformation; class KWIN_EXPORT ColorLUT { diff --git a/src/colormanager.cpp b/src/colors/colormanager.cpp similarity index 100% rename from src/colormanager.cpp rename to src/colors/colormanager.cpp diff --git a/src/colormanager.h b/src/colors/colormanager.h similarity index 100% rename from src/colormanager.h rename to src/colors/colormanager.h diff --git a/src/colors/colorpipelinestage.cpp b/src/colors/colorpipelinestage.cpp new file mode 100644 index 0000000000..8b1c368042 --- /dev/null +++ b/src/colors/colorpipelinestage.cpp @@ -0,0 +1,49 @@ +/* + KWin - the KDE window manager + This file is part of the KDE project. + + SPDX-FileCopyrightText: 2022 Xaver Hugl + + SPDX-License-Identifier: GPL-2.0-or-later +*/ +#include "colorpipelinestage.h" + +#include + +#include "utils/common.h" + +namespace KWin +{ + +ColorPipelineStage::ColorPipelineStage(cmsStage *stage) + : m_stage(stage) +{ +} + +ColorPipelineStage::~ColorPipelineStage() +{ + if (m_stage) { + cmsStageFree(m_stage); + } +} + +QSharedPointer ColorPipelineStage::dup() const +{ + if (m_stage) { + auto dup = cmsStageDup(m_stage); + if (dup) { + return QSharedPointer::create(dup); + } else { + qCWarning(KWIN_CORE) << "Failed to duplicate cmsStage!"; + } + } + return nullptr; + ; +} + +cmsStage *ColorPipelineStage::stage() const +{ + return m_stage; +} + +} diff --git a/src/colors/colorpipelinestage.h b/src/colors/colorpipelinestage.h new file mode 100644 index 0000000000..60cf573d4e --- /dev/null +++ b/src/colors/colorpipelinestage.h @@ -0,0 +1,33 @@ +/* + KWin - the KDE window manager + This file is part of the KDE project. + + SPDX-FileCopyrightText: 2022 Xaver Hugl + + SPDX-License-Identifier: GPL-2.0-or-later +*/ +#pragma once + +#include "kwin_export.h" + +#include + +typedef struct _cmsStage_struct cmsStage; + +namespace KWin +{ + +class KWIN_EXPORT ColorPipelineStage +{ +public: + ColorPipelineStage(cmsStage *stage); + ~ColorPipelineStage(); + + QSharedPointer dup() const; + cmsStage *stage() const; + +private: + cmsStage *const m_stage; +}; + +} diff --git a/src/colors/colortransformation.cpp b/src/colors/colortransformation.cpp new file mode 100644 index 0000000000..e5944f92c8 --- /dev/null +++ b/src/colors/colortransformation.cpp @@ -0,0 +1,61 @@ + +#include "colortransformation.h" + +#include + +#include "colorpipelinestage.h" +#include "utils/common.h" + +namespace KWin +{ + +ColorTransformation::ColorTransformation(const QVector> &stages) + : m_pipeline(cmsPipelineAlloc(nullptr, 3, 3)) +{ + if (!m_pipeline) { + qCWarning(KWIN_CORE) << "Failed to allocate cmsPipeline!"; + m_valid = false; + return; + } + for (const auto &stage : stages) { + if (stage) { + const auto dup = stage->dup(); + if (!dup) { + m_valid = false; + return; + } + m_stages << dup; + if (!cmsPipelineInsertStage(m_pipeline, cmsAT_END, dup->stage())) { + qCWarning(KWIN_CORE) << "Failed to insert cmsPipeline stage!"; + m_valid = false; + return; + } + } + } +} + +ColorTransformation::~ColorTransformation() +{ + if (m_pipeline) { + cmsStage *last = nullptr; + do { + cmsPipelineUnlinkStage(m_pipeline, cmsAT_END, &last); + } while (last); + cmsPipelineFree(m_pipeline); + } +} + +bool ColorTransformation::valid() const +{ + return m_valid; +} + +std::tuple ColorTransformation::transform(uint16_t r, uint16_t g, uint16_t b) const +{ + const uint16_t in[3] = {r, g, b}; + uint16_t out[3] = {0, 0, 0}; + cmsPipelineEval16(in, out, m_pipeline); + return {out[0], out[1], out[2]}; +} + +} diff --git a/src/colors/colortransformation.h b/src/colors/colortransformation.h new file mode 100644 index 0000000000..361324357c --- /dev/null +++ b/src/colors/colortransformation.h @@ -0,0 +1,41 @@ +/* + KWin - the KDE window manager + This file is part of the KDE project. + + SPDX-FileCopyrightText: 2022 Xaver Hugl + + SPDX-License-Identifier: GPL-2.0-or-later +*/ +#pragma once + +#include +#include +#include +#include + +#include "kwin_export.h" + +typedef struct _cmsPipeline_struct cmsPipeline; + +namespace KWin +{ + +class ColorPipelineStage; + +class KWIN_EXPORT ColorTransformation +{ +public: + ColorTransformation(const QVector> &stages); + ~ColorTransformation(); + + bool valid() const; + + std::tuple transform(uint16_t r, uint16_t g, uint16_t b) const; + +private: + cmsPipeline *const m_pipeline; + QVector> m_stages; + bool m_valid = true; +}; + +}