Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugins (alphablend): Fix blending and associated crashes due to buffer overruns #383

Merged
merged 4 commits into from
Jan 25, 2025
Merged

Plugins (alphablend): Fix blending and associated crashes due to buffer overruns #383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b40b4f7
Plugins (alphablend): Fix calculation of alpha.
kaixiong Jan 25, 2025
6e6a705
Plugins (alphablend): Fix blending and associated crashes due to buff…
kaixiong Jan 21, 2025
5fe3eb1
Core (alpha-blend): Fix visual_alpha_blend_16() only blending half of…
kaixiong Jan 21, 2025
aba8a84
Core (alpha-blend): Rename `size` parameter of visual_alpha_blend_*()…
kaixiong Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 30 additions & 21 deletions libvisual-plugins/plugins/morph/alphablend/morph_alphablend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,17 @@
#include "config.h"
#include "gettext.h"
#include <libvisual/libvisual.h>
#include <stdlib.h>

VISUAL_PLUGIN_API_VERSION_VALIDATOR

static int lv_morph_alpha_init (VisPluginData *plugin);
static void lv_morph_alpha_cleanup (VisPluginData *plugin);
static void lv_morph_alpha_apply (VisPluginData *plugin, float progress, VisAudio *audio, VisVideo *dest, VisVideo *src1, VisVideo *src2);

static inline void alpha_blend_buffer (uint8_t *dest, uint8_t *src1, uint8_t *src2, int size, int depth, float alpha);
typedef void (*BlendFunc) (uint8_t *, const uint8_t *, const uint8_t *, visual_size_t, uint8_t);

static BlendFunc get_blend_func (VisVideoDepth depth);

const VisPluginInfo *get_plugin_info (void)
{
Expand Down Expand Up @@ -80,33 +83,39 @@ static void lv_morph_alpha_cleanup (VisPluginData *plugin)

static void lv_morph_alpha_apply (VisPluginData *plugin, float progress, VisAudio *audio, VisVideo *dest, VisVideo *src1, VisVideo *src2)
{
alpha_blend_buffer (visual_video_get_pixels (dest),
visual_video_get_pixels (src1),
visual_video_get_pixels (src2),
visual_video_get_size (dest),
visual_video_get_depth (dest),
progress);
int width = visual_video_get_width (dest);
int height = visual_video_get_height (dest);
int depth = visual_video_get_depth (dest);
int pitch = visual_video_get_pitch (dest);

uint8_t *src1_row_ptr = visual_video_get_pixels (src1);
uint8_t *src2_row_ptr = visual_video_get_pixels (src2);
uint8_t *dest_row_ptr = visual_video_get_pixels (dest);

uint8_t alpha = progress * 255;
kaixiong marked this conversation as resolved.
Show resolved Hide resolved
BlendFunc blend_func = get_blend_func (depth);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_blend_func can return NULL. If we keep allowing it to, we likely need a check for NULL somewhere around here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default case is unreachable and largely included to silence compiler warnings about enum values unaccounted for (e.g. VISUAL_VIDEO_DEPTH_NONE and VISUAL_DEPTH_NONE_ALL).

We don't have unreachable() in C99 or C11 so I returned a NULL instead. Would you prefer abort()?

Copy link
Member

@hartwork hartwork Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaixiong I would favor some flavor of abort or assert — something with a message —, yes, maybe using libvisual's log_and_exit? (Maybe we need one more check for that for lv_checks.h.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartwork I've added a critical log message followed by abort().

I spent some time thinking about the general approach of a portable unreachable() function that defaults to log-and-abort in debug builds... but I've ultimately concluded that for now, we should keep the change small for this PR 😄


for (int y = 0; y < height; y++) {
blend_func (dest_row_ptr, src1_row_ptr, src2_row_ptr, width, alpha);
src1_row_ptr += pitch;
src2_row_ptr += pitch;
dest_row_ptr += pitch;
}
}

static inline void alpha_blend_buffer (uint8_t *dest, uint8_t *src1, uint8_t *src2, int size, int depth, float alpha)
static BlendFunc get_blend_func (VisVideoDepth depth)
{
uint8_t a = alpha * (1/255.0);

switch (depth) {
case VISUAL_VIDEO_DEPTH_8BIT:
visual_alpha_blend_8 (dest, src1, src2, size, a);
break;

return visual_alpha_blend_8;
case VISUAL_VIDEO_DEPTH_16BIT:
visual_alpha_blend_16 (dest, src1, src2, size, a);
break;

return visual_alpha_blend_16;
case VISUAL_VIDEO_DEPTH_24BIT:
visual_alpha_blend_24 (dest, src1, src2, size, a);
break;

return visual_alpha_blend_24;
case VISUAL_VIDEO_DEPTH_32BIT:
visual_alpha_blend_32 (dest, src1, src2, size, a);
break;
return visual_alpha_blend_32;
default:
visual_log (VISUAL_LOG_CRITICAL, "Unsupported depth for blending (%d)", depth);
abort ();
}
}
17 changes: 8 additions & 9 deletions libvisual/libvisual/lv_alpha_blend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,31 +16,30 @@ typedef struct {

#pragma pack()

void visual_alpha_blend_8 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t size, uint8_t alpha)
void visual_alpha_blend_8 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t n, uint8_t alpha)
{
simd_interpolate_8 (dest, src1, src2, alpha, (int) size);
simd_interpolate_8 (dest, src1, src2, alpha, (int) n);
}

