* reset local start time at each recording boundary
* include the final frame's time when converting from wall to media time
Together at 30 fps these caused about 15 seconds of drift over 4 hours.
As noted in the changelog, this problem caused restarts to take a long
time, and the SQLite database to (slowly) grow without bound.
The problem was simple: the directory worker incorrectly aborted
`collect_garbage` if nothing was watching the result. For some
operations (e.g. reading from sample files) this is the correct
behavior, but in this case it contradicts the `Pool::collect_garbage`
contract.
`v0.7.26` introduced a busy loop error when the dir pool was shutting
down with multiple streams. This commit tests and fixes this scenario.
Also fixes a second problem in dropping a writer without an explicit
close would panic in debug mode about using (dropping) the blake3 hasher
while holding a lock. The check is meant to detect slow operations while
holding a lock. The panic wouldn't happen in release mode and probably
isn't a significant source of slowness, but fix it anyway.
This puts a lot of state into a new `LockedStream` that was formerly
guarded more coarsely by the recording lock or more granularly by
`RecordingToInsert` locks. And it restructures `LockedDatabase::flush`
to avoid holding this lock while performing SQLite operations.
Streamers write only into a recent frames buffer in RAM, which the
directory pool workers asynchronously write to disk approximately once
per GOP, where formerly there was a write call per frame.
Before, streamers could fall behind due to slow sample file directory
disk I/O calls or if writing got stuck in a retry loop, for example if
out of disk space. If Moonfire falls too far behind in receiving,
cameras tend to disconnect or (sadly) mis-frame messages because they do
not handle this case well. This removes a confusing potential cause of
such errors.
In the new model, streamers never wait for disk I/O. Frames are buffered
up to a limit. Recordings are aborted if the I/O pool falls too far
behind. There's no retry loop.
This roughly halves CPU usage as thread hand-offs and syscalls are less
common. It also enables several desirable future changes:
* audio recording (which radically increases the number of frames and
thus would otherwise significantly increase CPU usage).
* "monitor mode" (allowing live view without recording to disk),
* further CPU reductions for live view (by skipping all SQLite ops and
disk I/O).
In the new model, streamers also rarely wait for SQLite database
operations either—now only on video parameter changes. That might go
away in an upcoming change too.
The "syncer" (which synced files/dirs and kept track of what has to be
flushed into the SQLite database when) has mostly gone away too. The
directory pool workers handle the syncing now, and then they notify
a new "flusher" for the latter portion.
I'd like to go further still and have the database as a whole be owned
by a dedicated thread, the only one that uses the SQLite connection.
This prevents the tokio threads from blocking on SQLite operations.
I tried out doing so before making the streamers less dependent on the
db, but the extra thread handoffs were awful for performance.
Now, all IO happens on each directory's two-thread pool with threads
named `dir-<PATH>`. Formerly, IO could happen on the per-stream writer
threads, per-directory reader threads, and occasional on even the tokio
worker threads.
The recording path uses more CPU than before due to extra thread
hand-offs. That will change in the next commit, which will radically
change the approach to writing.
This centralizes a major source of `.unwrap()` throughout the code,
and one that would otherwise grow with upcoming changes. The new
error message should be more clear.
`slices::Slices::get_range` was too closely following the example of
`http_serve::Entity::get_range`:
The latter is once-per-request, so just boxing is low-cost and makes
sense to easily avoid monomorphization bloat when there are potentially
many types of entity streams in one program. In Moonfire, it's used with
different streams defined in the `moonfire_nvr::web::mp4`,
`moonfire_nvr::bundled_ui`, and `http_serve::file` modules. Putting them
all into a single boxless enum would be a pain. In particular, the last
one is not a nameable type today and would need more generic parameters
to implement the caller-demanded `Entity` definition.
The former is once-per-slice, there are tons of slices per request, and
it's easy to define a two-case enum right where it's needed. So the
trade-off is quite different.
Also fix up some out-of-date comments.
There's no transcoding, so if the browser/player doesn't support these
formats, they don't play. But it will record and allow downloads, and
it seems to be working with Chrome on macOS at least.
Fixes#33
This is a step toward H.265 recording support (#33) by using Retina
for the `VisualSampleEncoding` instead of the prior H.264-specific
logic. The only further change to Moonfire itself needed for H.265 is to
add `| "h265"` to the check for the right video stream.
This also adds support for recording MJPEG, although browser playback
almost certainly won't work.
It's cleaner anyway to use `tokio::broadcast::channel` than the list
of callbacks.
Also make it send pings only on long pauses between frames, as when
the camera is disconnected.
Users are often puzzled why there are short recordings. Previously
the only way to see this was to examine Moonfire's logs. This should
be a much better experience to find it right in the UI where you're
wondering, and without the potential the logs are gone.
Fixes#302
HTTP requests were only returning the error message to the caller, not
logging locally. In most cases the problem could be understood
client-side, but there are some exceptions. E.g. if Moonfire returns
a 403 on WebSocket update, even in the Chrome debug tools's network
tab the HTTP response body seems to be unavailable. And in general,
it's nice to have more context server-side.
Logging a `response::Body` isn't practical (it could be a stream), so
convert all the web stuff to use `base::Error` err returns.
Convert the `METHOD_NOT_ALLOWED` paths to return `Ok` for now. This is a
bit lame but punts on having some way of plumbing an explicit/overridden
status code in `base::Error`, as no gRPC error kind cleanly maps to
that.
Also convert `db::auth`, rather than making up an error kind in the web
layer.
This is also a small step toward getting rid of `failure::Error`.
Sessions' last use updates weren't getting persisted to the database
because the update statement wasn't passing through the hash.
Also address a TODO of tracing in tests by using the same tracing
setup as in production.
I think this is a big improvement in readability.
I removed the `lnav` config, which is a little sad, but I don't think it
supports this structured logging format well. Still seems worthwhile on
balance.
Scott Lamb
Skye