blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 779 Projects 19 Wiki Activity

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

Merged
Aras Pranckevicius merged 8 commits from aras_p/blender:vse_filter_aa into main 2024-02-02 16:29:01 +01:00
Conversation 3 Commits 8 Files Changed 16 +18 -14
3 changed files with 18 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d331a3ce54 - Show all commits

6
source/blender/blenlib/BLI_simd.h
View File

@ -30,3 +30,9 @@
#else
# define BLI_HAVE_SSE2 0
#endif
#if defined(__SSE4_1__) || (defined(__ARM_NEON) && defined(WITH_SSE2NEON))
# define BLI_HAVE_SSE4 1
#else
# define BLI_HAVE_SSE4 0
#endif

10
source/blender/blenlib/intern/math_interp.cc
View File

@ -54,13 +54,9 @@ template<enum eCubicFilter filter> static float4 cubic_filter_coefficients(float
# include <smmintrin.h> /* _mm_floor_ps */
# endif
/* Functions below are hard to express before SSE4. If compiling to that
* or NEON via sse2neon, just use the simple forms. On SSE2, do it the
* hard way. */
BLI_INLINE __m128 floor_simd(__m128 v)
{
# if defined(__SSE4_1__) || defined(__ARM_NEON) && defined(WITH_SSE2NEON)
# if BLI_HAVE_SSE4
__m128 v_floor = _mm_floor_ps(v);
# else
/* Truncate, for negative inputs this will round towards zero. Then compare
@ -74,7 +70,7 @@ BLI_INLINE __m128 floor_simd(__m128 v)
BLI_INLINE __m128i min_i_simd(__m128i a, __m128i b)
{
# if defined(__SSE4_1__) || defined(__ARM_NEON) && defined(WITH_SSE2NEON)
# if BLI_HAVE_SSE4
return _mm_min_epi32(a, b);
# else
__m128i cmp = _mm_cmplt_epi32(a, b);
@ -86,7 +82,7 @@ BLI_INLINE __m128i min_i_simd(__m128i a, __m128i b)
BLI_INLINE __m128i max_i_simd(__m128i a, __m128i b)
{
# if defined(__SSE4_1__) || defined(__ARM_NEON) && defined(WITH_SSE2NEON)
# if BLI_HAVE_SSE4
return _mm_max_epi32(a, b);
# else
__m128i cmp = _mm_cmplt_epi32(b, a);

16
source/blender/imbuf/intern/transform.cc
View File

@ -338,8 +338,9 @@ template<eIMBInterpolationFilterMode Filter>
static void transform_scanlines_filter(const TransformContext &ctx, IndexRange y_range)
{
int channels = ctx.src->channels;
if (ctx.dst->float_buffer.data && ctx.src->float_buffer.data) {
/* Float images. */
/* Float pixels. */
if (channels == 4) {
transform_scanlines<Filter, float, 4>(ctx, y_range);
}
@ -353,8 +354,9 @@ static void transform_scanlines_filter(const TransformContext &ctx, IndexRange y
transform_scanlines<Filter, float, 1>(ctx, y_range);
}
}
else if (ctx.dst->byte_buffer.data && ctx.src->byte_buffer.data) {
/* Byte images. */
if (ctx.dst->byte_buffer.data && ctx.src->byte_buffer.data) {
/* Byte pixels. */
if (channels == 4) {
transform_scanlines<Filter, uchar, 4>(ctx, y_range);
}
@ -423,8 +425,8 @@ static void edge_aa(const TransformContext &ctx)
/* DDA line raster: step one pixel along the longer direction. */
delta /= length;
if (ctx.dst->float_buffer.data) {
/* Float image. */
if (ctx.dst->float_buffer.data != nullptr) {
/* Float pixels. */
float *dst = ctx.dst->float_buffer.data;
for (int i = 0; i < length; i++) {
float2 pos = ptA + i * delta;
@ -439,8 +441,8 @@ static void edge_aa(const TransformContext &ctx)
}
}
aras_p marked this conversation as resolved
Sergey Sharybin commented 2024-02-02 09:58:16 +01:00
Review
Copy Link

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
Review
Copy Link

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
Review
Copy Link

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
Review
Copy Link

Ah ok! Makes sense, will do.

Ah ok! Makes sense, will do.
}
else {
/* Byte image. */
if (ctx.dst->byte_buffer.data != nullptr) {
/* Byte pixels. */
uchar *dst = ctx.dst->byte_buffer.data;
for (int i = 0; i < length; i++) {
float2 pos = ptA + i * delta;