yukijoou/kwin
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

ScreenCast : fix edge-case for format modifier fixation

Browse source 
The current implementation for DMA-Buf sharing on kwin is slightly broken.

When a client connects to the kwin screencast's stream, it supplies a `SPA_PARAM_EnumFormat` list, containing entries for every supported format, and the drm modifier list for that format (for dmabuf sharing).

Usually, the `SPA_FORMAT_VIDEO_modifier` property from the consumer has the `SPA_POD_PROP_FLAG_DONT_FIXATE` flag set. Kwin then receives the full choice list, and gets to pick a relevant modifier.

In situations where the DONT_FIXATE flag is missing, pipewire chooses an arbitrary match between the consumer and kwin. In that case, kwin currently assumes DRM_FORMAT_MOD_INVALID is to be used no matter what, which goes against the comment right above it within the screencaststream.cpp file. Even worse, if DRM_FORMAT_MOD_INVALID is not supported by the consumer, this also causes the param_changed callback to freak out, as one of the conditions to update the stream params is `!receivedModifiers.contains(m_dmabufParams->modifier`. Since m_dmabufParams->modifier contains DRM_FORMAT_MOD_INVALID, which is not among the modifiers kwin receives, the params are changed continuously and no buffer sharing can happen.

--

I'd be happy to supply code to reproduce what I attempted to describe above if needed. I'm not aware of any program currently affected, but I found this out when messing around myself.

Relevant docs are here :
<https://docs.pipewire.org/page_dma_buf.html>

> If the SPA_PARAM_Format contains a modifier key, without the flag SPA_POD_PROP_FLAG_DONT_FIXATE, it should only contain one value in the SPA_CHOICE_Enum. In this case announce the SPA_PARAM_Buffers accordingly to the selected format and modifier.
This commit is contained in:
Gatien DA ROCHA 2024-08-29 20:18:28 +00:00 committed by Vlad Zahorodnii
parent 0eb143c0ef
commit e86ae36088
1 changed files with 7 additions and 11 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
src/plugins/screencast/screencaststream.cpp
View file

@ -199,7 +199,7 @@ void ScreenCastStream::onStreamParamChanged(uint32_t id, const struct spa_pod *f
}
if (!format || id != SPA_PARAM_Format) {
qCDebug(KWIN_SCREENCAST) << objectName() << "stream param request ignored, id:" << id << "and with format:"<< (format != nullptr);
qCDebug(KWIN_SCREENCAST) << objectName() << "stream param request ignored, id:" << id << "and with format:" << (format != nullptr);
return;
}
@ -220,16 +220,12 @@ void ScreenCastStream::onStreamParamChanged(uint32_t id, const struct spa_pod *f
}
if (!m_dmabufParams || m_dmabufParams->width != m_resolution.width() || m_dmabufParams->height != m_resolution.height() || !receivedModifiers.contains(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);
}
m_dmabufParams = testCreateDmaBuf(m_resolution, m_drmFormat, receivedModifiers);
} else {
m_dmabufParams = testCreateDmaBuf(m_resolution, m_drmFormat, {DRM_FORMAT_MOD_INVALID});
// 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);
}
m_dmabufParams = testCreateDmaBuf(m_resolution, m_drmFormat, receivedModifiers);
// 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
@ -247,7 +243,7 @@ void ScreenCastStream::onStreamParamChanged(uint32_t id, const struct spa_pod *f
return;
}
} else {
m_dmabufParams.reset();
m_dmabufParams.reset();
}
qCDebug(KWIN_SCREENCAST) << objectName() << "Stream format found, defining buffers";