search

scottlamb / moonfire-nvr

Moonfire NVR, a security camera network video recorder
Other
1.2k stars 138 forks source link

Api call to retrieve API fails #177

Closed clydebarrow closed 2 years ago

clydebarrow commented 2 years ago

Describe the bug

The /api/cameras/<UUID>/<stream>/view.mp4?s=<spec> endpoint returns a 400 error when the <spec> includes a time range with no upper bound.

To Reproduce

Test the sample endpoint call as per the API documentation e.g.

/api/cameras/fd20f7a2-9d69-4cb3-94ed-d51a20c3edfe/main/view.mp4?s=1.26

Expected behavior

This should return a valid MP4 stream (assuming the UUID is valid)

Actual Behaviour

The call returns a 400 error with the message "invalid s parameter: 1.26".

Providing an end point, e.g. s=1.26-90000 works.

scottlamb commented 2 years ago

Hmm, above it says START_ID[-END_ID][@OPEN_ID][.[REL_START_TIME]-[REL_END_TIME]], so ending with a hyphen to start at time 26 and extend to the end. The code matches this. So I think I just need to fix the example to include that hyphen.

(It might seem inconsistent for the time range to expect a hyphen when you only give a start and the ids to not expect a hyphen when you only give a start. But this inconsistency is because the two behave differently. When there's only a start time, the time range goes to the end of the id(s) you specified. When there's only a start id, it ends at the same id.)

clydebarrow commented 2 years ago

Consistency wasn't the issue - the example was very clear, and claimed to do exactly what I wanted, so it never occurred to me that it was not intended to work that way.

I had already worked around it by providing an explicit end time - taking the end time of the recording in 90k units and converting to seconds. That presumably runs the risk of truncating the last fraction of a second, but that's not a big deal.

Apropos consistency, why is it in seconds when AFAIK everything else is in 90k units?

scottlamb commented 2 years ago

It's supposed to be in 90k units. That's not the behavior you're seeing? I'm surprised.

clydebarrow commented 2 years ago

Doh! Yes, it is - I'm using seconds internally...

clydebarrow commented 2 years ago

And that's partly because the html video element has a currentTime attribute that I use to keep playback across multiple streams in sync without having to restart the stream, and it's in seconds.

scottlamb commented 2 years ago

Yeah, I think that conversion in the UI is probably needs to stay. As you said, HTMLMediaElement.currentTime is in float seconds. We might change the API unit when adding audio support (TBD), but we clearly want to keep having sub-second precision, and unless you have a strong argument otherwise, I'd prefer to keep using integers. They're easier for me to reason about than floats.