From 4c8d08d2294c042c276f945c11ea7c6dd605025d Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Fri, 5 Jul 2024 14:34:09 +1000 Subject: [PATCH 1/7] platform/DRMFormat: Don't require a `FormatInfo` for each format. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We're now using `DRMFormat` in a situation (linux-dmabuf) where: * We don't actually need to care about many the properties of the format, and * We're processing formats submitted by cilents. This means that the existing “throw an exception if we haven't got a `FormatInfo`” approach works badly; we will disconnect clients with a Wayland internal error if they submit buffers in a format we haven't explicitly listed, even though it *would* work if we just treated the format as an opaque identifier. So, don't do that anymore. Instead, store the fourcc code directly in `DRMFormat` alongside the `FormatInfo` if it exists. Fixes: #3462 --- include/platform/mir/graphics/drm_formats.h | 1 + src/platform/graphics/drm_formats.cpp | 51 ++++++++++++--------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/include/platform/mir/graphics/drm_formats.h b/include/platform/mir/graphics/drm_formats.h index 9344ec9c7d7..c582f330c75 100644 --- a/include/platform/mir/graphics/drm_formats.h +++ b/include/platform/mir/graphics/drm_formats.h @@ -54,6 +54,7 @@ class DRMFormat struct FormatInfo; private: + uint32_t fourcc; FormatInfo const* info; }; diff --git a/src/platform/graphics/drm_formats.cpp b/src/platform/graphics/drm_formats.cpp index b8d7553c2d4..e0de256133b 100644 --- a/src/platform/graphics/drm_formats.cpp +++ b/src/platform/graphics/drm_formats.cpp @@ -67,8 +67,8 @@ struct mg::DRMFormat::FormatInfo { uint32_t format; bool has_alpha; - uint32_t opaque_equivalent; - uint32_t alpha_equivalent; + std::optional opaque_equivalent; + std::optional alpha_equivalent; std::optional components; }; @@ -223,7 +223,7 @@ constexpr std::array const formats = { DRM_FORMAT_RGB565, false, DRM_FORMAT_RGB565, - DRM_FORMAT_INVALID, + {}, mg::DRMFormat::RGBComponentInfo{ 5, 6, 5, {} }, @@ -232,7 +232,7 @@ constexpr std::array const formats = { DRM_FORMAT_BGR565, false, DRM_FORMAT_BGR565, - DRM_FORMAT_INVALID, + {}, mg::DRMFormat::RGBComponentInfo{ 5, 6, 5, {} }, @@ -241,7 +241,7 @@ constexpr std::array const formats = { DRM_FORMAT_RGB888, false, DRM_FORMAT_RGB888, - DRM_FORMAT_INVALID, + {}, mg::DRMFormat::RGBComponentInfo{ 8, 8, 8, {} }, @@ -250,7 +250,7 @@ constexpr std::array const formats = { DRM_FORMAT_BGR888, false, DRM_FORMAT_BGR888, - DRM_FORMAT_INVALID, + {}, mg::DRMFormat::RGBComponentInfo{ 8, 8, 8, {} }, @@ -443,67 +443,76 @@ constexpr auto maybe_info_for_format(uint32_t fourcc_format) -> mg::DRMFormat::F } -constexpr auto info_for_format(uint32_t fourcc_format) -> mg::DRMFormat::FormatInfo const& +constexpr auto info_for_format(uint32_t fourcc_format) -> mg::DRMFormat::FormatInfo const* { auto const info = maybe_info_for_format(fourcc_format); if (info) { - return *info; + return info; } - BOOST_THROW_EXCEPTION(( - std::runtime_error{ - std::string{"Unsupported DRM format: "} + mg::drm_format_to_string(fourcc_format)})); + return nullptr; } } mg::DRMFormat::DRMFormat(uint32_t fourcc_format) - : info{&info_for_format(fourcc_format)} + : fourcc{fourcc_format}, + info{info_for_format(fourcc_format)} { } auto mg::DRMFormat::name() const -> const char* { - return drm_format_to_string(info->format); + return drm_format_to_string(fourcc); } auto mg::DRMFormat::opaque_equivalent() const -> const std::optional { - if (info->opaque_equivalent != DRM_FORMAT_INVALID) + if (info) { - return DRMFormat{info->opaque_equivalent}; + return info->opaque_equivalent.transform([](auto const& fourcc) { return DRMFormat{fourcc};}); } return {}; } auto mg::DRMFormat::alpha_equivalent() const -> const std::optional { - if (info->alpha_equivalent != DRM_FORMAT_INVALID) + if (info) { - return DRMFormat{info->alpha_equivalent}; + return info->alpha_equivalent.transform([](auto const& fourcc) { return DRMFormat{fourcc};}); } return {}; } bool mg::DRMFormat::has_alpha() const { - return info->has_alpha; + if (info) + { + return info->has_alpha; + } + // The safe default if we don't know is “true” + return true; } auto mg::DRMFormat::components() const -> std::optional const& { - return info->components; + static std::optional no_components = std::nullopt; + if (info) + { + return info->components; + } + return no_components; } mg::DRMFormat::operator uint32_t() const { - return info->format; + return fourcc; } auto mg::DRMFormat::as_mir_format() const -> std::optional { - switch (info->format) + switch (fourcc) { case DRM_FORMAT_ARGB8888: return mir_pixel_format_argb_8888; From 6b31e6eb385075b1330f6b8f7755c439cec0cf9b Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Mon, 8 Jul 2024 14:59:23 +1000 Subject: [PATCH 2/7] platform/DRMFormat: Defer format info lookup until requested This way things which treat `DRMFormat` as entirely opaque don't pay for the extra format information, and we can warn when the things that *do* care about the format details try to look up details we haven't included yet. --- include/platform/mir/graphics/drm_formats.h | 42 ++++-- src/platform/graphics/drm_formats.cpp | 130 ++++++++---------- src/platform/graphics/linux_dmabuf.cpp | 13 +- src/platform/symbols.map | 9 +- .../gbm-kms/server/buffer_allocator.cpp | 31 +++-- src/server/scene/basic_surface.cpp | 4 +- 6 files changed, 124 insertions(+), 105 deletions(-) diff --git a/include/platform/mir/graphics/drm_formats.h b/include/platform/mir/graphics/drm_formats.h index c582f330c75..55af23d1992 100644 --- a/include/platform/mir/graphics/drm_formats.h +++ b/include/platform/mir/graphics/drm_formats.h @@ -27,35 +27,47 @@ namespace mir::graphics class DRMFormat { public: - struct RGBComponentInfo + struct FormatInfo; + class Info { - uint32_t red_bits; - uint32_t green_bits; - uint32_t blue_bits; - std::optional alpha_bits; - }; + public: + struct RGBComponentInfo + { + uint32_t red_bits; + uint32_t green_bits; + uint32_t blue_bits; + std::optional alpha_bits; + }; - // This could be constexpr, at the cost of moving a bunch of implementation into the header - explicit DRMFormat(uint32_t fourcc_format); + auto opaque_equivalent() const -> std::optional; + auto alpha_equivalent() const -> std::optional; - auto name() const -> char const*; + bool has_alpha() const; - auto opaque_equivalent() const -> std::optional const; - auto alpha_equivalent() const -> std::optional const; + auto components() const -> std::optional const&; + private: + friend class DRMFormat; + Info(FormatInfo const* info); - bool has_alpha() const; + FormatInfo const* info; + }; - auto components() const -> std::optional const&; + constexpr explicit DRMFormat(uint32_t fourcc_format) + : fourcc{fourcc_format} + { + } + + auto name() const -> char const*; + + auto info() const -> std::optional; operator uint32_t() const; auto as_mir_format() const -> std::optional; static auto from_mir_format(MirPixelFormat format) -> DRMFormat; - struct FormatInfo; private: uint32_t fourcc; - FormatInfo const* info; }; auto drm_modifier_to_string(uint64_t modifier) -> std::string; diff --git a/src/platform/graphics/drm_formats.cpp b/src/platform/graphics/drm_formats.cpp index e0de256133b..f27f2e0f688 100644 --- a/src/platform/graphics/drm_formats.cpp +++ b/src/platform/graphics/drm_formats.cpp @@ -69,7 +69,7 @@ struct mg::DRMFormat::FormatInfo bool has_alpha; std::optional opaque_equivalent; std::optional alpha_equivalent; - std::optional components; + std::optional components; }; namespace @@ -80,7 +80,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_XRGB4444, DRM_FORMAT_ARGB4444, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 4, 4, 4, {} }, }, @@ -89,7 +89,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_XBGR4444, DRM_FORMAT_ABGR4444, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 4, 4, 4, {} }, }, @@ -98,7 +98,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_RGBX4444, DRM_FORMAT_RGBA4444, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 4, 4, 4, {} }, }, @@ -107,7 +107,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_BGRX4444, DRM_FORMAT_BGRA4444, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 4, 4, 4, {} }, }, @@ -116,7 +116,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_XRGB4444, DRM_FORMAT_ARGB4444, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 4, 4, 4, 4 }, }, @@ -125,7 +125,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_XBGR4444, DRM_FORMAT_ABGR4444, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 4, 4, 4, 4 }, }, @@ -134,7 +134,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_RGBX4444, DRM_FORMAT_RGBA4444, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 4, 4, 4, 4 }, }, @@ -143,7 +143,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_BGRX4444, DRM_FORMAT_BGRA4444, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 4, 4, 4, 4 }, }, @@ -152,7 +152,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_XRGB1555, DRM_FORMAT_ARGB1555, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 5, 5, {} }, }, @@ -161,7 +161,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_XBGR1555, DRM_FORMAT_ABGR1555, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 5, 5, {} }, }, @@ -170,7 +170,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_RGBX5551, DRM_FORMAT_RGBA5551, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 5, 5, {} }, }, @@ -179,7 +179,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_BGRX5551, DRM_FORMAT_BGRA5551, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 5, 5, {} }, }, @@ -188,7 +188,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_XRGB1555, DRM_FORMAT_ARGB1555, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 5, 5, 1 }, }, @@ -197,7 +197,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_XBGR1555, DRM_FORMAT_ABGR1555, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 5, 5, 1 }, }, @@ -206,7 +206,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_RGBX5551, DRM_FORMAT_RGBA5551, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 5, 5, 1 }, }, @@ -215,7 +215,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_BGRX5551, DRM_FORMAT_BGRA5551, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 5, 5, 1 }, }, @@ -224,7 +224,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_RGB565, {}, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 6, 5, {} }, }, @@ -233,7 +233,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_BGR565, {}, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 5, 6, 5, {} }, }, @@ -242,7 +242,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_RGB888, {}, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, {} }, }, @@ -251,7 +251,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_BGR888, {}, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, {} }, }, @@ -260,7 +260,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_XRGB8888, DRM_FORMAT_ARGB8888, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, {} }, }, @@ -269,7 +269,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_XBGR8888, DRM_FORMAT_ABGR8888, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, {} }, }, @@ -278,7 +278,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_RGBX8888, DRM_FORMAT_RGBA8888, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, {} }, }, @@ -287,7 +287,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_BGRX8888, DRM_FORMAT_BGRA8888, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, {} }, }, @@ -296,7 +296,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_XRGB8888, DRM_FORMAT_ARGB8888, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, 8 }, }, @@ -305,7 +305,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_XBGR8888, DRM_FORMAT_ABGR8888, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, 8 }, }, @@ -314,7 +314,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_RGBX8888, DRM_FORMAT_RGBA8888, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, 8 }, }, @@ -323,7 +323,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_BGRX8888, DRM_FORMAT_BGRA8888, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 8, 8, 8, 8 }, }, @@ -332,7 +332,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_XRGB2101010, DRM_FORMAT_ARGB2101010, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 10, 10, 10, {} }, }, @@ -341,7 +341,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_XBGR2101010, DRM_FORMAT_ABGR2101010, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 10, 10, 10, {} }, }, @@ -350,7 +350,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_RGBX1010102, DRM_FORMAT_RGBA1010102, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 10, 10, 10, {} }, }, @@ -359,7 +359,7 @@ constexpr std::array const formats = { false, DRM_FORMAT_BGRX1010102, DRM_FORMAT_BGRA1010102, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 10, 10, 10, {} }, }, @@ -368,7 +368,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_XRGB2101010, DRM_FORMAT_ARGB2101010, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 10, 10, 10, 2 }, }, @@ -377,7 +377,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_XBGR2101010, DRM_FORMAT_ABGR2101010, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 10, 10, 10, 2 }, }, @@ -386,7 +386,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_RGBX1010102, DRM_FORMAT_RGBA1010102, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 10, 10, 10, 2 }, }, @@ -395,7 +395,7 @@ constexpr std::array const formats = { true, DRM_FORMAT_BGRX1010102, DRM_FORMAT_BGRA1010102, - mg::DRMFormat::RGBComponentInfo{ + mg::DRMFormat::Info::RGBComponentInfo{ 10, 10, 10, 2 }, }, @@ -442,67 +442,45 @@ constexpr auto maybe_info_for_format(uint32_t fourcc_format) -> mg::DRMFormat::F } } - -constexpr auto info_for_format(uint32_t fourcc_format) -> mg::DRMFormat::FormatInfo const* -{ - auto const info = maybe_info_for_format(fourcc_format); - - if (info) - { - return info; - } - return nullptr; } +auto mg::DRMFormat::name() const -> const char* +{ + return drm_format_to_string(fourcc); } -mg::DRMFormat::DRMFormat(uint32_t fourcc_format) - : fourcc{fourcc_format}, - info{info_for_format(fourcc_format)} +auto mg::DRMFormat::Info::opaque_equivalent() const -> std::optional { + return info->opaque_equivalent.transform([](auto const& fourcc) { return DRMFormat{fourcc};}); } -auto mg::DRMFormat::name() const -> const char* +auto mg::DRMFormat::Info::alpha_equivalent() const -> std::optional { - return drm_format_to_string(fourcc); + return info->alpha_equivalent.transform([](auto const& fourcc) { return DRMFormat{fourcc};}); } -auto mg::DRMFormat::opaque_equivalent() const -> const std::optional +bool mg::DRMFormat::Info::has_alpha() const { - if (info) - { - return info->opaque_equivalent.transform([](auto const& fourcc) { return DRMFormat{fourcc};}); - } - return {}; + return info->has_alpha; } -auto mg::DRMFormat::alpha_equivalent() const -> const std::optional +auto mg::DRMFormat::Info::components() const -> std::optional const& { - if (info) - { - return info->alpha_equivalent.transform([](auto const& fourcc) { return DRMFormat{fourcc};}); - } - return {}; + return info->components; } -bool mg::DRMFormat::has_alpha() const +mg::DRMFormat::Info::Info(FormatInfo const* info) + : info{info} { - if (info) - { - return info->has_alpha; - } - // The safe default if we don't know is “true” - return true; } -auto mg::DRMFormat::components() const -> std::optional const& +auto mg::DRMFormat::info() const -> std::optional { - static std::optional no_components = std::nullopt; - if (info) + if (auto info = maybe_info_for_format(fourcc)) { - return info->components; + return Info(info); } - return no_components; + return {}; } mg::DRMFormat::operator uint32_t() const diff --git a/src/platform/graphics/linux_dmabuf.cpp b/src/platform/graphics/linux_dmabuf.cpp index d28b53903d2..81f80e22fb1 100644 --- a/src/platform/graphics/linux_dmabuf.cpp +++ b/src/platform/graphics/linux_dmabuf.cpp @@ -944,6 +944,17 @@ class DMABufTex : public mg::gl::Texture std::shared_ptr const egl_delegate; }; +namespace +{ +auto format_has_known_alpha(mg::DRMFormat format) -> std::optional +{ + /* TODO: It would be sensible to log when we don't have Info for the format, + * but this will happen a lot, so we would want some ratelimiting in the logger + */ + return format.info().transform([](auto const& info) { return info.has_alpha(); }); +} +} + class DmabufTexBuffer : public mg::BufferBasic, public mg::DMABufBuffer @@ -965,7 +976,7 @@ class DmabufTexBuffer : on_consumed{std::move(on_consumed)}, on_release{std::move(on_release)}, size_{dma_buf.size()}, - has_alpha{dma_buf.format().has_alpha()}, + has_alpha{format_has_known_alpha(dma_buf.format()).value_or(true)}, // Has-alpha is the safe default for unknown formats planes_{dma_buf.planes()}, modifier_{dma_buf.modifier()}, format_{dma_buf.format()} diff --git a/src/platform/symbols.map b/src/platform/symbols.map index 80961be7627..823491a8e08 100644 --- a/src/platform/symbols.map +++ b/src/platform/symbols.map @@ -10,14 +10,15 @@ MIR_PLATFORM_2.17 { mir::graphics::DMABufEGLProvider::DMABufEGLProvider*; mir::graphics::DMABufEGLProvider::as_texture*; mir::graphics::DRMFormat::DRMFormat*; - mir::graphics::DRMFormat::alpha_equivalent*; + mir::graphics::DRMFormat::Info::alpha_equivalent*; mir::graphics::DRMFormat::as_mir_format*; - mir::graphics::DRMFormat::components*; + mir::graphics::DRMFormat::Info::components*; mir::graphics::DRMFormat::from_mir_format*; - mir::graphics::DRMFormat::has_alpha*; + mir::graphics::DRMFormat::Info::has_alpha*; mir::graphics::DRMFormat::name*; - mir::graphics::DRMFormat::opaque_equivalent*; + mir::graphics::DRMFormat::Info::opaque_equivalent*; mir::graphics::DRMFormat::operator?unsigned?int*; /* Is actually operator uint32_t(), but 🤷 */ + mir::graphics::DRMFormat::info*; mir::graphics::DisplayConfiguration::operator*; mir::graphics::DisplayConfiguration::valid*; mir::graphics::DisplayConfigurationOutput::extents*; diff --git a/src/platforms/gbm-kms/server/buffer_allocator.cpp b/src/platforms/gbm-kms/server/buffer_allocator.cpp index 4b4156c3725..be849f41205 100644 --- a/src/platforms/gbm-kms/server/buffer_allocator.cpp +++ b/src/platforms/gbm-kms/server/buffer_allocator.cpp @@ -350,14 +350,18 @@ class GBMOutputSurface : public mg::gl::OutputSurface static auto egl_config_for_format(EGLDisplay dpy, mg::GLConfig const& config, mg::DRMFormat format) -> std::optional { - mg::DRMFormat::RGBComponentInfo const default_components = { 8, 8, 8, 0}; + mg::DRMFormat::Info::RGBComponentInfo const default_components = { 8, 8, 8, 0}; + auto const components = + format.info().transform([](auto info) { return info.components(); }) + .value_or(default_components) // optional> -> optional + .value_or(default_components); // optional -> Components EGLint const config_attr[] = { EGL_SURFACE_TYPE, EGL_WINDOW_BIT, - EGL_RED_SIZE, static_cast(format.components().value_or(default_components).red_bits), - EGL_GREEN_SIZE, static_cast(format.components().value_or(default_components).green_bits), - EGL_BLUE_SIZE, static_cast(format.components().value_or(default_components).blue_bits), - EGL_ALPHA_SIZE, static_cast(format.components().value_or(default_components).alpha_bits.value_or(0)), + EGL_RED_SIZE, static_cast(components.red_bits), + EGL_GREEN_SIZE, static_cast(components.green_bits), + EGL_BLUE_SIZE, static_cast(components.blue_bits), + EGL_ALPHA_SIZE, static_cast(components.alpha_bits.value_or(0)), EGL_DEPTH_SIZE, config.depth_buffer_bits(), EGL_STENCIL_SIZE, config.stencil_buffer_bits(), EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT, @@ -405,11 +409,22 @@ class GBMOutputSurface : public mg::gl::OutputSurface auto alternate_format = [&]() -> std::optional { - if (format.has_alpha()) + if (auto info = format.info()) { - return format.opaque_equivalent(); + if (info->has_alpha()) + { + return info->opaque_equivalent(); + } + else + { + return info->alpha_equivalent(); + } + } + else + { + // If we don't know about the format, we can't find alternatives + return {}; } - return format.alpha_equivalent(); }(); if (alternate_format) diff --git a/src/server/scene/basic_surface.cpp b/src/server/scene/basic_surface.cpp index a58e54e40aa..2af9f2ce2bf 100644 --- a/src/server/scene/basic_surface.cpp +++ b/src/server/scene/basic_surface.cpp @@ -729,7 +729,9 @@ class SurfaceSnapshot : public mg::Renderable { return transformation_; } bool shaped() const override - { return entry->pixel_format().has_alpha(); } + { + return entry->pixel_format().info().transform([](auto info) { return info.has_alpha();}).value_or(true); + } mg::Renderable::ID id() const override { return id_; } From 202b2b0afc535a3819c63ed04d911248a0e3009f Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Mon, 8 Jul 2024 17:51:52 +1000 Subject: [PATCH 3/7] platform/DRMFormat: Log when we're asked for missing format info --- src/platform/graphics/drm_formats.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/platform/graphics/drm_formats.cpp b/src/platform/graphics/drm_formats.cpp index f27f2e0f688..5d20f3bfe49 100644 --- a/src/platform/graphics/drm_formats.cpp +++ b/src/platform/graphics/drm_formats.cpp @@ -24,6 +24,8 @@ #include #include +#include "mir/log.h" + #ifdef MIR_HAVE_DRM_GET_MODIFIER_NAME #include #endif @@ -480,6 +482,11 @@ auto mg::DRMFormat::info() const -> std::optional { return Info(info); } + /* TODO: This could fire quite frequently. + * Ideally we would have a rate-limited (or once-per-format) logger, but for now, just + * throw it in the debug stream. + */ + mir::log_debug("Detailed info for format %s missing; please report so this can be added", name()); return {}; } From 0c244d653e962f3d67d38a6906d18f343ea0ca3c Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Tue, 9 Jul 2024 16:28:04 +1000 Subject: [PATCH 4/7] Bump mirplatform ABI; we've dropped some `DRMFormat` symbols --- debian/control | 4 ++-- debian/libmirplatform28.install | 1 - debian/libmirplatform29.install | 1 + src/CMakeLists.txt | 2 +- src/platform/symbols.map | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) delete mode 100644 debian/libmirplatform28.install create mode 100644 debian/libmirplatform29.install diff --git a/debian/control b/debian/control index a05b93b6888..e8350992dc3 100644 --- a/debian/control +++ b/debian/control @@ -77,7 +77,7 @@ Description: Display server for Ubuntu - server library . Contains the shared library needed by server applications for Mir. -Package: libmirplatform28 +Package: libmirplatform29 Section: libs Architecture: linux-any Multi-Arch: same @@ -140,7 +140,7 @@ Section: libdevel Architecture: linux-any Multi-Arch: same Pre-Depends: ${misc:Pre-Depends} -Depends: libmirplatform28 (= ${binary:Version}), +Depends: libmirplatform29 (= ${binary:Version}), libmircommon-dev (= ${binary:Version}), libboost-program-options-dev, ${misc:Depends}, diff --git a/debian/libmirplatform28.install b/debian/libmirplatform28.install deleted file mode 100644 index 8b2aeef1ca8..00000000000 --- a/debian/libmirplatform28.install +++ /dev/null @@ -1 +0,0 @@ -usr/lib/*/libmirplatform.so.28 diff --git a/debian/libmirplatform29.install b/debian/libmirplatform29.install new file mode 100644 index 00000000000..aae6fdb0e79 --- /dev/null +++ b/debian/libmirplatform29.install @@ -0,0 +1 @@ +usr/lib/*/libmirplatform.so.29 diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 498724de7ce..a05ca99d99e 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -1,5 +1,5 @@ # We need MIRPLATFORM_ABI in both libmirplatform and the platform implementations. -set(MIRPLATFORM_ABI 28) +set(MIRPLATFORM_ABI 29) set(MIRAL_VERSION_MAJOR 5) set(MIRAL_VERSION_MINOR 1) diff --git a/src/platform/symbols.map b/src/platform/symbols.map index 823491a8e08..6cc7ac3e89c 100644 --- a/src/platform/symbols.map +++ b/src/platform/symbols.map @@ -1,4 +1,4 @@ -MIR_PLATFORM_2.17 { +MIR_PLATFORM_2.18 { global: extern "C++" { mir::graphics::AtomicFrame::increment*; From e380660e9721002f8de7d089b6ebcc67a5d80bef Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Fri, 12 Jul 2024 13:19:05 +1000 Subject: [PATCH 5/7] platform/DRMFormat: Log *only once per format* when we're asked for missing format info --- src/platform/graphics/drm_formats.cpp | 16 +++++++++++----- src/platform/graphics/linux_dmabuf.cpp | 3 --- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/platform/graphics/drm_formats.cpp b/src/platform/graphics/drm_formats.cpp index 5d20f3bfe49..6cab34cc2cb 100644 --- a/src/platform/graphics/drm_formats.cpp +++ b/src/platform/graphics/drm_formats.cpp @@ -15,6 +15,7 @@ */ #include "mir/graphics/drm_formats.h" +#include "mir/synchronised.h" #include "mir_toolkit/common.h" #include @@ -22,6 +23,7 @@ #include #include #include +#include #include #include "mir/log.h" @@ -482,11 +484,15 @@ auto mg::DRMFormat::info() const -> std::optional { return Info(info); } - /* TODO: This could fire quite frequently. - * Ideally we would have a rate-limited (or once-per-format) logger, but for now, just - * throw it in the debug stream. - */ - mir::log_debug("Detailed info for format %s missing; please report so this can be added", name()); + + // Actually, we probably want `std::flat_set`, but no libstdc++ support yet. + static Synchronised> unknown_formats_guard; + auto unknown_formats = unknown_formats_guard.lock(); + if (!unknown_formats->contains(fourcc)) + { + mir::log_warning("Detailed info for format %s missing; please report so this can be added", name()); + unknown_formats->insert(fourcc); + } return {}; } diff --git a/src/platform/graphics/linux_dmabuf.cpp b/src/platform/graphics/linux_dmabuf.cpp index 81f80e22fb1..9eea3a8bb4a 100644 --- a/src/platform/graphics/linux_dmabuf.cpp +++ b/src/platform/graphics/linux_dmabuf.cpp @@ -948,9 +948,6 @@ namespace { auto format_has_known_alpha(mg::DRMFormat format) -> std::optional { - /* TODO: It would be sensible to log when we don't have Info for the format, - * but this will happen a lot, so we would want some ratelimiting in the logger - */ return format.info().transform([](auto const& info) { return info.has_alpha(); }); } } From f3a0b274b2e2d6f1e25a0a873510bad0c4725b22 Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Fri, 12 Jul 2024 13:21:57 +1000 Subject: [PATCH 6/7] trivial: Be more precise in returning `std::nullopt` --- src/platform/graphics/drm_formats.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/graphics/drm_formats.cpp b/src/platform/graphics/drm_formats.cpp index 6cab34cc2cb..a79c8932ec8 100644 --- a/src/platform/graphics/drm_formats.cpp +++ b/src/platform/graphics/drm_formats.cpp @@ -493,7 +493,7 @@ auto mg::DRMFormat::info() const -> std::optional mir::log_warning("Detailed info for format %s missing; please report so this can be added", name()); unknown_formats->insert(fourcc); } - return {}; + return std::nullopt; } mg::DRMFormat::operator uint32_t() const From 2d01c224173dd119cef6c6fdc798621a91a37a3d Mon Sep 17 00:00:00 2001 From: Christopher James Halse Rogers Date: Mon, 15 Jul 2024 15:27:29 +1000 Subject: [PATCH 7/7] platform/DRMFormat: Point people at the GitHub new issue page for reporting missing formats --- src/platform/graphics/drm_formats.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/platform/graphics/drm_formats.cpp b/src/platform/graphics/drm_formats.cpp index a79c8932ec8..42f549d7426 100644 --- a/src/platform/graphics/drm_formats.cpp +++ b/src/platform/graphics/drm_formats.cpp @@ -490,7 +490,8 @@ auto mg::DRMFormat::info() const -> std::optional auto unknown_formats = unknown_formats_guard.lock(); if (!unknown_formats->contains(fourcc)) { - mir::log_warning("Detailed info for format %s missing; please report so this can be added", name()); + mir::log_warning( + "Detailed info for format %s missing; please report this to https://github.com/canonical/mir/issues/new so this can be added", name()); unknown_formats->insert(fourcc); } return std::nullopt;