techyian / MMALSharp

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

Update timestamp annotations #166

Closed MV10 closed 3 years ago

MV10 commented 3 years ago

Hi Ian, as mentioned in that feature-request issue, these changes refresh date/time annotations at a configurable interval based on the time resolution of the annotation. The format strings are configurable.

AnnotateImage

MMALCameraComponentExtensions

MMALCamera

DateTimeTextRefreshRate intervals

On my Pi4 with the 5MP camera encoding a 1080p MP4 (fragmented), I was still able to get a steady 25FPS with the subsecond interval. I imagine older model Pis or maybe the 8MP camera could start to miss frames at 41ms, given I timed the annotation update at about 0.4-0.5ms per invocation.

No conflicts with my other open PR. Hope you like it.

techyian commented 3 years ago

Hi Jon,

Apologies for the delay on your PRs, I've been away for the past week. After having a quick look over them, I still have concerns with the changes to the FileStreamCaptureHandler class, specifically:

1) From what I can see, the latest changes will not work when taking images from the camera's still port as there is no implicit call to NewFile when using a ImageStreamCaptureHandler. Prior to your changes, whenever a new instance of FileStreamCaptureHandler was created, a new FileStream was initialised, but since you've changed it to a MemoryStream, the only time a new file will be created is in the video capture scenarios (Fast Image, Video when calling Split()). Please correct me if I'm wrong, though. 2) Related to the above, I feel as though this capture handler is now a bit unclear due to extending MemoryStreamCaptureHandler - to me this doesn't really make sense, although I appreciate why you've done this. Could there be any changes made to the CircularBufferCaptureHandler to support your use-case? As I've mentioned previously, I don't want to bake too much into the core library itself which can be achieved via Callback/Capture handlers but if the CircularBufferCaptureHandler can be modified to allow individual frame recording that might help solve the problem you have. 3) I can see your code style in your PRs does differ somewhat to the style I've been trying to adhere to throughout the library - I have tried to become a bit more strict with myself recently. I think it's time to write up a document with more info on this but as I don't regularly see contributors it's not been high on the list of priorities. 4) I think this PR should have been separate to your changes to the FileStreamCaptureHandler. I haven't had chance to test your changes to the Annotation feature, but I can see the benefits it brings and I'll try to find some time over the next few days to test it. Please can you branch it off your dev branch instead?

Thanks, Ian

MV10 commented 3 years ago

Hi Ian,

Sorry about the dependent PRs, I suppose a Github PR is a bit different than the git setup I normally use (a frankly terrible Atlassian BitBucket setup mandated by the corporate types). There, non-conflicting PRs can be merged separately whether they're dependent or not. I'll set up a new PR branched off dev with the annotation changes.

I imagine you're correct about the still port situation, I've never looked at that code and didn't realize it interacted differently. I'll do some testing and I'll close this if it turns out to be a problem. I hadn't considered using the circular buffer but I'll look into that as well, thanks for the suggestion.

MV10 commented 3 years ago

Yup, still port is broken. Back to the drawing board! Good call.