Motioncapture

[MV10]

Fixes #157

Ian, as before, I've only tested under .NET Core, but these are not especially complex changes.

Changes to make recording optional

• CircularBufferCaptureHandler constructor accepts bufferSize arg only
• VideoStreamCaptureHandler adds a parameterless constructor
• FileStreamCaptureHandler adds a parameterless constructor (with logged warning)
• Video and file handlers add safety checks to allow null CurrentStream

Change to CircularBufferCaptureHandler to address motion detection bug (issue 157)

• Calling DisableMotionCapture calls FrameDiffAnalyser.ResetAnalyser() (updates test frame)

Changes to MotionConfig to periodically update the test frame

• All constructor arguments optional
• Default RecordDuration is TimeSpan.Zero to record indefinitely after motion event
• For indefinite recording, consumer is responsible for stopping recording
• Threshold defaults to 130 (as per wiki suggestion for indoor use)
• New TestFrameInterval is the TimeSpan to refresh the test frame
• Default TestFrameInterval is 10 seconds

Changes to FrameDiffAnalyser to update the test frame

• New Stopwatch field _testFrameAge (never null, but not always running)
• When Apply finishes the full TestFrame, stopwatch is started
• When FrameAnalyser has a full frame, TestFrameExpired() can "steal" it
• TestFrameExpired checks elapsed time, updates test frame, restarts stopwatch

[techyian]

Thanks Jon. If the user does not want to record the raw stream, there seems to be an issue in that the _onStopDetect callback is never called even if there is a valid RecordDuration set. From my testing this is because of two things:

1) The user no longer calls StartRecording on the circular buffer handling the raw stream, only the encoded video handler and that's not the one that gets configured for motion detection - this results in _recordingElapsed being null when CheckRecordingProgress is called. 2) Even if the user does call it, there's now an InvalidOperationException being thrown if the stream is null.

Having said this, I actually think that the _onStopDetect callback is just complicating the situation and I have put together a gist of some changes I wish to propose:

• Remove the _onStopDetect callback completely and have the user handle the Starting/Stopping of recordings in the _onDetect callback.
• Instead of _onDetect being an Action type, change this to be a Func<Task> so asynchronous methods can be awaited cleanly within the lambda, which leads me onto the next point.
• Change StartRecording to return a Task. This method should accept a recordDuration TimeSpan and a CancellationToken. It awaits a TaskCompletionSource object which is triggered (within CheckRecordingProgress) when the record duration has passed.
• I have also noticed in the Process method that we're not dumping the contents of the buffer into the file at the right time.

From a user point of view, calling the API would look like this:

var motionConfig = new MotionConfig(); // Using default property values.
var recordDuration = TimeSpan.FromSeconds(10);

await cam.WithMotionDetection(motionCircularBufferCaptureHandler, motionConfig,
async () =>
{
    MMALLog.Logger.LogInformation("In onDetect callback.");

    // This callback will be invoked when motion has been detected.

    // Stop motion detection while we are recording.
    motionCircularBufferCaptureHandler.DisableMotionDetection();

    // (Optionally) Request a key frame to be immediately generated by the video encoder.
    vidEncoder.RequestIFrame();

    // Start recording our video.
    var rawTask = motionCircularBufferCaptureHandler.StartRecording(recordDuration, CancellationToken.None);
    var vidTask = vidCaptureHandler.StartRecording(recordDuration, CancellationToken.None);

    await Task.WhenAll(rawTask, vidTask);

    motionCircularBufferCaptureHandler.StopRecording();
    vidCaptureHandler.StopRecording();

    MMALLog.Logger.LogInformation("Recording has finished. Enabling motion again.");

    // We want to re-enable the motion detection.
    motionCircularBufferCaptureHandler.EnableMotionDetection();

    // Optionally create two new files for our next recording run.
    motionCircularBufferCaptureHandler.Split();
    vidCaptureHandler.Split();
})
.ProcessAsync(cam.Camera.VideoPort, cts.Token);

Please let me know what you think. I might have added additional debugging comments to the gist which you'll want to remove prior to updating your PR.

P.S. I've seen your comments around the performance of the motion code and when I get chance to review them properly I'll get back to you - I completely agree there's a lot that can be done in this area, it's quite a tricky subject to get right. I don't have much time at the moment to dedicate to this project due to work commitments.

[techyian]

I've just noticed in my gist that I haven't wrapped the invoking of onDetect() within a Task.Run which might be better?

[MV10]

I do agree that onStopDetect complicates things and I'll study your gist later today. These days I generally prefer Func<Task> for all the obvious reasons, although I'm not sure Task.Run buys us anything, I'll have to think about that a bit more.

Regarding the duration, I'd prefer that it still be optional. In my use-case the decision to stop recording is more complex than duration. My cameras start in groups (for example, motion on the front porch might start recording on an indoor camera watching the front door) and they only stop when none in the group still sees motion. So that brings up something else I hadn't mentioned (or really thought about in detail) yet -- a way to signal ongoing motion, which probably leads to other config requirements such as tolerance for brief interruptions in motion. I suppose it could be argued that onStopDetect could fill that need if it were not duration-driven (which would also be a better match for the method name).

No worries about being busy, I appreciate the thought you've put into this.

[MV10]

