blur: expand partial render region by one pixel

[dmrlsn]

Fixes #3113.

Partial blur rendering only writes the current blur_region into the
intermediate buffer. A following blur pass can sample one pixel outside
that region, leaving the edge dependent on previous or undefined buffer
contents.

Expand the region by one pixel before scissoring and clip it to the
intermediate buffer size.

Tested on current master (4c34a7bf) on an Apple M2 with Asahi/Mesa
26.1.6. The magenta edge described in #3113 disappears with this change.

Also tested with Kawase, Gaussian and Box blur.

[ammen99]

I wonder whether the issue won't be fixed if we use floating-point regions here (and convert to integers right before scissoring). Do you think that could fix the issue?

[dmrlsn]

I had already tried variations of this direction during the initial
debugging, around where the damage region gets scaled and converted to
integer coordinates, without fixing the artifact.

I repeated the test more directly now by keeping the blur damage as
regionf_t through the framebuffer mapping, translation, degradation
scaling and blur passes, and only converting to a containing integer
box immediately before scissoring.

The scale 1.6 / kawase_degrade 10 case is still bad. The magenta edge
is unchanged.

This is consistent with the earlier diagnostics. The issue is not just
where the region is quantized. A blur pass can sample texels outside
the area written by the previous partial pass. Moving the integer
conversion later can change the exact region boundaries, but it does
not make those sampled texels initialized.

Expanding the partial render region by one pixel does.

I also have a separate, larger rendering regression involving
floating-point geometry. I will report that separately once this issue
is resolved.

[ammen99]

@dmrlsn I am not sure I understand though why the expand by 1 fixes the issue (or what the issue actually is). The current algorithm already assumes that the region outside of the damaged region is black / invalid colors - this is why we add padding, blur a larger region (where we know the parts near the edges will be wrong) and then replace the 'wrong' parts with the old contents (see blur.cpp).

[ammen99]

That is not to say that we don't have to expand the region, I am simply trying to understand why the current compensation mechanism is failing and we need more workarounds.

[dmrlsn]

I think the compensation in blur.cpp and the issue here are at two
different levels.

The code in blur.cpp handles the boundary of the scene damage. It
pads the damage, saves the extra area, renders the larger region, and
restores the extra pixels afterwards.

What I am seeing happens inside the intermediate blur pass chain.

In the failing case I measured an intermediate buffer of256x162,
while the partial blur region had extents y=1..162. So row 0 is part
of the allocated intermediate texture,but that pass never writes it.

A following blur pass can still sample row 0 while producing pixels
inside the region that is being kept.At that point the undefined value
has already propagated into pixels which are not part of the outer
area restored by saved_pixels.

I tested this directly by modifying that omitted row. If I fill it
with red before the next pass, the resulting edge becomes red on both
the M2 and NVIDIA. If I leave it untouched,Asahi/M2 with compressed
textures shows magenta there, while NVIDIA and Asahi/M2 with
compression disabled read it as zero. Rendering the whole intermediate
region also fixes the artifact.

So the problem is not that pixels outside the padded scene region are
invalid. The sampled pixel is inside the allocated intermediate
texture,but outside the area written by the previous partial pass.

The one-pixel expansion makes that intermediate texel get written
before it can be sampled. expanding by two pixels also works, but one
is enough for the failing cases I tested.

[soreau]

Does it work if this code is gated with a conditional that is only executed if degrade != 0?

[dmrlsn]

'degrade' cannot be zero here - all blur degrade options have a
minimum value of 1, and it is also used as a divisor when preparing
the blur buffer

if you meant degrade != 1, I don't think that would be appropriate
either.A degrade value of 1 only disables the initial downscaling. The
intermediate blur passes still run and can sample outside the area
written by the prvious pass.

Since the expansion is done inside render_iteration(), it already
only applies when an intermediate blur pass is actually rendered.

[soreau]

I think this is another example of what types of bugs might crop up if using a 'bad' output scale value.

@ammen99 I thought the plan was to snap to 'good' output scale values to sidestep off-by-one issues like these?

[dmrlsn]

Yes, scale 1.6 is one of the alignments which exposes the problem, but
frankly speaking I don't think snapping the output scale addresses
the underlying issue.

For example, with the same setup:

• scale 1.5, degrade 10: good
• scale 1.6, degrade 10: bad
• scale 1.6, degrade 9: good
• scale 2.0, degrade 10: good

