summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorVito Caputo <vcaputo@pengaru.com>2023-06-14 18:20:04 -0700
committerVito Caputo <vcaputo@pengaru.com>2023-06-14 18:20:04 -0700
commitbdbfec4d72927892b4df8594e58efb6a53e481bb (patch)
tree04c67b64d8fa9d7c1267ec3072e12234a3fa16d3
parent8857d92c599efdb34b76f0311a2bf206ad8434e6 (diff)
modules/mixer: update T tap once in prepare_frame
It's problematic/racy to always be dereferencing the tap pointer to access it's current value. Running checkers:fill_module=mixer on a 2c/4t (n_cpus=4) machine was quite effective at crashing in mixer::render_fragment() due to dereferencing a NULL snapshot fragment. The "tween" T value is being used to indicate when interpolation of the snapshots is necessary. But repeatedly re-reading the T value via the tap pointer would race with the driving tap. In the case of checkers, which is a threaded module, the mixer contexts are all rendering in parallel at the same path so they're sharing a single pipe for their T taps. This situation of sets of contexts sharing a single path, resulting in their taps all landing on a single pipe, is still largely up in the air and might be actively prevented in the future. But until then, it can be made far less crashy and problematic by just being more careful about dereferencing the tap to access its latest value just once at the prepare frame stage, storing it in the local T variable in the context. Then all the render_fragment() accesses can at least find a stable value in the context from prepare-to-render, so at least the snapshots are there when they should be according to the T value etc. Something like this will probably need to be done regardless of what happens with the context sets sharing the same path. Since when a given tap isn't the driver, it still has to take care to just grab the updated value once for the frame... The tap API can't really automagically do that single update of the local variable when passenger though, since it's been deliberately kept devoid of all the type-specific accessor junk (ptr/elems etc are all void **/void* in the tap api side). Hence why this commit is just ad-hoc updating the local variable in the else branch; it's best positioned to do so directly. But there still needs to be more consideration for thread-safety.
-rw-r--r--src/modules/mixer/mixer.c23
1 files changed, 16 insertions, 7 deletions
diff --git a/src/modules/mixer/mixer.c b/src/modules/mixer/mixer.c
index 7de91e6..efa7de3 100644
--- a/src/modules/mixer/mixer.c
+++ b/src/modules/mixer/mixer.c
@@ -1,3 +1,4 @@
+#include <assert.h>
#include <math.h>
#include <stdlib.h>
#include <string.h>
@@ -110,10 +111,12 @@ static void mixer_prepare_frame(til_module_context_t *context, til_stream_t *str
if (!til_stream_tap_context(stream, context, NULL, &ctxt->taps.T))
*ctxt->T = cosf(ticks * .001f) * .5f + .5f;
+ else /* we're not driving the tap, so let's update our local copy just once */
+ ctxt->vars.T = *ctxt->T; /* FIXME: taps need synchronization/thread-safe details fleshed out / atomics */
switch (((mixer_setup_t *)context->setup)->style) {
case MIXER_STYLE_FLICKER:
- if (randf(&context->seed) < *ctxt->T)
+ if (randf(&context->seed) < ctxt->vars.T)
i = 1;
else
i = 0;
@@ -121,20 +124,23 @@ static void mixer_prepare_frame(til_module_context_t *context, til_stream_t *str
til_module_render(ctxt->inputs[i].module_ctxt, stream, ticks, &fragment);
break;
- case MIXER_STYLE_FADE:
- if (*ctxt->T < 1.f) {
+ case MIXER_STYLE_FADE: {
+ float T = ctxt->vars.T;
+
+ if (T < 1.f) {
til_module_render(ctxt->inputs[0].module_ctxt, stream, ticks, &fragment);
- if (*ctxt->T > 0.f)
+ if (T > 0.f)
ctxt->snapshots[0] = til_fb_fragment_snapshot(&fragment, 0);
}
- if (*ctxt->T > 0.f) {
+ if (T > 0.f) {
til_module_render(ctxt->inputs[1].module_ctxt, stream, ticks, &fragment);
- if (*ctxt->T < 1.f)
+ if (T < 1.f)
ctxt->snapshots[1] = til_fb_fragment_snapshot(&fragment, 0);
}
break;
+ }
default:
assert(0);
@@ -189,11 +195,14 @@ static void mixer_render_fragment(til_module_context_t *context, til_stream_t *s
break;
case MIXER_STYLE_FADE: {
- float T = *ctxt->T;
+ float T = ctxt->vars.T;
if (T <= 0.f || T >= 1.f)
break;
+ assert(ctxt->snapshots[0]);
+ assert(ctxt->snapshots[1]);
+
/* for the tweens, we already have snapshots sitting in ctxt->snapshots[],
* which we now interpolate the pixels out of in parallel
*/
© All Rights Reserved