VSE: bilinear upscaling no longer adds transparent border around the image #117717

Aras Pranckevicius · 2024-02-01T11:26:17+01:00

2024-02-01 11:26:17 +01:00

Part of overall "improve image filtering situation" (#116980), this PR addresses two issues:

Bilinear (default) image filtering makes half a source pixel wide transparent border around the image. This is very noticeable when scaling images/movies up in VSE. However, when there is no scaling up but you have slightly rotated image, this creates a "somewhat nice" anti-aliasing around the edge.
The other filtering kinds (e.g. cubic) do not have this behavior. So they do not create unexpected transparency when scaling up (yay), however for slightly rotated images the edge is "jagged" (oh no).

As to why that happens, no one remembers the reasons. Ever since low level "bilinear/bicubic" interpolation functions were added back in 2007 (9298357670), they had different behavior in how a pixel that is right on the edge of an image is treated: bilinear was blending in transparent black, whereas bicubic was clamping samples to the image edge.

So what we do here is:

Add new set of bilinear functions that clamp samples to image edges, very similar to how cubic sampling functions do. And use these new functions in VSE transformations (i.e. IMB_transform).
- Keep the old set of functions too, just add border to their names to make it clear that they treat image as if it had a transparent black border around it. The old functions are still used in CPU compositor and a handful other places.
To address the "slightly rotated image edge is jagged" issue, do a cheap/fast/simple approximation inside IMB_transform, i.e. walk over the pixels right at the edges and adjust their transparency based on approximate coverage. This does not affect things that are not rotated and placed at non-fractional pixel positions. This is not correct coverage calculation, but is fast and quite simple (doing fully correct anti-aliased raster seems to be quite involved, judging by amount of code Skia has for it).

Here's throwing in an image into a VSE that is 6x larger resolution: note that black outline appears around the edges.

And here's the same with this PR:

And here's a more synthetic test that shows image upscaled in several ways, Bilinear on the left side, Cubic on the right side. Current main branch:

And with this PR: note that edge anti-aliasing is consistent between bilinear and cubic (whereas before "too much fading" was in bilinear case, and no anti-aliasing at all in cubic case). The gap between rotated but neighboring strips (top left) is much smaller too.

Note: buildbot will be failing compositor render tests (that I have very recently added), as expected. The images need to be updated together with merging of this PR.

Part of overall "improve image filtering situation" (#116980), this PR addresses two issues: 1. Bilinear (default) image filtering makes half a source pixel wide transparent border around the image. This is very noticeable when scaling images/movies up in VSE. However, when there is no scaling up but you have slightly rotated image, this creates a "somewhat nice" anti-aliasing around the edge. 1. The other filtering kinds (e.g. cubic) do not have this behavior. So they do not create unexpected transparency when scaling up (yay), however for slightly rotated images the edge is "jagged" (oh no). As to why that happens, no one remembers the reasons. Ever since low level "bilinear/bicubic" interpolation functions were added back in 2007 (9298357670f5), they had different behavior in how a pixel that is right on the edge of an image is treated: bilinear was blending in transparent black, whereas bicubic was clamping samples to the image edge. So what we do here is: * Add new set of bilinear functions that clamp samples to image edges, very similar to how cubic sampling functions do. And use these new functions in VSE transformations (i.e. `IMB_transform`). * Keep the old set of functions too, just add `border` to their names to make it clear that they treat image as if it had a transparent black border around it. The old functions are still used in CPU compositor and a handful other places. * To address the "slightly rotated image edge is jagged" issue, do a cheap/fast/simple approximation inside `IMB_transform`, i.e. walk over the pixels right at the edges and adjust their transparency based on approximate coverage. This does not affect things that are not rotated and placed at non-fractional pixel positions. This is *not* correct coverage calculation, but is fast and quite simple (doing fully correct anti-aliased raster seems to be quite involved, judging by amount of code Skia has for it). Here's throwing in an image into a VSE that is 6x larger resolution: note that black outline appears around the edges. ![](/attachments/c48f2d77-d381-4223-bbdc-8ee19ff0c417) And here's the same with this PR: ![](/attachments/ec431c42-9acc-4aa0-a8d3-d91b5d871a68) And here's a more synthetic test that shows image upscaled in several ways, Bilinear on the left side, Cubic on the right side. Current main branch: ![](/attachments/7b23bf55-5caa-4e2a-807a-451697dc816e) And with this PR: note that edge anti-aliasing is consistent between bilinear and cubic (whereas before "too much fading" was in bilinear case, and no anti-aliasing at all in cubic case). The gap between rotated but neighboring strips (top left) is much smaller too. ![](/attachments/3e969433-c994-4ec2-9933-8a36ae7df37c) Note: buildbot will be failing compositor render tests (that I have very recently added), as expected. The images need to be updated together with merging of this PR.

aa_02.png

70 KiB

aa_03.png

67 KiB

Aras Pranckevicius added 4 commits 2024-02-01 11:26:27 +01:00

VSE: add approximate edge anti-aliasing for rotated images 90ae1b1f24

Doing "fully properly correct" analytical edge anti-aliasing sounds
complicated and possibly a performance problem. So do an approximation,
i.e. literally just go over the edge pixels and adjust their opacity
based on approximate coverage.

Tests: lower VSE render test passing thresholds, defaults are quite high 85342c8f57

Added edge anti-aliasing clearly changed the edges of the results,
but due to "eh, that's less than 1% of all pixels" they were considered
as passing.

Cleanup: rename interpolate_bilinear to have "border" in the name c178460a76

So that it's clear that it treats the image as if having a black border
around it, for edge pixels. In prep for adding bilinear that does not
do that.

VSE: use bilinear sampling that does not have half a source texel fade to transparency around the edges

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

0f803ba6ff

Particularly when scaling things up, it created a border of transparency
around the source image.

Aras Pranckevicius changed title from ~~WIP: VSE: bilinear no longer adds an edge of transparency, instead all filters anti-alias one pixel edge~~ to WIP: VSE: bilinear upscaling no longer adds transparent border around the image

2024-02-01 11:26:40 +01:00

Aras Pranckevicius referenced this pull request

2024-02-01 11:43:13 +01:00

VSE: Improve image filtering defaults/options #116980

Aras Pranckevicius referenced this pull request

2024-02-01 11:43:36 +01:00

WIP: VSE: bilinear upscaling no longer adds transparent border around the image #117279

Aras Pranckevicius added 1 commit 2024-02-01 11:47:10 +01:00

Fix build/lint

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

4ed09a1c86

Aras Pranckevicius added this to the Video Sequencer project 2024-02-01 11:48:39 +01:00

Aras Pranckevicius commented

2024-02-01 12:01:21 +01:00

@blender-bot build

Aras Pranckevicius changed title from ~~WIP: VSE: bilinear upscaling no longer adds transparent border around the image~~ to VSE: bilinear upscaling no longer adds transparent border around the image

2024-02-01 12:25:08 +01:00

Aras Pranckevicius requested review from Sergey Sharybin 2024-02-01 12:25:23 +01:00

Aras Pranckevicius added 1 commit 2024-02-01 14:55:39 +01:00

Add non-border bilinear byte code path for fallback non-SSE 5f87655a62

This is not used on any of officially supported platforms, but
in case someone is building on something more exotic (PPC? RISC-V? etc.)
let it behave consistently.

Sergey Sharybin commented

2024-02-01 16:32:49 +01:00

This seems to be giving results closer what one would expect on user level!

Did you check if it has any measurable impact on the render time of VSE edits you have?

This seems to be giving results closer what one would expect on user level! Did you check if it has any measurable impact on the render time of VSE edits you have?

Aras Pranckevicius commented

2024-02-01 19:42:29 +01:00

Did you check if it has any measurable impact on the render time of VSE edits you have?

It does not. Both forms of bilinear sampling are pretty much the same performance, and the "walk 4 edges of lines for AA" is peanuts compared to everything else. And also I skip walking said lines if they are completely horizontal or vertical and placed at integer pixel positions (i.e. would not alter the edge at all), which for the edit data sets I have happens "all the time" since nothing is rotated.

> Did you check if it has any measurable impact on the render time of VSE edits you have? It does not. Both forms of bilinear sampling are pretty much the same performance, and the "walk 4 edges of lines for AA" is peanuts compared to everything else. And also I skip walking said lines if they are completely horizontal or vertical and placed at integer pixel positions (i.e. would not alter the edge at all), which for the edit data sets I have happens "all the time" since nothing is rotated.

Sergey Sharybin reviewed 2024-02-02 09:58:34 +01:00

source/blender/blenlib/intern/math_interp.cc

						
				@ -70,1 +74,4 @@

				BLI_INLINE __m128i min_i_simd(__m128i a, __m128i b)

				{

				#  if defined(__SSE4_1__) || defined(__ARM_NEON) && defined(WITH_SSE2NEON)

Sergey Sharybin commented

2024-02-02 09:52:59 +01:00

This check is coming from some older code, but it seems to be spreading out in a lot of other cases. The confusing part of it is the order of operations. Can we make them explicit, like defined(__SSE4_1__) || (defined(__ARM_NEON) && defined(WITH_SSE2NEON)) ?

This check is coming from some older code, but it seems to be spreading out in a lot of other cases. The confusing part of it is the order of operations. Can we make them explicit, like `defined(__SSE4_1__) || (defined(__ARM_NEON) && defined(WITH_SSE2NEON))` ?

Aras Pranckevicius commented

2024-02-02 10:30:37 +01:00

Added BLI_HAVE_SSE4 to BLI_simd.h and use that.

Added `BLI_HAVE_SSE4` to `BLI_simd.h` and use that.

aras_p marked this conversation as resolved

source/blender/imbuf/intern/transform.cc

						
				@ -362,0 +439,4 @@

				        }

				      }

				    }

				    else {

Sergey Sharybin commented

2024-02-02 09:58:16 +01:00

ImBuf can have both float and byte buffers. The commonly typical approach in the ImBuf's transfomration is to handle both cases:

if (ctx.dst->float_buffer.data) {
}
if (ctx.dst->byte_buffer.data) {
}

`ImBuf` can have both float and byte buffers. The commonly typical approach in the ImBuf's transfomration is to handle both cases: ``` if (ctx.dst->float_buffer.data) { } if (ctx.dst->byte_buffer.data) { } ```

Aras Pranckevicius commented

2024-02-02 10:00:18 +01:00

Hmm, a lot of existing code (at least within sequencer/transform) have different logic, i.e. it does "float buffer exists? do that, otehrwise do this". Or, put it different way, pretty much none of the code within sequencer is prepared to work with images that have both byte and float buffers.

Hmm, a lot of existing code (at least within sequencer/transform) have different logic, i.e. it does "float buffer exists? do that, otehrwise do this". Or, put it different way, pretty much _none_ of the code within sequencer is prepared to work with images that have both byte and float buffers.

Sergey Sharybin commented

2024-02-02 10:14:59 +01:00

Sequencer will do that because it has some strong opinion on what the source of truth of pixels is, and will ensure that the result does not have both float and byte buffers in an inconsistent states. Generic transform code in ImBuf does not know what the caller will consider that source of truth, so it applies transform on both buffers.

Some recent additions to the ImBuf seems to diverge from this, but I do not think it is that great of idea. It just leads to a silent inconsistency, which is likely to case problems later on. What is even more sad is that some of those additions will modify float buffer, and will not even tag byte buffer as dirty (it probably still would lead to incorrect result somewhere, but at least you'd have an indication that something is in fact out of date). If we say only one of those buffers can exist, then it needs to become a part of overall design.

Sequencer will do that because it has some strong opinion on what the source of truth of pixels is, and will ensure that the result does not have both float and byte buffers in an inconsistent states. Generic transform code in `ImBuf` does not know what the caller will consider that source of truth, so it applies transform on both buffers. Some recent additions to the `ImBuf` seems to diverge from this, but I do not think it is that great of idea. It just leads to a silent inconsistency, which is likely to case problems later on. What is even more sad is that some of those additions will modify float buffer, and will not even tag byte buffer as dirty (it probably still would lead to incorrect result somewhere, but at least you'd have an indication that something is in fact out of date). If we say only one of those buffers can exist, then it needs to become a part of overall design.

Aras Pranckevicius commented

2024-02-02 10:16:19 +01:00

Ah ok! Makes sense, will do.

aras_p marked this conversation as resolved

Aras Pranckevicius added 1 commit 2024-02-02 10:28:39 +01:00

Code review adjustments d331a3ce54

Aras Pranckevicius added 1 commit 2024-02-02 11:43:56 +01:00

Fix parentheses in edge AA "can this line be skipped" check ca8788d034

Sergey Sharybin approved these changes 2024-02-02 12:14:34 +01:00

Aras Pranckevicius referenced this issue from a commit

2024-02-02 16:29:01 +01:00

VSE: bilinear upscaling no longer adds transparent border around the image