| Age | Commit message (Collapse) | Author |
|---|
|
Returning the failed desc was just a lazy half-assed thing that
was sort of the best option in the simpler, pre-paths world.
But now that everything has paths thanks to recursive settings,
let's just return the path to the failed setting's desc.
This conveniently gets rid of a UAF bug when setup returned the
setting->desc as the failed desc, and main would print the desc
*after* freeing all the settings in its final moments.
But the best part is now more of the errors parsing settings
should be accompanied by an illuminatingly relevant setting path.
Previously you'd at best get a bare key from the desc, but often
no failed desc was returned at all and you saw no guidance at
all. But with the recent improvements to the setup error
handling I think those cases should be few if not entirely
eliminated.
|
|
tired of this polluting `git status`
|
|
It was too coarse a condition filtering the list on !name.
This change actually looks up non-NULL names to verify it's a
resolvable module name before producing the unfiltered list.
Mostly just to prevent fat-fingered manual module inputs from
resulting in unexpectedly larger and potentially dangerous module
lists in interactive "invalid input" retries. Observed in the
WIP rkt_scener stuff...
|
|
It just happened to be tolerable that the first setting in these
randomized settings instances wasn't ever getting described...
due to the ad-hoc nature of how rtv used them.
But in preparation for rkt_scener which will have "randomize
scene" functionality, as well as interactive (re)configuring of
the scenes, these settings instances need to be more correct to
not break things.
It seems awkward to be duplicating this "Renderer module" desc
here however. But I just want to make progress on the scene
editor for rkt, I suspect this will be revisited in the future.
On a high-level it feels like til_module_setup() should be the
thing producing that desc, and the randomizer just picking the
random settings through that setup func.
|
|
With the "ref" builtin module established and seeming to work
well enough, it looks unlikely we'll need access to unresolvable
module names in the contexts.
The thinking originally was that these names might have special
syntax making them more generic. E.g something like
"@/module/rkt/scenes/[1]/drizzle" for a module_name would have
been supported, which would get resolved either at context create
or even later (as in the ref builtin) at render time.
But the ref builtin is using a path setting, so module names just
stay module names.
Maybe in the future a special syntax will be added for brevity
reasons, but it does make the code more complicated vs. module
names just being names and resolving them entirely at setup time.
Anyhow, this commit does away with the module_name in the
context's scenes. You can still access it via
til_module_context_t.module.name anyways... it's basically just a
resolution-of-name-to-context time constraint that's being
codified now.
|
|
This is just temporary until all the setup_funcs always return a
res_setting on -EINVAL.
Currently they only cover this requirement when res_setup is
NULL...
|
|
fix desc leak
|
|
In the interests of improving error handling of interactive
setups, return the setting that was invalid when setup returns
-EINVAL.
For now this is only supported for non-finalized/non-baking setup
calls, i.e. (!res_setup).
In the future I want this also supported for finalizing when
res_setup is set. A lot of the -EINVAL returns are actually
during that stage (or need to be added) due to that being when
the more sensitive parsing occurs going from strings to native
numeric types and such.
The main reason it's not already being done in this commit is
it'll churn quite a bit to get there, since the setup funcs don't
generally have the setting pointers onhand at that phase. It'll
require changing the string-value-centric local variables to
instead all be the til_setting_t* holding those values.
And if setup funcs aren't going to be so value-centric, the
settings API will likely change to not even return the values
directly anymore and only return the full-blown til_settings_t as
the main return pointer, perhaps rid of their res_setting use
even.
Anyway, while here I did some random little cleanups too.
|
|
There was only til_setting_desc_fprint_path() (for
setup_interactively()), but for rkt_scener this stuff has to go
out the socket and the queueing mechanism is built around
til_str_t so let's get a til_str_t-centric variant in there.
|
|
This was a UAF bug when the context being freed contained a
driving tap... just careless placement of the untap call.
It needs to occur before destroying the context, because module
contexts are often used to store the til_tap_t instances.
|
|
When setting up a nested module, the HERMETIC modules shouldn't
be considered as eligible nor should one be the preffered option
(rtv).
This uses the presence of a parent on the settings as a heuristic
for if it's a nested scenario (no parent == root). When nested
the default changes to "compose" from "rtv", and HERMETIC modules
are omitted from the listed values.
|
|
This was assuming non-NULL res_setup which would assert in
til_module_setup_finalize() which doesn't expect a NULL
res_setup since that's its entire purpose.
It's not a bug that was actually triggered by any callers (yet)
|
|
trivial change
|
|
I get why I called this seed at the time as it was the starting
string of a potentially larger buildup... but it's just
ambiguous naming with the other more descriptive uses of seed in
the tree, and isn't even really appropriate.
|
|
In rkt_scener it's desirable to fully construct a new settings
instance *before* wiring it into the parent rkt/scenes settings.
But the way these positional settings labels get constructed
depends on the parent's settings entries. So it becomes a
chicken-egg problem to require the proper label @ creation before
its containing setting is even added in the parent's entries.
With this change a temporary WIP label can be used while
constructing the settings instance, then amend it once we know
the settings instance is complete and ready to join the party.
|
|
Introduce til_settings_get_parent(), impetus being so
til_module_setup() can infer if it's hermetic-appropriate or not.
|
|
It's more ergonomic more often to behave consistently with
strlen() here, plus it's just the established mental model.
While here I made til_settings_path_as_buf() private as nothing
external uses it, it's essentially just a logically distinct
private helper function from the public wrappers around it.
Dragged into this changeset due to clarifying some
naming/semantics as it's one of the few til_str_to_buf() callers.
But nobody was actually passing a non-NULL res_bufsz/res_len to
it anyways, as its use is minimal.
|
|
Helper for trimming off a trailing CRNL or NL if present
Clearly I once knew perl if this is the name that came to mind
|
|
You can't just reuse the ap in multiple calls to vsnprintf
without restarting... fixed in the obvious way
|
|
Make this a distinct heap allocation so it can be enlarged when
editing the scenes... (preparatory commit for scenes editing)
|
|
Preparatory commit for adding an interactive scene editing server
of sorts. It'll go in a separate listing, but needs these types
as it'll operate on rkt_context_t->scenes[].
|
|
I think this got messed up in the shift to libtil
|
|
See previous commits re: drm_fb/mem_fb
|
|
Let's some a more user-friendly ways of exiting...
|
|
til_fb_fragment_slice_single() and indirectly
til_fragmenter_slice_per_cpu() could get into infinite loops when
slicing small fragments into many slices.
This became more likely with commit a2f7397 which increased
per_cpu slice counts by 16X, which is how I tripped over it
running rtv. A checkers,size=8,fill_module=moire sent things
spinning...
This commit prevents it in the obvious manner.
|
|
See previous commit re: mem_fb
|
|
Snapshotting made page alloc/free more frequent, and
modules/mixer really hammers on snapshots in fade mode... drizzle
as well, but mixer snapshots both module outputs per frame.
So by keeping these around the reclaimed page snapshots can be
held onto at within the fb as spares for quicker allocation. In
practice it should just hit a high water mark of a working set
for spare pages, once in a steady state, assuming snapshots
aren't leaking.
Future commits will replicate this change in {drm,sdl}_fb
|
|
Slight improvement of CPU utilization for fragmenters using this
strategy...
I noticed tile64 would give better FPS in some scenarios where it
seemed obvious slice_per_cpu() was the appropriate option. And
that turned out to just be by virtue of being able to give idle
threads something to do while busy ones finished what was on
their plate.
So just make the slices a bit more granular than n_cpus... this
may have to be revisited in the future to find the sweet spot,
and may need to be more sophisticated than just multiplying by a
constant factor.
|
|
I had benchmarked this change and it showed no difference at all
on my 2c/4c i7 X230.
But having just tried it on an RPi4B where it moved the test case
from 54FPS to 60FPS, a +10% improvement, it's worth the
readability loss.
It's interesting how Intel's cleverness discourages optimizing in
ways that benefit probably *all* the competition... even when the
optimization is such a minor change in terms of effort.
|
|
Major gain comes from eliminating the cosf() from the inner loop...
There's still a bunch left on the table for moire but even just
these changes turn 19FPS into 81FPS over here for:
'--module=compose,layers=moire\\\,centers\\\=2\,moire\\\,centers\\\=2\,moire\\\,centers\\\=2\,moire\\\,centers\\\=2,texture=none' '--video=mem,size=1366x768'
|
|
This was written when module names were going to have an
@/path/to/context "handle" syntax. But instead I went the "ref"
builtin module route, with path=/path/to/context as a setting.
While it's more verbose in the settings, it "just works"
everywhere that can take a module+settings because the ref
builtin is just another module like any other.
So this TODO is referring to something that won't happen in a
"ref" builtin world.
|
|
This conditionally will end the stream on scene 99999 if
connect=off (playback mode)
When connect=on it'll just make it show an "EXIT SCENE"
diagnostic instead of the "NO SCENE" message.
Now you just stick 99999 in the rkt:scene track to end the show.
It's assumed 99999 scenes will never be needed...
|
|
I can't see this staying so simple for very long, but for now
this at least enables making rototiller-based rkt-sequenced demos
that exit gracefully when they're finished.
In a future where rtv may play embedded rkt configs+tracks, it
needs a way to detect the end of stream without making main exit.
But I'll cross that bridge when I get there...
|
|
rkt needs a way to signal the end of a sequence, this will
probably get more work in the future but something simple is fine
for now
|
|
This probably needs more work... it seems wrong to be bypassing
the taps altogether when dt is 0
|
|
And just maintain it as the last ticks value after rendering with
the context...
A couple modules were already doing this manually in an ad-hoc
fashion, just make it a general thing.
Updated those modules to reflect the new situation
Especially in a rkt world with modules::mixer doing fades, it
becomes common to render the same context twice in the same frame
for the blending. We need to prevent accelerated animations in
such situations. For now let's just rely on ticks in a delta-T
fashion to prevent animating the context when ticks is the same.
modules::stars in particular needs this fixed up, upcoming commit
|
|
I'm not sure if this will stay long-term, but it seems not be
disrupting linux builds and with open_memstream all gone now
mingw is succeeding with this present.
It would really make sense to simply replace all rand_r() usage
with a libtil rand_r equivalent. That way it can guaranteen
consistent behavior across platforms...
|
|
This moves the tap updating to a function shared by rendering and
context create... so we can have a valid externally-driven tap
value before rendering a single frame if possible. (this is
important for not having spurious frames/flickers in rkt
sequences)
|
|
It's problematic getting this stuff online at render time,
because the modules end up rendering with uninitialized tap
values in that first frame.
With this change modules can get their taps on-stream at context
create, and we'll notice and do the initial rocket update just
before returning the rkt context to prime everyone on-stream.
|
|
This really needs SIMD to fly on-cpu, but this improves things
some.
Using `--module=mixer,style=fade,a_module=roto,b_module=roto\
--video=mem,size=1366x768 --defaults --go` to test:
Before FPS: 92-95 floating mostly around 94-95
After FPS: 107-111 floating mostly around 108-109
so +14.8% FPS
(2c/4t i7 X230)
|
|
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.
|
|
this was breaking mixer as checkers::fill_module when the
checkers were centered via shifting on the Y axis
|
|
Maybe earlier versions used the absolute coordinates in the
frame, but the current code doesn't make use of this and simply
needs to confine itself into the WxH of the fragment.
|
|
This should have been in there already
|
|
This is not optimized at all and tends to hurt the FPS
significantly. This is one of those things that would hugely
benefit from SIMD, but even without SIMD it could be done better.
I just slapped together something obvious for now, as I'd like to
focus more on the rkt side but need a better fader for scene
transitions than style=flicker.
Also changed {a,b}_module= preferred values to blank,compose
so you see something happen if you just run --defaults.
Otherwise, the compose,compose would just fade between two
identical compositions invisibly.
|
|
Originally I was thinking a variadic inputs= like compose::layers
would be desirable, with the T value's integer serving as an
index into the inputs, and the fraction between the integers as
the mixing T.
But I changed my mind and am instead constraining mixer
explicitly to two distinctly named modules; a_module= and
b_module= with the T value 0-1 mapping to a-b.
|
|
Not sure why this was -ENOMEM, module lookups don't allocate
anything...
|
|
Commit 24911280 made til_setup_t required, and most setup code
has already adopted til_module_setup_finalize() added in
c94e4683 for this purpose.
But til_module_setup()'s fall-through case when there's no
til_module_t.setup() on the module being setup was missed. This
broke running simple modules without a .setup() directly like
--module=roto, oops.
Fix is simple.
|
|
I thought the build was already using -Wall but that seems to not
be the case, maybe got lost somewhere along the line or messed up
in configure.ac
After forcing a build with -Wall -Werror, these showed up.
Fixed up in the obvious way, nothing too scary.
|
|
Similar to fill= and fill_color=, these influence how to treat
the "clear" cells.
Until now "clear" cells would unconditionally just be cleared via
til_fb_fragment_clear(). Now they can also be filled
w/color,sampled,textured, and maybe in the future a clear_module=
will also be introduced.
Note that for now I've left the clear_color randomizer disabled,
see code comments.
Having this setting also makes me wonder if the "filled" cells
should be optionally cleared using the "clear" setting first.
Imagine a scenario where you have fill_module=shapes, and you
want the color around the shape to match the clear color, for
instance... Especially once you can pass a color down to the
fill_module, controlling these things becomes more critical.
There's definitely more work to do here.
|
