[vlc-commits] [Git][videolan/vlc][master] 2 commits: Revert "qml: set children ShaderEffects' `supportsAtlasTextures` in BlurEffect"
Steve Lhomme (@robUx4)
gitlab at videolan.org
Mon Mar 24 15:29:03 UTC 2025
Steve Lhomme pushed to branch master at VideoLAN / VLC
Commits:
6e8403fa by Fatih Uzunoglu at 2025-03-24T15:15:24+00:00
Revert "qml: set children ShaderEffects' `supportsAtlasTextures` in BlurEffect"
This reverts commit 2baca7155cfb2227214e2f3ea0f930b6e5450ca6.
Unfortunately it turns out that "They support atlas/sub textures" is not fully
correct. I made that assumption after taking a look at the shader code, and I
concluded that they simply did not bother setting `supportsAtlasTextures` to
`true` from its default value `false` due to negligence because the shader
code that samples the source texture primarily uses the coordinate Qt supplies
(`qt_MultiTexCoord0`) where it already takes the atlas textures into account.
ShaderEffect's documentation states that:
> If supportsAtlasTextures is true, coordinates will be based on position in
> the atlas instead.
Therefore, the coordinates supplied can be used as is with atlas textures. In
fact, we (and Qt) are already doing this in various places (`vec4 texel =
texture(source, qt_TexCoord0)`).
The blur effect shaders uses the same (pre-adjusted) coordinate for sampling
the texture. So, I thought that setting `supportsAtlasTextures` would not have
a negative impact.
However, due to the way Qt does blurring, this pre-adjusted (atlas) coordinate
is offsetted and that offset is obviously specific to the blur effect and not
pre-adjusted for the atlas texture (as Qt does with `ShaderEffect`) since
they never claim to support atlas textures in blur effect, they don't bother
normalizing the offset coordinate according to the atlas.
Unfortunately, this means that atlas textures must be detached from the atlas,
incurring a copy, to be used as a source for the blur effect. This is not a
big problem for us, because most of the places where we use the blur effect
already have the source textures independent (not in the atlas). And we still
benefit from `QSGTextureView` that it prevents incurring a copy when the source
texture is not in the atlas.
- Main view (for mini player frosted glass effect background) is an item
layer (not to mention a dynamic texture). They are never put in the atlas.
- Player background uses mipmap, currently Qt does not support mipmapped
atlas textures, so all mipmapped textures are independent textures. We use
mipmapping here not to prevent this issue, but we actually benefit from
mipmapping in this case as we do fluent size change based on window size.
- Music artist header background, depending on the atlas size, may be put
in the atlas. This currently depends on the initial window size, if it is
small, the texture is not put in the atlas and blurring works fine. This
is because currently Qt determines the atlas size based on the initial
window size (I guess this is from the times when they primarily targeted
mobile platforms with Qt Quick, back in 2010s).
Unfortunately `QQuickImage` (`Image`) does not have a parameter to disable
putting textures in the atlas. Setting `mipmap` as done in player would
be fine, but we don't want/need mipmapping in music artist view. This does
not mean much anyway, as with the revert here the shader effect would ask
the texture to "remove" itself from the atlas (by copying, as per the default
RHI atlas texture implementation), so blurring starts working fine.
This change brings an additional requirement, although not used currently,
if a target of `QSGTextureView` is in the atlas, and the texture view is
used as source for blur effect, the request of detaching from the atlas
texture (`QSGTexture::removedFromAtlas()`) would fail. The next commit
makes sure that `::removedFromAtlas()` works fine with `QSGTextureView`,
too, even though currently it is not used (I used a trick to disable
layering instead of using `QSGTextureView` in music artist header).
- - - - -
1908011f by Fatih Uzunoglu at 2025-03-24T15:15:24+00:00
qt: implement `QSGTextureView::removedFromAtlas()`
This is useful when `QSGTextureView` is used as the source
of a `ShaderEffect` that does not support atlas textures.
Notable examples are `FastBlur` and `MultiEffect`.
- - - - -
3 changed files:
- modules/gui/qt/util/qsgtextureview.cpp
- modules/gui/qt/widgets/qml/BlurEffect.qml
- modules/gui/qt/widgets/qml/compat/BlurEffect.qml
Changes:
=====================================
modules/gui/qt/util/qsgtextureview.cpp
=====================================
@@ -191,12 +191,27 @@ bool QSGTextureView::isAtlasTexture() const
QSGTexture *QSGTextureView::removedFromAtlas(QRhiResourceUpdateBatch *batch) const
{
- Q_UNUSED(batch);
-
- // QSGTextureView does not remove the source texture from the atlas. Removing
- // from atlas means copying the texture, and this is not necessary. Shaders
- // support atlas texture by having `qt_SubRect_X` uniform, or, in the case
- // of `ShaderEffect`, `supportsAtlasTextures` is set.
+ assert(m_texture); // it is absurd to call removedFromAtlas() when there is no target
+
+ // We can "remove" the target texture from the atlas, and re-attach the view to the detached texture.
+ // It should be noted that this method does not mutate the target texture, which can be guessed by
+ // the method signature (being a const method), but rather creates a new texture which is an independent
+ // texture that has its content copied from the atlas texture (still owned by the atlas texture).
+ // This would not cause a leak, because:
+ // 1) we are not owning the atlas texture (current view target) here.
+ // 2) the returned (detached) texture is owned by the atlas texture (current view target).
+ QSGTexture *const detachedTexture = m_texture->removedFromAtlas(batch);
+ if (Q_LIKELY(detachedTexture))
+ {
+ // This virtual method is const, which is fine for QSGTexture (removing
+ // from atlas means returning a new image which is a copy from the relevant
+ // part of the atlas), but not for QSGTextureView, because the texture view
+ // needs to mutate to point to the detached texture. There is not much to
+ // do besides 1) creating a new QSGTextureView and returning it, and leaking
+ // this instance 2) using const cast.
+ const_cast<QSGTextureView*>(this)->setTexture(detachedTexture);
+ return const_cast<QSGTextureView*>(this);
+ }
return nullptr;
}
=====================================
modules/gui/qt/widgets/qml/BlurEffect.qml
=====================================
@@ -31,15 +31,4 @@ MultiEffect {
autoPaddingEnabled: false
property alias radius: effect.blurMax
-
- onChildrenChanged: {
- for (let i in children) {
- if (children[i] instanceof ShaderEffect) {
- // Qt creates multiple ShaderEffect, depending
- // on parameters. They support atlas/sub textures
- // but Qt does not set `supportsAtlasTextures`:
- children[i].supportsAtlasTextures = true
- }
- }
- }
}
=====================================
modules/gui/qt/widgets/qml/compat/BlurEffect.qml
=====================================
@@ -26,15 +26,4 @@ FastBlur {
// Avoid using padding, as Qt thinks that it needs to layering implicitly.
transparentBorder: false
-
- onChildrenChanged: {
- for (let i in children) {
- if (children[i] instanceof ShaderEffect) {
- // Qt creates multiple ShaderEffect, depending
- // on parameters. They support atlas/sub textures
- // but Qt does not set `supportsAtlasTextures`:
- children[i].supportsAtlasTextures = true
- }
- }
- }
}
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/858073faa680d8836b5dec9c3806f5fc5778352b...1908011fc1ce667c0650883603e23e6dc07ebd19
--
View it on GitLab: https://code.videolan.org/videolan/vlc/-/compare/858073faa680d8836b5dec9c3806f5fc5778352b...1908011fc1ce667c0650883603e23e6dc07ebd19
You're receiving this email because of your account on code.videolan.org.
VideoLAN code repository instance
More information about the vlc-commits
mailing list