I also meant to ask, would you prefer that I also PR corresponding wiki changes? (Is the wiki set up that way? I haven't noticed.)

[MV10]

Hmm, what might work better is for StartRecording to only accept a CancellationToken -- if the caller wants the recording to be duration-based, they can create a token with a timeout. Use-cases like mine can associate more complex behavior with token cancellation.

[techyian]

Yes I agree with only having a CancellationToken on that method, no problem with that. I don't think GitHub Wikis can have pull requests made directly AFAIK but following the guidance here it looks like it should be fairly trivial for you to create an issue once you've made some changes in your forked repo's wiki and I can merge them in.

[MV10]

Good catch about the Process sequence.

I'm going to backpedal on my earlier comment about Func<Task> for onDetect -- that's an event which should inherently be fire-and-forget (MMALSharp doesn't care about the outcome), which is the sole use-case for async void. As always, the trouble with async void is that the implementation has be extra careful to guard against unhandled exceptions, which will tear down the entire dotnet process. Also, as written in the gist, nothing awaits/observes the Task so that would have caused issues on the library side.

With StartRecording solely based on an optional CancellationToken argument, CheckRecordingProgress isn't needed at all. StartRecording really only needs to flip the recording-flag on, then await the token cancellation, if one was provided. (If none is provided, the caller will execute StopRecording out-of-band.)

I'll PR these for your consideration.

[techyian]

Also, as written in the gist, nothing awaits/observes the Task so that would have caused issues on the library side.

Yes I did have my suspicions around this and the need to await that task would have trickled up so not ideal. Happy for it to be fire-and-forget and also happy with your comments around StartRecording.

[MV10]

I'm not enthused about the "shape" of the call to match the wiki example (recording both h.264 and the raw stream for a set duration), but I also still question the utility of recording the raw stream. This is what the consumer looks like now. You have to start the h.264 task but not await it just yet, first you must request an I-frame from the encoder. It doesn't appear to me that the encoder is visible to the capture handler, is it? I'd rather make the I-frame request internal to Process in the capture handler (based on whether the context is h.264), but I couldn't see a way to do that.

Maybe StartRecording could take an optional Action initRecording to which the caller could pass encoder.RequestIFrame?

This all works, though.

await cam.WithMotionDetection(
    motionCircularBufferCaptureHandler,
    motionConfig,
    // This callback will be invoked when motion has been detected.
    async () =>
    {
        // Stop motion detection while we are recording.
        motionCircularBufferCaptureHandler.DisableMotionDetection();

        // Prepare a token timeout to end recording after 10 seconds
        var stopRecordingCts = new CancellationTokenSource(10 * 1000);

        // Invoked when the token times out
        stopRecordingCts.Token.Register(() =>
        {
            // We want to re-enable the motion detection.
            motionCircularBufferCaptureHandler.EnableMotionDetection();

            // Optionally create new files for our next recording run
            vidCaptureHandler.Split();
            motionCircularBufferCaptureHandler.Split();
        });

        // Start recording our H.264 video, request an immediate h.264 key frame, and also record the raw stream.
        var vidCaptureTask = vidCaptureHandler.StartRecording(stopRecordingCts.Token);
        vidEncoder.RequestIFrame();
        await Task.WhenAll(
            vidCaptureTask,
            motionCircularBufferCaptureHandler.StartRecording(stopRecordingCts.Token)
        );
    })
    .ProcessAsync(cam.Camera.VideoPort, cts.Token);

[MV10]

Er... assume ConfigureAwait(false) will be added all over the place... :bulb:

[techyian]

I'm not enthused about the "shape" of the call to match the wiki example (recording both h.264 and the raw stream for a set duration)

Could you expand a little on this please? Are you referring to the way the user has to interact with the API to use this functionality? I appreciate what you're saying about the raw stream, I think in the majority of real life CCTV scenarios you wouldn't want to record the raw stream as it's not useful. Now that we're not reliant on the raw stream being recorded, it can probably be removed from the wiki example. If you have any other improvements about the way the user interacts with the API, I'm all ears, but I think this PR is definitely a good step in the right direction.

[MV10]

Specifically this part felt very awkward to me, which is why I suggested the additional change to StartRecording:

var h264Task = h264Handler.StartRecording(stopRecordingCts.Token);
h264Encoder.RequestIFrame();
await Task.WhenAll(
        h264Task,
        motionHandler.StartRecording(stopRecordingCts.Token)
);

Adding the Action initRecording argument would make h.264 recording cleaner:

await Task.WhenAll(
        h264Handler.StartRecording(h264Encoder.RequestIFrame, stopRecordingCTts.Token),
        motionHandler.StartRecording(stopRecordingCts.Token)
);

Unless capture handlers can somehow access the encoder which feeds them? In that case I'd just set a flag in StartRecording and have Process request the I-frame when context indicates h.264, and it would all Just Work™️...

[techyian]

Ah I see. Capture handlers do not have any visibility of the main MMALSharp project, the reason being is that I wanted a logical structure where data is passed from component -> callback handler -> capture handler, otherwise I was aware that it would be all to easy to start calling component specific code from areas where they shouldn't really be doing (in my opinion). I like your modification and happy to accept it into this PR if you want to make the change before I merge.

[MV10]

I figured that was likely the reason, just wanted to be sure I wasn't overlooking something.

I'll get that changed and pushed shortly, thanks!

[techyian]

Thanks very much for this!