void visual_alpha_blend_16 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t size, uint8_t alpha)
void visual_alpha_blend_16 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t n, uint8_t alpha)
{
rgb16_t *destr = (rgb16_t *) dest;
rgb16_t *src1r = (rgb16_t *) src1;
rgb16_t *src2r = (rgb16_t *) src2;
visual_size_t i;

for (i = 0; i < size / 2; i++) {
for (visual_size_t i = 0; i < n; i++) {
destr[i].r = (alpha * (src2r[i].r - src1r[i].r)) / 255 + src1r[i].r;
destr[i].g = (alpha * (src2r[i].g - src1r[i].g)) / 255 + src1r[i].g;
destr[i].b = (alpha * (src2r[i].b - src1r[i].b)) / 255 + src1r[i].b;
}
}

void visual_alpha_blend_24 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t size, uint8_t alpha)
void visual_alpha_blend_24 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t n, uint8_t alpha)
{
simd_interpolate_8 (dest, src1, src2, alpha, (int) size * 3);
simd_interpolate_8 (dest, src1, src2, alpha, (int) n * 3);
}

void visual_alpha_blend_32 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t size, uint8_t alpha)
void visual_alpha_blend_32 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t n, uint8_t alpha)
{
simd_interpolate_8 (dest, src1, src2, alpha, (int) size * 4);
simd_interpolate_8 (dest, src1, src2, alpha, (int) n * 4);
}
8 changes: 4 additions & 4 deletions libvisual/libvisual/lv_alpha_blend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,10 @@

LV_BEGIN_DECLS

LV_API void visual_alpha_blend_8 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t size, uint8_t alpha);
LV_API void visual_alpha_blend_16 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t size, uint8_t alpha);
LV_API void visual_alpha_blend_24 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t size, uint8_t alpha);
LV_API void visual_alpha_blend_32 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t size, uint8_t alpha);
LV_API void visual_alpha_blend_8 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t n, uint8_t alpha);
LV_API void visual_alpha_blend_16 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t n, uint8_t alpha);
LV_API void visual_alpha_blend_24 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t n, uint8_t alpha);
LV_API void visual_alpha_blend_32 (uint8_t *LV_RESTRICT dest, const uint8_t *LV_RESTRICT src1, const uint8_t *LV_RESTRICT src2, visual_size_t n, uint8_t alpha);

LV_END_DECLS

Expand Down