From 75c7689f678409b594e3f4ed0161aa12f2baf1ec Mon Sep 17 00:00:00 2001 From: Jan Grulich Date: Wed, 15 Mar 2023 15:38:46 +0100 Subject: [PATCH] Screencast: fix renegotiation when client fails to use DMABuf modifiers When client fails to import a DMABuf with a certain modifier, it will remove it from offerings on his side and offer a new set of modifiers for us to use. In case the list ends up being small enough that even we won't be able to use any of the modifiers, we would still keep these modifiers on our list and offering them to the client. With this we would end up in a loop renegotiating and never actually fallback to use MemFD buffers instead. We should also offer modifier-less DMABufs as in the past. Using these we just need to avoid offering them as they should be used as the last option. BUG: 448839 --- src/plugins/screencast/screencaststream.cpp | 26 ++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/plugins/screencast/screencaststream.cpp b/src/plugins/screencast/screencaststream.cpp index 334c9bf9a9..5143afe10f 100644 --- a/src/plugins/screencast/screencaststream.cpp +++ b/src/plugins/screencast/screencaststream.cpp @@ -165,14 +165,36 @@ void ScreenCastStream::onStreamParamChanged(void *data, uint32_t id, const struc uint32_t modifiersCount = SPA_POD_CHOICE_N_VALUES(modifierPod); uint64_t *modifiers = (uint64_t *)SPA_POD_CHOICE_VALUES(modifierPod); receivedModifiers = QVector(modifiers, modifiers + modifiersCount); + // Remove duplicates + std::sort(receivedModifiers.begin(), receivedModifiers.end()); + receivedModifiers.erase(std::unique(receivedModifiers.begin(), receivedModifiers.end()), receivedModifiers.end()); } if (modifierProperty && (!pw->m_dmabufParams || !receivedModifiers.contains(pw->m_dmabufParams->modifier))) { if (modifierProperty->flags & SPA_POD_PROP_FLAG_DONT_FIXATE) { + // DRM_MOD_INVALID should be used as a last option. Do not just remove it it's the only + // item on the list + if (receivedModifiers.count() > 1) { + receivedModifiers.removeAll(DRM_FORMAT_MOD_INVALID); + } pw->m_dmabufParams = kwinApp()->outputBackend()->testCreateDmaBuf(pw->m_resolution, pw->m_drmFormat, receivedModifiers); } else { pw->m_dmabufParams = kwinApp()->outputBackend()->testCreateDmaBuf(pw->m_resolution, pw->m_drmFormat, {DRM_FORMAT_MOD_INVALID}); } + // In case we fail to use any modifier from the list of offered ones, remove these + // from our all future offerings, otherwise there will be no indication that it cannot + // be used and clients can go for it over and over + if (!pw->m_dmabufParams.has_value()) { + for (uint64_t modifier : receivedModifiers) { + pw->m_modifiers.removeAll(modifier); + } + // Also in case DRM_FORMAT_MOD_INVALID was used and didn't fail, we still need to + // set it as our modifier, otherwise it would be set to default value (0) which is + // also a valid modifier, but not the one we want to actually use + } else if (receivedModifiers.count() == 1 && receivedModifiers.constFirst() == DRM_FORMAT_MOD_INVALID) { + pw->m_dmabufParams->modifier = DRM_FORMAT_MOD_INVALID; + } + qCDebug(KWIN_SCREENCAST) << "Stream dmabuf modifiers received, offering our best suited modifier" << pw->m_dmabufParams.has_value(); char buffer[2048]; auto params = pw->buildFormats(pw->m_dmabufParams.has_value(), buffer); @@ -356,10 +378,12 @@ bool ScreenCastStream::createStream() if (itModifiers == supported.constEnd()) { m_drmFormat = m_source->drmFormat(); - m_modifiers = {}; + m_modifiers = {DRM_FORMAT_MOD_INVALID}; } else { m_drmFormat = itModifiers.key(); m_modifiers = *itModifiers; + // Also support modifier-less DmaBufs + m_modifiers += DRM_FORMAT_MOD_INVALID; } m_hasDmaBuf = kwinApp()->outputBackend()->testCreateDmaBuf(m_resolution, m_drmFormat, {DRM_FORMAT_MOD_INVALID}).has_value();