So changing either the output scale or the degradation factor can move
the partial region boundary enough to hide or expose it.

More importantly, I tested the omitted intermediate row
directly. Filling that row with red before the following pass makes
the resulting edge red on both the M2 and NVIDIA. This shows that the
following blur pass samples a texel which was not written by the
previous partial pass.

Snapping to a different scale may avoid a particular alignment, but it
does not guarantee that the sampling footprint of one pass is initialized
by the previous pass. The one-pixel guard band does.

[soreau]

I will spin the CI on this since the change here isn't really computationally expensive and a potential improvement. @ammen99 ultimately has the decision though.

[dmrlsn]

I recorded a short direct capture to make the intermediate-buffer
behavior easier to see, and I am leaving it here as a reference for
future investigation of this issue.

This is one continuous recording from the apple M2/asahi system
where the issue reproduces. The nested wayfire instance is restarted
between the three runs with the same geometry and blur settings.

Stock shows the magenta edge. Filling the omitted intermediate pixels
with red makes the same edge red. Expanding the partial blur region by
one pixel removes it.

In the failing run the intermediate target is 256x162 and the partial
region starts at y=1, leaving row 0 unwritten.

Since this happens in wayfire's intermediate blur rendering rather
than in the client, wayfire on apple silicon/asahi can be affected
whenever the geometry, damage and degradation alignment leaves such
pixels unwritten. Other drivers may hide thesame bug if unwritten
texture contents happen to read as zero.

wayfire-blur-partial-repaint-asahi-m2.mp4

[soreau]

@dmrlsn Since you are able to reproduce this issue, I am curious to know if adjusting alpha_threshold [blur] option to 0.25 helps at all.

[ammen99]

@dmrlsn Since you are able to reproduce this issue, I am curious to know if adjusting alpha_threshold [blur] option to 0.25 helps at all.

Very unlikely, this is about the internal blur algorithm, not how blur interacts with transparent areas.

[ammen99]

@dmrlsn I think I am starting to understand the issue. I assume it is because of these samples here:

wayfire/plugins/blur/kawase.cpp

Lines 29 to 32 in 4c34a7b

	sum += texture2D(bg_texture, uv - halfpixel.xy * offset);
	sum += texture2D(bg_texture, uv + halfpixel.xy * offset);
	sum += texture2D(bg_texture, uv + vec2(halfpixel.x, -halfpixel.y) * offset);
	sum += texture2D(bg_texture, uv - vec2(halfpixel.x, -halfpixel.y) * offset);

This makes me wonder whether expanding by 1 pixel would be enough if you had blur/kawase_offset set to for example 3? I imagine you might need to expand the damage by a few more pixels in this case.

Also, is this bug limited to the kawase algorithm? If so, we could expand the damage before calling render_iteration, ie. here https://github.com/WayfireWM/wayfire/blob/master/plugins/blur/kawase.cpp#L106 and/or here https://github.com/WayfireWM/wayfire/blob/master/plugins/blur/kawase.cpp#L125

[dmrlsn]

I tested alpha_threshold=0.25 with the same 2560x1664 /scale 1.6 /
kawase_degrade 10 setup. It does not help. Stock still shows the
magenta edge, red poison still makes the edge red, and the +1 guard
still removes it.

I also tested the larger kawase offset you suggested:

kawase_degrade=10, kawase_iterations= 2, kawase_offset=3.0

Stock still reproduces the artifact and the one-pixel guard still
removes it.

The issue is not limited to kawase either. I can reproduce the same
stock / guard behavior with:

gaussian_degrade=3, gaussian_iterations=10, gaussian_offset=2.0

and with:

box_degrade=3, box_iterations=10, box_offset= 2.0

In both cases stock shows the magenta edge and the +1 guard removes
it.

so moving the expansion into kawase.cpp would not cover the gaussian
and box cases. Keeping it in the common render_iteration() path
seems more appropriate to me.

For the current reproducer, +1 is also sufficient with kawase_offset=
3.0. I have not tried to establish that one pixel is sufficient for
every possible geometry and blur parameter combination though.

[ammen99]

Well, I am still not quite certain why the other suggestions didn't work out but I believe that this is a genuine issue, if this really improves blur then I don't see why we shouldn't merge it. And we have provided enough context in this PR so that if other people have similar issues in the future, they know where to look at. Thanks for the explanations and for this PR!