My installation recently somehow ended up with a recording with a
duration of 503793844 90,000ths of a second, way over the maximum of 5
minutes. (Looks like the machine was pretty unresponsive at the time
and/or having network problems.)
When this happens, the system really spirals. Every flush afterward (12
per minute with my installation) fails with a CHECK constraint failure
on the recording table. It never gives up on that recording. /var/log
fills pretty quickly as this failure is extremely verbose (a stack
trace, and a line for each byte of video_index). Eventually the sample
file dirs fill up too as it continues writing video samples while GC is
stuck. The video samples are useless anyway; given that they're not
referenced in the database, they'll be deleted on next startup.
This ensures the offending recording is never added to the database, so
we don't get the same persistent problem. Instead, writing to the
recording will fail. The stream will drop and be retried. If the
underlying condition that caused a too-long recording (many
non-key-frames, or the camera returning a crazy duration, or the
monotonic clock jumping forward extremely, or something) has gone away,
the system should recover.
This is so far completely untested, for use by a new UI prototype.
It creates a new URL endpoint which sends one video/mp4 media segment
per key frame, with the dependent frames included. This means there will
be about one key frame interval of latency (typically about a second).
This seems hard to avoid, as mentioned in issue #59.
Use version 1 of the mvhd, tkhd, and mdhd boxes to support 64-bit
durations. 2^32 units / 90,000 units/sec / 60 sec/min / 60 min/hr ~=
13.25 hrs.
Compatibility: looks like Chrome, Firefox, VLC, and ffmepg all support
version 1 with no problem.
I went with the third idea in 1ce52e3: have the tests run each iteration
of the syncer explicitly. These are messy tests that know tons of
internal details, but I think they're less confusing and racy than if I
had the syncer running in a separate thread.
Now each syncer has a binary heap of the times it plans to do a flush.
When one of those times arrives, it rechecks if there's something to do.
Seems more straightforward than rechecking each stream's first
uncommitted recording, especially with the logic to retry failed flushes
every minute.
Also improved the info! log for each flush to see the actual recordings
being flushed for better debuggability.
No new tests right now. :-( They're tricky to write. One problem is that
it's hard to get the timing right: a different flush has to happen
after Syncer::save's database operations and before Syncer::run calls
SimulatedClocks::recv_timeout with an empty channel[*], advancing the
time. I've thought of a few ways of doing this:
* adding a new SyncerCommand to run something, but it's messy (have
to add it from the mock of one of the actions done by the save),
and Box<dyn FnOnce() + 'static> not working (see
rust-lang/rust#28796) makes it especially annoying.
* replacing SimulatedClocks with something more like MockClocks.
Lots of boilerplate. Maybe I need to find a good general-purpose
Rust mock library. (mockers sounds good but I want something that
works on stable Rust.)
* bypassing the Syncer::run loop, instead manually running iterations
from the test.
Maybe the last way is the best for now. I'm likely to try it soon.
[*] actually, it's calling Receiver::recv_timeout directly;
Clocks::recv_timeout is dead code now? oops.
This no longer requires installing ffmpeg manually, so there should be
significantly less data to cache (faster runs). The build step itself
should also be faster when the cache is unavailable/stale.
Also sneak in a change from "pkg-config" to "pkgconf" package in the
scripts and travis CI. They didn't match the manual instructions; make
them all consistent. They both seem to work fine, but I gather pkgconf
is the newer thing. Its roadmap is here and notes that distros are
moving toward it.
https://github.com/pkgconf/pkgconf/wiki/Roadmap
Fixes#46. If there are no video_sample_entries, it returns
InvalidArgument, which gets mapped to a HTTP 400. Various other failures
turn into non-500s as well.
There are many places that can & should be using typed errors, but it's
a start.
* remove intermediate bool from adjust_day.
* rewrite LockedDatabase::list_aggregate_recordings.
I started by collapsing the flush into the first part of the if, in a
similar way to adjust_day. But then I refactored more and ended up
with a structure that probably would have been allowed with the old
lexical borrow checker. I think it's more readable, and it does 1
btree operation per row where before it did 2 or 3.
This is mostly just "cargo fix --edition" + Cargo.toml changes.
There's one fix for upgrading to NLL in db/writer.rs:
Writer::previously_opened wouldn't build with NLL because of a
double-borrow the previous borrow checker somehow didn't catch.
Restructure to avoid it.
I'll put elective NLL changes in a following commit.
Before, this would panic from the reactor thread. After, it returns a
internal server error. Still not ideal, but better.
To return "bad request" as it should, mp4::FileBuilder::build() should
return a new error type that distinguishes "invalid argument" from
"internal" and the like. I'm thinking of using a ErrorKind enum
throughout the program that's similar to grpc::StatusCode.
Apparently with docopt, --require-auth=false doesn't work, so booleans
with a default value of true can't be turned off. Toggle the default to
false to deal with this, for now. I'd prefer the default be true, but
I also would prefer to not use a negative --no-require-auth or
--allow-unauthenticated flag. I think I'll switch from docopt to clap
in the near future; it seems to be what the cool kids use.
The guide is not as quick to follow and amateur-friendly as I'd like. A
few things that might improve matters:
* complete #27 (built-in https+letsencrypt), so that when not sharing
the port, users don't need to use nginx or certbot.
* more ubiquitous IPv6 (out of my control but should happen over
time) to reduce need to share the port
* embed a dynamic DNS client
* support UPnP Internet Gateway Device Control Protocol (if common
routers have this enabled? probably not for security reasons.)
It's progress, though. Enough that I think I'll merge the auth branch
into master shortly.
I initially chose SameSite=Lax because I thought if a user followed a
link to the landing page, the landing page's ajax requests wouldn't send
the cookie. But I just did an experiment, and that's not true. Only the
initial page load (of a .html file) lacks the cookie. All of its
resources and ajax requests send the cookie. I'm not sure about
document.cookie accesses, but my cookie is HttpOnly anyway, so it's
irrelevant. So no reason to be lax.
newTimeFormat didn't handle newTimeZone not having been called well.
Restore the prior behavior of having called newTimeZone(null), which was
apparently good enough.
I just ran a "cargo test" on this after a round of tweaks, not
"cargo test --all", so I missed compile errors in the db crate,
and a Javascript lint config error. travis-ci caught these.
Some caveats:
* it doesn't record the peer IP yet, which makes it harder to verify
sessions are valid. This is a little annoying to do in hyper now
(see hyperium/hyper#1410). The direct peer might not be what we want
right now anyway because there's no TLS support yet (see #27). In
the meantime, the sane way to expose Moonfire NVR to the Internet is
via a proxy server, and recording the proxy's IP is not useful.
Maybe better to interpret a RFC 7239 Forwarded header (and/or
the older X-Forwarded-{For,Proto} headers).
* it doesn't ever use Secure (https-only) cookies, for a similar reason.
It's not safe to use even with a tls proxy until this is fixed.
* there's no "moonfire-nvr config" support for inspecting/invalidating
sessions yet.
* in debug builds, logging in is crazy slow. See libpasta/libpasta#9.
Some notes:
* I removed the Javascript "no-use-before-defined" lint, as some of
the functions form a cycle.
* Fixed#20 along the way. I needed to add support for properly
returning non-OK HTTP statuses to signal unauthorized and such.
* I removed the Access-Control-Allow-Origin header support, which was
at odds with the "SameSite=lax" in the cookie header. The "yarn
start" method for running a local proxy server accomplishes the same
thing as the Access-Control-Allow-Origin support in a more secure
manner.
Fixes#62
* added travis config for latest node as well as 8.
* ran "yarn upgrade -P webpack-dev-server", which caused the upath
dependency to be upgraded. I arrived at this by inspecting yarn.lock
for the things depending on upack, along with some trial and error.
("yarn upgrade -P chokidar" was less successful.)
Fixes#60
The reqwest dependency is significant because the old version required
an old version of openssl, complicating compilation on newer platforms.
reqwest also pulled in old/duplicate versions of hyper, tokio, etc.
Nice to drop a lot of that cruft.
I left rusqlite and uuid alone because they had breaking changes I
didn't want to mess with at the moment.
Bumped the minimum Rust version to 1.30.0, as required by the
new encoding_rs crate (and perhaps other things).
1.26 doesn't work with the updated rusqlite:
error[E0658]: use of unstable library feature 'duration_extras' (see issue #46507)
--> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/rusqlite-0.14.0/src/busy.rs:26:49
|
26 | .and_then(|t| t.checked_add(timeout.subsec_millis().into()))
| ^^^^^^^^^^^^^
1.25 also fails with the upgraded reffers because u128 isn't stable:
error[E0658]: 128-bit type is unstable (see issue #35118)
--> /home/travis/.cargo/git/checkouts/reffers-rs-0d00fc7f893338b3/49a4d75/src/rc_bitmask.rs:194:34
|
194 | rc_bit_mask_internal!(primitive, u128, 42, 42, 42);
| ^^^^
travis-ci pointed out that building with 1.21 broke with a recent dep
upgrade (8c52c36). reffers now uses nested groups of imports, which is a
feature introduced with Rust 1.25. Prior to 1.25, it fails as follows:
error: expected one of `,` or `as`, found `::`
--> /home/travis/.cargo/git/checkouts/reffers-rs-0d00fc7f893338b3/49a4d75/src/arc.rs:6:46
|
6 | use std::sync::atomic::{AtomicUsize, Ordering::SeqCst};
| ^^ expected one of `,` or `as` here
Scott Lamb