After a frustrating search for a suitable channel to use for shutdown
(tokio::sync:⌚:Receiver and
futures::future::Shared<tokio::sync::oneshot::Receiver> didn't look
quite right) in which I rethought my life decisions, I finally just made
my own (server/base/shutdown.rs). We can easily poll it or wait for it
in async or sync contexts. Most importantly, it's convenient; not that
it really matters here, but it's also efficient.
We now do a slightly better job of propagating a "graceful" shutdown
signal, and this channel will give us tools to improve it over time.
* Shut down even when writer or syncer operations are stuck. Fixes#117
* Not done yet: streamers should instantly shut down without waiting for
a connection attempt or frame or something. I'll probably
implement that when removing --rtsp-library=ffmpeg. The code should be
cleaner then.
* Not done yet: fix a couple places that sleep for up to a second when
they could shut down immediately. I just need to do the plumbing for
mock clocks to work.
I also implemented an immediate shutdown mode, activated by a second
signal. I think this will mitigate the streamer wait situation.
I saw this recently while working on new-schema. It was probably due
to some manual upgrade or downgrade I did rather than an actual bug.
Improve debuggability a little nonetheless.
I copied the example of the password field by introducing a setter.
But I forgot: it was only that way because the password field has
the complexity of hashing/salting. For fields where setting is
idempotent, it can be directly exposed.
As written in the changelog: Live streams formerly worked around a
Firefox pixel aspect ratio bug by forcing all videos to 16:9, which
dramatically distorted 9:16 camera views. Playback didn't, so anamorphic
videos looked correct on Chrome but slightly stretched on Firefox. Now
both live streams and playback are fully correct on all browsers.
This isn't well-tested and doesn't yet support an initial connection
timeout. But in a quick test, it successfully returns video!
I'd like to do some more aggressive code restructuring for zero-copy
and to have only one writer thread per sample file directory (rather
than the syncer thread + one writer thread per RTSP stream). But I'll
likely wait until I drop support for ffmpeg entirely.
Reading from the mmap()ed region in the tokio threads could cause
them to stall:
* That could affect UI serving when there were concurrent
UI requests (i.e., not just requests that needed the reads in
question anyway).
* If there's a faulty disk, it could cause the UI to totally hang.
Better to not mix disks between threads.
* Soon, I want to handle RTSP from the tokio threads (#37). Similarly,
we don't want RTSP streaming to block on operations from unrelated
disks.
I went with just one thread per disk which I think is sufficient.
But it'd be possible to do a fixed-size pool instead which might improve
latency when some pages are already cached.
I also dropped the memmap dependency. I had to compute the page
alignment anyway to get mremap to work, and Moonfire NVR already is
Unix-specific, so there wasn't much value from the memmap or memmap2
crates.
Fixes#88
I saw this error once:
Apr 27 21:01:33 nuc moonfire-nvr[188570]: s-reolink-sub
moonfire_nvr::streamer] reolink-sub: sleeping for Duration { secs: 1,
nanos: 0 } after error: CHECK constraint failed: video_sample_entry
and would like to understand it better.
To improve reliability of live streams (#59) on Safari.
Safari was dropping the cookie from websocket update requests.
(But it worked sometimes. I don't get why.) I saw folks on the Internet
thinking this related to HttpOnly:
* https://developer.apple.com/forums/thread/104488
* https://stackoverflow.com/q/47742807/23584
but I still see this behavior without HttpOnly. SameSite=Strict vs
SameSite=Lax appears to make a difference. Try that instead.
SameSite=Strict is pointless for us anyway as noted in a new comment.
Turning off HttpOnly would be more unfortunate security-wise.
I spotted this by inspection: adding a media time and wall time didn't
look right. I also confirmed the brokenness on my primary NVR:
```
sqlite> .mode column
sqlite> select
...> r1.composite_id,
...> r1.prev_media_duration_90k,
...> r1.wall_duration_90k,
...> r1.media_duration_delta_90k,
...> r2.composite_id,
...> r2.prev_media_duration_90k
...> from
...> recording r1 join recording r2 on (r1.composite_id = r2.composite_id - 1)
...> where
...> r1.prev_media_duration_90k + r1.wall_duration_90k + r1.media_duration_delta_90k !=
...> r2.prev_media_duration_90k
...> limit 5;
4296791095 2232623913716 5398956 154 4296791096 2232629312672
4296791096 2232629312672 5400016 38 4296791097 2232634712688
4296791097 2232634712688 5400729 105 4296791098 2232640113417
4296791098 2232640113417 5399024 80 4296791099 2232645512441
4296791099 2232645512441 5400770 124 4296791100 2232650913211
```
In the first row, the second recording's prev_media_duration_90k is the
first's prev_media_duration_90k plus its wall time, not its media time.
I also enforced some invariants in the signals code, fixing a couple
bugs. The signals code is more complex than I'd like, but hopefully
is working now.
I'd once thought about using 1 second resolution for signals and wrote
this map to match that. But I decided to match signals to the timestamps
used elsewhere instead. Match that for the days map also.
My main goal is to support creating indexes for signals as well as
recordings. An additional goal is to just shrink db.rs a bit; it's
gotten quite large.
Looks like a refactoring in 9d7cdc09 introduced the possibility this
could fail (where before it might produce a silly i32 pts) and forgot
to restore the invariant.
Otherwise `moonfire-nvr check --delete-orphan-rows` can fail with this
error:
```
I0305 113848.655 main moonfire_db::check] Deleting 2 recording rows
E0305 113848.655 main moonfire_nvr] Exiting due to error: FOREIGN KEY constraint failed
```
The new order matches the online system's `db::raw::delete_recordings`.
I'm tired of all the boilerplate, so use the new
GPL-3.0-linking-exception license identifier instead in all the server
components.
I left the ui stuff alone because I'm just going to replace it (#111).
Add a checker for the header because it's easy to forget.
I want to make the project more accessible by not expecting folks to
match my idiosyncratic style. Now almost [1] everything is written
in the "standard" style. CI enforces this.
[1] "Almost": I used #[rustfmt::skip] in a few sections where I felt
aligning things in columns significantly improves readability.
For recovering from corruption, as in #107. These should aid in
restoring database integrity without throwing away the entire database.
I only added the conditions that came up in #107, so far.
* "Missing ... row" => --trash-orphan-sample-files
* "Recording ... missing file" => --delete-orphan-rows
* "bad video_index" => --trash-corrupt-rows
In particular, if there are recordings in progress when the process
dies, they may still be around when check runs. They are easily
identifiable by having an id > cum_recordings and get automatically
deleted on startup, so there's no reason to complain about them.
Inspired by the poor error message here:
https://github.com/scottlamb/moonfire-nvr/issues/107#issuecomment-777587727
* print the friendlier Display version of the error rather than Debug.
Eg, "EROFS: Read-only filesystem" rather than "Sys(EROFS)". Do this
everywhere: on command exit, on syncer retries, and on stream
retries.
* print the most immediate problem and additional lines for each
cause.
* print the backtrace or an advertisement for RUST_BACKTRACE=1 if it's
unavailable.
* also mention RUST_BACKTRACE=1 in the troubleshooting guide.
* add context in various places, including pathnames. There are surely
many places more it'd be helpful, but this is a start.
* allow subcommands to return failure without an Error.
In particular, "moonfire-nvr check" does its own error printing
because it wants to print all the errors it finds. Printing "see
earlier errors" with a meaningless stack trace seems like it'd just
confuse. But I also want to get rid of the misleading "Success" at
the end and 0 return to the OS.
* give a rule of thumb for update time in the documentation
* log the SQLite3 version, which can affect performance
* do the vacuum in non-WAL mode, to correctly set the page size and to
avoid very slow behavior on older SQLite3 versions. Larger page sizes
are generally faster (including subsequent vacuum operations).
This won't help much for the first vacuum after this change, but it
will help afterward.
* likewise, set the page size properly on "moonfire-nvr init".
Besides being more clear about what belongs to which, this helps with
docker caching. The server and ui parts are only rebuilt when their
respective subdirectories change.
Extend this a bit further by making the webpack build not depend on
the target architecture. And adding cache dirs so parts of the server
and ui build process can be reused when layer-wide caching fails.
