techyian / MMALSharp

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

Delete circular buffer zero-length file #192

Open MV10 opened 3 years ago

MV10 commented 3 years ago

Hi Ian, this a quick-fix to remove the zero-length file that the circular buffer leaves behind after stream disposal. It also handles the edge case of disposal while recording is active (meaning it won't delete the valid in-progress file), including the scenario where StopRecording was called but Split was not (which can be detected if CurrentStream is null).

So I think it's pretty safe, I can't think of any other oddball scenarios.

I also tweaked the summaries of a couple file stream handler props/methods, I was having trouble remembering exactly which parts of the full pathname they return (some names seem a bit inconsistent). That wasn't strictly necessary of course, but my first attempt put the file deletion in that class -- until I realized video capture handler would delete its valid file (oops).

MV10 commented 3 years ago

Hold off on this one while I re-think it tomorrow.

I've found that the zero-length file is not deleted if you never start recording from the buffer.

I'm not sure there is enough information to decide when it's safe to delete the file under all scenarios:

MV10 commented 3 years ago

Turns out the easiest thing to do is add a flag that explicitly tracks whether data has been written to the file. The only time it's zero-length is after the constructors or immediately after calling Split. Passes all my tests now!

techyian commented 3 years ago

Hi Jon, thanks for raising this. Does this issue only affect the Circular Buffer handler or is it worth implementing this change further up at the FileStreamCaptureHandler?

MV10 commented 3 years ago

Good call, I think you're right. No current handler has the problem except the circular buffer -- the video handler immediately writes to the file -- but I suppose some future handler might exhibit the same problem.

I'm about to leave the house but I will revise the PR tomorrow morning.

MV10 commented 3 years ago

Ok, fixed, thanks for the insight. I suppose even the video handler could exhibit the problem in an edge case where the app was shutting down just as the constructor or split was called.

I also removed a left-over MotionType prop in the video handler that I missed in the earlier motion detection PRs.

I'm not personally a fan of unit tests , but it occurs to me that this is probably something that is very testable. (I understand the attraction but I've been involved with too many mega-scale enterprise projects where deciphering test code and fixing test bugs is as much work as the real codebase.) Thanks to my personal prejudice against them, my test-writing-fu is rusty at best, and perhaps the worst unit test experience is when someone writes a block of tests that doesn't match the project's general testing style, if you know what I mean. So, apologies in advance for not handling that.