-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multiple proposed changes to resolve retriggering and increase configurability #7
base: main
Are you sure you want to change the base?
Conversation
Thanks for the thoughtful changes! 1/ 👍 RE: why it used processes instead of threads: originally, the video decoding is handled within the process, so the process model was better since it's gonna be CPU intensive. But now the decoding work has been offloaded to ffmpeg (commit e4fcc6c) as a subprocess. The difference doesn't matter anymore. 3/ This was to prevent feedback loop. A message containing summary would be received by itself and re-processed without an explicit check. I know this seems like a dumb issue but maybe there's a better fix? I dunno yet. 4 & 5/ Love this. I experienced the issue as well. 6/ The additional modes sound great! One small suggestion would be combining some of the paramaters for simplicity e.g. 7/ Not exactly sure what these mean. But I'd recommend breaking them into another PR if possible. Smaller PRs make discussions easier IMO. 8/ 👍. I ran into the bad json problem too. OpenAI's newly introduced json mode isn't available for the GPT-4v model yet unfornately. I went over your diffs briefly and I think they make sense overall. But your base branch isn't up to date making it a bit harder to review. It'd also be great if you could break down this PR into smaller chunks if possible becuase some of these changes are clearly great and ready to be merged but some need some more discussions. Thank you again! |
Hello,
This is still a work in process but I made a couple minor and and couple larger changes across the script, let me know if you like any of the ideas as I am more than happy to clean them up but in this current configuration it does seem to work better for me.
Change every processing of an event to use a thread vs a process, I see little benefit from a process and it may just be harder to manage down the road.
Resolve "stationary" objects from re-triggering a new detection
Resolve "updated" but already processed events from being triggered again. My guess is the following code was meant to prevent this case:
if "summary" in payload["after"] and payload["after"]["summary"]:
Unfortunately I don't see how that would've been hit as the summary is being published to the MQTT_SUMMARY_TOPIC but only the MQTT_FRIGATE_TOPIC is subed
There is a longish standing issue that often times a single 'object' will trigger multiple almost identical events. Frigate actually has a task that cleans these up in the UI but the result I was seeing was that processing of multiple identical events with different event_ids was occurring.
The above issue, and all the general issues of multiple or unneeded event processing is being handled by two global TTLCache dictionaries
I additionally saw a large amount of errors on my frigate server from api requests for a video clip before one was available. Additionally when the script was able to get a clip, it was often too short or incomplete. For this reason I introduce the following options: (these are not meant to be used all together but instead gives a few choices of how the event is processed)
Added configurable parameters for some additional values you can checkout in the sample config including: resized_frame_size, max_response_token, max_frames_to_process, min_frames_to_process, low_detail, update_frigate_sublabel.
I added a small system message to the GPT api request and slightly modified how the prompt is being generated. I was seeing an issue that GPT responses were not consistently proper json, with these changes it seems to be a bit better?