Skip to content
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

add eventstart and eventend actions. #2691

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

clavay
Copy link

@clavay clavay commented Jan 25, 2023

Hello,

I needed to manually start recording movies using actions but 'record_start' and 'record_end' actions don't do nothing. (see actions.py)

I add the eventstart and eventend action to available actions (copying the snapshot action).

This works with motion 4.3.2.
With motion 4.5.1 it doesn't work as expected because eventstart action makes motion create records continuously as if recording mode is set to 'Continuous recording' (but it is not).

@MichaIng
Copy link
Member

Many thanks for your contribution. Since motion v4.5.1 is installed by default with motioneye_init, we should figure out why the behaviour changed and fix this. If it is really an API change and not a missing config conversion of so, we have a function to obtain the (cached) motion version as well, for conditional steps.

I needed to manually start recording movies
...
With motion 4.5.1 it doesn't work as expected because eventstart action makes motion create records continuously

So isn't continuous recording what you wanted in your particular case 😄? However, I agree, this should trigger whatever is configured to happen when a motion event is detected.

@MichaIng
Copy link
Member

I'm not sure what the safety check are telling us, since setuptools-66.1.1 is installed not the affected one. And I do not see it being downgraded due to some versioned dependency either 🤔.

I'll adjust the locales update to run only when head branch is from our repo. Even that the workflow is triggered from this repo, secrets seem to be used from the head repo, where they can be missing. A security precaution, to not allow anyone pulling secrets from other repos via PR, I guess. Makes sense.

@clavay
Copy link
Author

clavay commented Jan 26, 2023

Many thanks for your contribution. Since motion v4.5.1 is installed by default with motioneye_init, we should figure out why the behaviour changed and fix this. If it is really an API change and not a missing config conversion of so, we have a function to obtain the (cached) motion version as well, for conditional steps.

I have the same behavior when I start an event using the motion webcontrol (using http://127.0.0.1:7999/1/action/eventstart).
So I think something changed in motion.

So isn't continuous recording what you wanted in your particular case smile? However, I agree, this should trigger whatever is configured to happen when a motion event is detected.

If I want continuous recording I change the recording_mode parameter, but in this case I want to start a movie (or an event that start a movie) manually (like having an external trigger).

@clavay
Copy link
Author

clavay commented Jan 26, 2023

I found that this motion commit introduce the bug : 262b6b0ac62f92fa2410bdc556a10daacb28e7f3

I open a motion issue : Motion-Project/motion#1625

@MichaIng
Copy link
Member

MichaIng commented Jan 29, 2023

To be sure I understand it correctly:

  • You want a single snapshot/picture to be created on eventstart instead of a continuous movie until eventend, right?
  • You did not change the motion config (in motionEye GUI) to still images with "Motion Triggered (One Picture)" mode?

I would expect motion to behave on eventstart just like like a real motion event was detected, respecting the settings in the config. As still images with one picture is a motion setting which motionEye uses, I would expect this to work. But I also expect an eventend action to be required at some point.

If you really want a single snapshot/picture created via trigger, I however would just use the snapshot motionEye action or motion parameter (same name on both).


Btw, I solved the CI errors on base branch, and your code basically looks very fine. I'm just not sure about the use case. We shouldn't add actions if there is not really a use case which cannot be addressed better with the snapshot action, a definit recording start/end action that does not rely on motion detection events, and for external API/webhook calls: If this cannot be better done via motion directly, instead of going the loop through motionEye first.

@clavay
Copy link
Author

clavay commented Jan 30, 2023

To be sure I understand it correctly:

* You want a single snapshot/picture to be created on `eventstart` instead of a continuous movie until `eventend`, right?

No, I want to start the record of a movie with an external trigger. I use the event functionality of motion.
As the MotionEye action record_start and record_stop don't do anything, I add the eventstart and eventend action to MotionEye that trigger the event as snapshot do.
Since the version 4.4 of motion the result of triggering an event changed as explained here
Now (since motion 4.4) starting an event will record movies (of the length defined in the movie_max_time setting) till eventend is triggered.

I would expect motion to behave on eventstart just like like a real motion event was detected, respecting the settings in the config.

My changes are doing what is expected : start and end an event in motion.
The result after (make a movie, take a snapshot...) depend on the motion configuration.

@MichaIng
Copy link
Member

MichaIng commented Jan 30, 2023

Ah I think I understand now: Until v4.4, there was a single movie_max_time length movie recorded only and recording stopped afterwards even without eventend? I agree with motion devs then that this is unexpected and inconsistent with how motion behaves on real motion events. One expects all motion to be recorded until there is none anymore, instead of only the first movie_max_time.

Why not sending eventend at a defined time after eventstart, at which your desired recording length has been reached? movie_max_time would then need to be a little longer to assure no 2nd short movie is started.

Further questions regarding the need of this PR:

  1. Why do you not call motion directly but motionEye instead? Open ports, IP binding, firewalls can be good reasons, just want to be sure.
  2. Wouldn't it be for your particular use case better to implement the record_start/record_end actions properly, so allow recording independent of other implications the motion event start may have? There is also an explicit snapshot action, so this would be consistent.

@clavay
Copy link
Author

clavay commented Jan 30, 2023

Ah I think I understand now: Until v4.4, there was a single movie_max_time length movie recorded only and recording stopped afterwards even without eventend? I agree with motion devs then that this is unexpected and inconsistent with how motion behaves on real motion events. One expects all motion to be recorded until there is none anymore, instead of only the first movie_max_time.

Yes this is the change since 4.4.

Why not sending eventend at a defined time after eventstart, at which your desired recording length has been reached? movie_max_time would then need to be a little longer to assure no 2nd short movie is started.

Yes this is my solution.

Further questions regarding the need of this PR:

1. Why do you not call motion directly but motionEye instead? Open ports, IP binding, firewalls can be good reasons, just want to be sure.

Because I use the motionEye frontend and I use motioneye_client inside a SCADA system plugin.

2. Wouldn't it be for your particular use case better to implement the `record_start`/`record_end` actions properly, so allow recording independent of other implications the motion event start may have? There is also an explicit snapshot action, so this would be consistent.

Yes but I don't see how to do it using motion web control commands
Only eventstart, eventend, snapshot, restart, quit and end actions are available.

@MichaIng
Copy link
Member

MichaIng commented Jan 30, 2023

Valid points. We should then also remove the record_start/record_end actions, after checking that they are not somehow used internally as dummies. But can/should be done in a new PR.

Adding some other reviewers, just in case, but good to merge from my side.

Copy link
Collaborator

@zagrim zagrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me.

I didn't do the due digging, but the existence of actions record_start and record_stop is rather odd as they both seem to be no-ops, at least currently. IMO they should be (eventually) removed unless someone figures out some use for them (in which case they should be wired to some action in the code, too). #1248 looks like the code really never was written for that kind of thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants