Skip to content
This repository has been archived by the owner on Feb 22, 2024. It is now read-only.

Update timestamp annotations #166

Closed
wants to merge 2 commits into from

Conversation

MV10
Copy link
Collaborator

@MV10 MV10 commented Aug 15, 2020

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

  • Adds string prop DateFormat which defaults to the formerly hard-coded "dd/MM/yyyy"
  • Adds string prop TimeFormat which defaults to the formerly hard-coded "HH:mm"
  • Adds property RefreshRate of the DateTimeTextRefreshRate type described below

MMALCameraComponentExtensions

  • Altered to use the DateFormat and TimeFormat formatting strings

MMALCamera

  • Modifies ProcessAsync to determine a refresh interval, invoke a refresh task, and control it by token
  • Adds a private RefreshAnnotation method which awakens periodically to call SetAnnotateSettings

DateTimeTextRefreshRate intervals

  • Disabled - consumer can invoke EnableAnnotation on some custom scheme
  • Daily - once per minute, useful if time is not shown at all
  • Minutes - once per second, useful for the default "HH:mm" format
  • Seconds - once every 250ms, useful for an "HH:mm:ss" format
  • SubSecond - every 41ms, which is about 24 FPS, useful for the "HH:mm:ss.ffff" format

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
Copy link
Owner

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
Copy link
Collaborator Author

MV10 commented Aug 16, 2020

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
Copy link
Collaborator Author

MV10 commented Aug 16, 2020

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

@MV10 MV10 closed this Aug 17, 2020
@MV10 MV10 deleted the UpdateTimestampAnnotations branch August 17, 2020 17:14
@techyian techyian self-requested a review August 29, 2020 13:32
@techyian techyian added this to the v0.7 milestone Aug 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants