search

koush / scrypted

Scrypted is a high performance video integration and automation platform
https://scrypted.app
Other
3.91k stars 237 forks source link

Allow overriding the staleness of cached snapshots #1443

Closed slyoldfox closed 1 month ago

koush commented 1 month ago

Can you check if this is still necessary? There were issues with snapshot staleness that were resolved that also prevent this change from merging.

koush commented 1 month ago

If the event is to debounce snapshots for some time after a snapshot request, it can be done with this line:

            }, eventSnapshot ? 0 : 10000, async () => {

By default a snapshot request is persisted for 10 seconds.

slyoldfox commented 1 month ago

The goal of the PR was to extend (and let the user configure) the time that the snapshot is cached. The idea being that I'd like it refreshed every 5 minutes (but users can specify any time) and not the default of 10 or 20 seconds.

koush commented 1 month ago

The expectation of the takePicture method is that it returns a recent snapshot. 5 minutes is excessive. I think the defaults are correct, for how a caller expects this method to behave.

I'm generally trying to avoid adding more settings that will allow the user to shoot themselves in the foot. Why does a user need this setting? Are you trying to address a SIP/btcino specific hardware issue with fetching snapshots? Is something broken?

A script mixin could always address this customization, without having to merge it to a core plugin and be made accessible to all users.

slyoldfox commented 1 month ago

Nothing is broken on the scrypted side. The bticino intercom is not able to hold the SIP call active for a long period of time, it forces a disconnect after about 30 seconds, during the time that the SIP call is active, it's not able to receive incoming doorbell events. So I'm trying to be careful not to hoard it too much with requests.

I totally agree the defaults are sane for webcams. For this unit the refresh times were slightly too aggressive for my liking, which is why I already cache the snapshot in the plugin for 5 minutes.

No problem if you don't want too add too much settings, totally understand! Thought it was a nice addition but perfectly understand your view on this. I'll keep the code in the plugin for now!

koush commented 1 month ago

I believe arlo has similar handling for snapshots being unavailable while the stream is ongoing. I do think there's something that needs to be done here for this sort of hardware scenario, but I don't think a user configurable value is the correct solution. Something built into the plugins seems good, or maybe some hints regarding snapshot availability in getPictureOptions.

koush commented 1 month ago

Also, throwing an error in takePicture can also be a valid solution. Snapshot plugin will try to use various recovery mechanisms (snapshot from pre buffer or cache).