techyian / MMALSharp

C# wrapper to Broadcom's MMAL with an API to the Raspberry Pi camera.
MIT License
195 stars 33 forks source link

migrate FFmpegCaptureHandler to ExternalProcessCaptureHandler, add VL… #156

Closed MV10 closed 3 years ago

MV10 commented 4 years ago

Project MMALSharp.Processing:

As mentioned in the original issue thread, buffering avoids frame drops and other problems due to Console.WriteLine performance and blocking behavior. I saw approx 10% to 15% improvement running 1080p 30FPS MP4 transcoding when buffering was added on a 4GB Pi 4B writing to ramdisk.

Project MMALSharp.FFmpeg:

Project MMALSharp.Tests:

Questions:

Usage versus the usual one-liner call to await ProcessAsync(...):

await Task.WhenAll(new Task[]{
    ffmpeg.ManageProcessLifecycleAsync(timerToken.Token),
    cam.ProcessAsync(cam.Camera.VideoPort, timerToken.Token),
}).ConfigureAwait(false);
techyian commented 4 years ago

Many thanks for sending in this PR Jon. I'm assuming your testing so far has been done in a .NET Core environment? I'll just need to make sure everything is OK on Mono also. I'll try and find some time this week to have a look over it all.

MV10 commented 4 years ago

Yes, purely .NET Core. Hadn't thought about that angle, sorry. Haven't touched Mono since the wife and I dabbled in game dev in Unity years ago, not really set up for that.

MV10 commented 4 years ago

I see your CancellationToken.AsTask extension, that's what my handler is doing in the WaitForCancellationAsync method (albeit as a completion). I'll push a change to use your method.

MV10 commented 3 years ago

I'm also fixing up a couple places where I overlooked ConfigureAwait(false) since you're using Mono (apparently it's no longer necessary under .NET Core, so I've been trying to wean myself off the habit), and I'm moving the handler's options class to a separate file which is a better match for the rest of the codebase.

MV10 commented 3 years ago

I've often wondered about the ConfigureAwait guidance. Since Blazor is their official UI direction including non-web scenarios (once the other renderers are added), I imagine this is why it's a blanket statement, even native renderers will be ASP.NET-based under the hood. Probably safest to continue adding it to library projects for the time being, though.

MV10 commented 3 years ago

I still don't like the need for this, particularly:

await Task.WhenAll(new Task[]{
    ffmpeg.ManageProcessLifecycleAsync(timerToken.Token),
    cam.ProcessAsync(cam.Camera.VideoPort, timerToken.Token),
}).ConfigureAwait(false);

I suppose nobody could reasonably use this handler without RTFM but it's still a break from the pattern. I played with a custom ProcessAsync on the handler but that was also just weird and required a tangle of new dependencies. What if we added an optional extra Task like this:

ProcessAsync(IOutputPort p, CancellationToken t = default, Task concurrentTask = null)

Then running the capture would look like this:

await cam.ProcessAsync(cam.Camera.VideoPort, cts.Token, ffmpeg.ManageProcessLifecycleAsync(cts.Token));

Internally ProcessAsync would become a wrapper which either awaited the "real" process call, or used the WhenAll form if the optional extra Task is provided.

What do you think? Too much tail-wagging-the-dog? I tried the other way around -- making the handler's lifecycle call accept the camera processing Task, but that is probably even weirder from the breaking-the-pattern standpoint (e.g. you're no longer using the camera to run processing). And by adding it on the camera side as a general-purpose Task it opens it up to coordinating other concurrent processing in the future. It could then even be overloaded to accept an array of Tasks in that same arg position to process any number of tasks.

Edit -- keeping in mind that returns a hot task (already running) which is no problem for this handler. But for better synchronization a Func<Task> might be preferable. Gets a little ugly because of the need for an async lambda to pass args though. I suppose with overloads we could add both -- and array variants.

await cam.ProcessAsync(cam.Camera.VideoPort, cts.Token, async () => ffmpeg.ManageProcessLifecycleAsync(cts.Token));
MV10 commented 3 years ago

I'm looking through ProcessAsync today and seeing that all your components are already managed as concurrent tasks. What may be easier (and better-hide this handler's requirement from the library consumer) is a new interface which is used to provide ProcessAsync with one or more arbitrary additional tasks that the handler requires.

I think it could be as simple as ProcessAsync tries to cast each component to something like IProcessHandlerTasks (or whatever) and if it succeeds, it passes that internal List<Task> to an interface method like AddTasks before processing starts.

Clean, simple, 100% transparent to the library consumer. What do you think?

techyian commented 3 years ago

I hear what you're saying, but what you're trying to achieve here doesn't really affect the components themselves, it affects the capture handlers. Each port has a "Callback Handler" associated with it to provide a layer of abstraction, which in turn has a capture handler association. I'm not sure it'll be as simple as you're suggesting.

The ProcessAsync method really has one job and that is to handle the processing of data that is returned by the camera (or standalone data) and asynchronously wait for the TaskCompletionSource objects to trigger. I see the ExternalCaptureHandler's task that we're wanting to await as something totally separate to the running of MMALSharp and although modifying ProcessAsync and any other related code to support your suggestions would keep things cleaner for the end-user, it straps on additional responsibility to this method and might make things harder to maintain going forward.

FFmpeg support has always been experimental and was added primarily as a shortcut for users so they didn't have to manually run it as a separate process once MMALSharp had finished doing its thing. It is a separate application and I think it's totally acceptable to expect users to read the manual before using this functionality - the way you have designed it actually makes it clearer in my head that these are two separate tasks complimenting each other. Additional code comments can be added if you feel they'll be necessary or changing method names (for example, does ProcessExternalAsync work better than ManageProcessLifecycleAsync?) but I'm quite happy with this approach as it currently stands.

MV10 commented 3 years ago

Sounds good to me. Thanks for the clarification.

techyian commented 3 years ago

I've just edited my post, sorry if you missed it before replying. I was just wondering whether using the method name ProcessExternalAsync may be better than ManageProcessLifecycleAsync, what do you think to that or do you think it's better as it currently stands?

MV10 commented 3 years ago

I did miss that, I'll change it. Funny too, I actually used ProcessExternalAsync for a short time, but originally had the "outer" code doing more of the work and changed it trying to clarify what was going on. Now that it's back to just one extra step, I think that name is good, I feel like it sort of helps "relate" it to the camera ProcessAsync call.