I see a lot of yields and such in CPU profiles. I think the workers
are frequently waking up, finding there's not much to do, and going back
to sleep. Reducing the number of worker threads seems reasonable.
Moonfire NVR has some enforcement on its own; this makes retina vs
ffmpeg more of an apples-to-apples comparison.
I'm also thinking of dropping enforcement from retina; enough things
have sketchy timestamps that this policy doesn't make much sense anyway.
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.
This is (slightly) complicating the switch from ffmpeg to retina
as the RTSP client. And it's not really that close to what I want
to end up with for analytics:
* I'd prefer the analytics happen in a separate process for
several reasons
* Feeding the entire frame to the object detector doesn't produce
good results.
* It doesn't do anything with the results yet anyway.
This caused served chunks to be truncated. On seek, nginx sometimes
served 502 errors, chrome sometimes returned
ERR_CONTENT_LENGTH_MISMATCH, and videos weren't playing properly.
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
* my dad's GW4089IP cameras use 720x480
* some Reolink cameras use 640x352
* I'm playing with rotated cameras (16x9 -> 9x16)
I'd prefer to calculate pasp from a configured camera aspect ratio
than to hardcode the assumption these are 16x9, but that requires
a schema change. This is an improvement for now.
The immediate motivation is to address these CI failures with nightly:
https://github.com/scottlamb/moonfire-nvr/runs/2593322801?check_suite_focus=true
```
Compiling lock_api v0.4.2
error[E0557]: feature has been removed
--> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/lock_api-0.4.2/src/lib.rs:91:42
|
91 | #![cfg_attr(feature = "nightly", feature(const_fn))]
| ^^^^^^^^ feature has been removed
|
= note: split into finer-grained feature gates
```
Strangely, they don't occur locally with "rustc 1.54.0-nightly
(fe72845f7 2021-05-16)" but do on CI with the exact same version?!?
I don't get it, but lock-api 0.4.4 is advertised as being updated for
latest nightly, so I expect this will address the problem anyway.
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.
* API change: in update signals, allow setting a start time relative
to now. This is an accuracy improvement in the case where the client
has been retrying an initial request for a while. Kind of an obscure
corner case but easy enough to address. And use a more convenient
enum representation.
* in update signals, choose `now` before acquiring the database lock.
If lock acquisition takes a long time, this more accurately reflects
the time the caller intended.
* in general, make Time and Duration (de)serializable and use them
in json types. This makes the types more self-describing, with
better debug printing on both the server side and on the client
library (in moonfire-playground). To make this work, base has to
import serde which initially seemed like poor layering to me, but
serde seems to be imported in some pretty foundational Rust crates
for this reason. I'll go with it.
In particular, this was happening out of the box on Raspberry Pi OS Lite
20210304, as reported by ironoxidizer@gmail.com here:
https://groups.google.com/g/moonfire-nvr-users/c/2j9LvfFl2u8/m/tJcNS2WfCQAJ
* adjust main.rs to make the problem more obvious
* mention it in the troubleshooting guide
* sidestep it in the nvr docker wrapper script
also just use --networking=host rather than --publish (avoiding a proxy
process). I'm using Docker to simplify the build and deployment process,
not as a security boundary, so just do the simpler thing.
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.
As required for live view (#59) to work on Safari.
Safari has some "interesting" expectations:
* There must be a non-empty list of compatible brands. The major brand
is not automatically included. (Looks like ISO/IEC 14496-12 doesn't
spell out which is correct.)
* The tfdt box must be before the trun boxes. Moonfire NVR was not
compliant with ISO/IEC 14496-12:2015 section 8.8.12.1 before.
Chrome and Firefox didn't care, but Safari does.
* The mdat must be written with the small format. Safari is not
implementing the spec properly.
I figured these out by painstakingly comparing Moonfire NVR's output
with gpac's, making it match almost byte-for-byte until it worked, then
backing out changes one at a time to check which were relevant. Ugh!
Chrome appears to time out at 60 seconds of inactivity otherwise.
I think it's better to keep the stream open, even if the camera is
broken.
The implementation looks awkward, but that might be the state of Rust
async right now.
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.
The CI nightly builds had been broken with the following error:
```
error: custom inner attributes are unstable
--> /home/runner/work/moonfire-nvr/moonfire-nvr/server/target/debug/build/moonfire-db-415ce696a754c614/out/schema.rs:10:4
|
10 | #![rustfmt::skip]
| ^^^^^^^^^^^^^
|
= note: `#[deny(soft_unstable)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #64266 <https://github.com/rust-lang/rust/issues/64266>
```
I'd thought this was by mistake given that #[rustfmt::skip] is still
advertised on rustfmt's github page, but maybe not. Looks like
rust-protobuf's newest version uses
`#![cfg_attr(rustfmt, rustfmt::skip)]` to avoid this error.
Also fix a warning on nightly about an extraneous semicolon.
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.
As noted in mylog's 2b1085c:
Looks like both the GNU tools' --color argument and cargo's
CARGO_TERM_COLOR expect always/never rather than on/off. Match that.
Might as well understand off/no/false and on/yes/true also.
This picks up moonfire-ffmpeg's 4b13378:
support ffmpeg's multi-call log messages
This should fix this annoying log output:
```
W20210310 13:17:09.060 s-garage_west-main moonfire_ffmpeg::rtsp] 0xaf300950: RTP H.264 NAL unit type 29
W20210310 13:17:09.060 s-garage_west-main moonfire_ffmpeg::rtsp] 0xaf300950: is not implemented. Update your FFmpeg version to the newest one from Git. If the problem still occurs, it means that your file has a feature which has not been implemented.
```
so it looks like this instead:
```
W20210310 13:17:09.060 s-garage_west-main moonfire_ffmpeg::rtsp] 0xaf300950: RTP H.264 NAL unit type 29 is not implemented. Update your FFmpeg version to the newest one from Git. If the problem still occurs, it means that your file has a feature which has not been implemented.
```
I think this has a minor behavior change: permission denied replies
change to HTTP 403 where they were HTTP 401. The new behavior seems
more correct, as these errors can occur when authentication has
succeeded but the session in question is not authorized for the given
operation. The UI currently doesn't care about this distinction.
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.
* add more description to the troubleshooting guide
* adjust the log format to match more recent glog
* include a config for the lnav tool, which will help colorize,
browse, and search the logs.
Next up: install an ffmpeg log callback for consistency.
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".
This was mostly straightforward. The most confusing part waas the Sync
bound change on body streams. I copied what hyper did and it seemed to
work. /shruggie
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.
