[RSDK-8274, RSDK-8624, RSDK-8647, RSDK-8687] Video Storage and Save/Fetch DoCommand #7

seanavery · 2024-09-04T16:26:23Z

Description

This PR includes a POC for the video-store module. The module contains the following core elements:

cam.go Camera interface, module configuration, do_command handlers.
encoder.go Encodes frames from source camera.
segmenter.go Splits encoded feed into video segments.
concater.go Combines video segments for save/fetch.
utils.go Various ffmpeg, datetime, and file helpers.

Tests

config_test.go Tests module configuration and validation.
save_test.go Tests save do_command.
fetch_test.go Tests fetch do_command.

FFmpeg Refs

Profiling

TODO

Simple detect tagging

randhid

Few nits, I'd like more testing and sanitization around the datamanager filepath, since that's a big initial useability pain point. We can make the storage more robust and cpu in a follow up ticket, that can be part of the epic since it's directly related to the scope asks.

randhid · 2024-09-10T16:52:00Z

cam/cam.go

+type Config struct {
+	Camera  string  `json:"camera"`
+	Sync    string  `json:"sync"`
+	Storage storage `json:"storage"`
+	Video   video   `json:"video,omitempty"`
+
+	// TODO(seanp): Remove once camera properties are returned from camera component.
+	Properties cameraProperties `json:"cam_props"`
+}


[readability] Add validate immediately after the config struct.

randhid · 2024-09-10T16:58:24Z

cam/cam.go

+
+	// Create segmenter to handle segmentation of video stream into clips.
+	if newConf.Storage.SegmentSeconds == 0 {
+		newConf.Storage.SegmentSeconds = defaultSegmentSeconds


do not overwrite the config struct, I can't tell if there will ever be a case with a read/write race in this code as of now, so let's do the dumb but safe thing and just set a variable to a default and overwrite it with the configured value. Do the same fo other writes of the config variable in the constructor code.

Sounds good - moved to that flow.

randhid · 2024-09-10T17:02:55Z

cam/cam.go

+	if newConf.Storage.UploadPath == "" {
+		newConf.Storage.UploadPath = filepath.Join(getHomeDir(), defaultUploadPath, vs.name.Name)


[q] we test this specific part of your pr for all edge cases?

upload path unset

bad/malformed upload path

If path is unset uses default $HOME/.viam/capture/video-upload

If path is malformed it will error out when it attempts to create the directory later on in the constructor.

randhid · 2024-09-10T17:05:22Z

cam/cam.go

+			return
+		default:
+		}
+		frame, _, err := vs.stream.Next(ctx)


[q] why do we never call release returned from Next here?

Good call out, I was meaning to add a TODO comment here.

Added in release call after encoding. Tested that things are still working.

tests/config_test.go

randhid · 2024-09-10T17:09:55Z

tests/config_test.go

+		test.That(t, err, test.ShouldBeNil)
+		defer r.Close(timeoutCtx)
+		_, err = camera.FromRobot(r, videoStoreComponentName)
+		test.That(t, err, test.ShouldNotBeNil)


Test the error, since you gave this a timeoutCtx to test with.

Added check for error msg contents.

hexbabe

Good stuff!! LGTM

hexbabe · 2024-09-10T16:22:25Z

Makefile

+               --enable-protocol=crypto \
+               --enable-bsf=h264_mp4toannexb
+
+# TODO(seanp): cleanup libx264 static link.


what's currently wrong with the static link?

Linked by absolute path instead of by lib name.

Relies on libx264 static build in canon.

I think relying on canon build is fine since we can use canon containers in CI matrix.

Removed TODO comment.

hexbabe · 2024-09-10T18:00:41Z

README.md

+    }
+```
+
+## Do Commands


Why do we need to document do commands? Is it so that someone could integrate with this module in their own app if they wanted to?

Yeah, I think we need to document DoCommands in the README since the module is pretty much worthless without save/fetch interface.

hexbabe · 2024-09-10T18:04:15Z

cam/cam.go

+	p, err := vs.cam.Properties(ctx)
+	if err == nil {
+		p.SupportsPCD = false
+	}


confused at the above conditional, why does having no error mean that supporting pcd is false

This camera has no intrinsics or distortion parameters in it's config, and so has no way of producing pointclouds from the underlying 2D image. The camera it depends on might be able to, and we could use the intrinsic and distortion properties from the underlying camera to produce point clouds, but we don't need to and don't, so we tell the truth about this camera's properties - it does not support point clouds.

A question here though - do we need to pass through the dependent camera's properties?

Good points - I think we should just return empty props here.

Matt Vella asked me about returning images from underlying camera here for the Event Manager, which would then make sense to have this props passthrough.

But AFAIK, the current plan is to use GetDetectionsFromCamera from the Vision Service.

Cool, check in with him again, but let's get this merged and a POC module in the registry first!

seanavery · 2024-09-11T14:53:16Z

OK merged code in. Had to do a force push to main instead of merging against empty-ref branch in this PR. Closing out.

randhid · 2024-09-11T15:01:47Z

OK merged code in. Had to do a force push to main instead of merging against empty-ref branch in this PR. Closing out.

To keep track of what was merged in, you can create commits so that JIRA can track them too:

Or just make a pr with the linked tickets. I wouldn't recommend using an empty ref in the future. It's okay, you can ask people to expand the code and read it for more context.

seanavery added 30 commits August 5, 2024 16:26

poc in progress

ad7c9b4

Remove segment wrap

6d80dad

Add deleter routine

893d866

Clean up path setters

4d08c13

Remove segment list and csv helpers

c882b9b

Add copy helper

7fa5f79

fsnotify working

2ab2177

Simple detection handling

759fd7a

Add upload path setter

bc0f91c

Add mutex

e2d1bb2

Remove stale copier

aab2f56

Change detect config

a3625b4

Remove outdated test

57315de

Change output pattern

cd1342f

Quick fix to storage deleter

dc0f851

Remove extra spacing

407b660

Remove dotenv

6b7f6fe

Merge pull request #1 from viam-modules/simple-detect-tagging

b903538

Simple detect tagging

Comments

4920daf

happy lint

9d5513d

Add folder creators for storage/upload

e3e6ae2

Comments are helpful

a365090

Try lint in CI

9e898d1

Test lint

7c56445

Add ffmpeg build dep to lint

0a62bf1

Support x86 build

bc44c46

Install libx264 if not avail

2b8312d

Add comments to enoder

215a839

More comments

7fde866

Install libx264

7154f76

seanavery added 9 commits September 9, 2024 10:52

Check sync service has subtype data_manager

a7a174a

Add git lfs config

ac31aa2

Add test video clips

ab9f2d5

Add save do_command tests

4e8d545

Add invalid datetime save test

e7909ee

Format fix

d171597

Pull in fetch

6b98fa0

Add todo comments for fetch

0706a70

Add fetch tests

d67ae6f

seanavery changed the title ~~POC: Video Storage and Save do_command~~ [RSDK-8274, RSDK-8624, RSDK-8647, RSDK-8687] Video Storage and Save/Fetch DoCommand Sep 9, 2024

seanavery added 4 commits September 9, 2024 16:27

Tmp path for fetch

e20b90d

Check max grpc size

011210a

Update readme

8230a5d

Filename

d54a527

seanavery requested review from randhid and hexbabe September 10, 2024 14:37

Add clean script

204f579

randhid approved these changes Sep 10, 2024

View reviewed changes

Move validate

1dd46bf

hexbabe approved these changes Sep 10, 2024

View reviewed changes

seanavery added 7 commits September 10, 2024 14:18

Do not overwrite the config

c67818e

Release frame after encode

b3416e5

Check cam return is nil

a5b998a

Check error msg

cae1464

Remove todo comment

14efd1c

Remove props passthrough

7ad11db

Lint

3380049

seanavery closed this Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RSDK-8274, RSDK-8624, RSDK-8647, RSDK-8687] Video Storage and Save/Fetch DoCommand #7

[RSDK-8274, RSDK-8624, RSDK-8647, RSDK-8687] Video Storage and Save/Fetch DoCommand #7

seanavery commented Sep 4, 2024 •

edited

Loading

randhid left a comment

randhid Sep 10, 2024

seanavery Sep 10, 2024 •

edited

Loading

randhid Sep 10, 2024

seanavery Sep 10, 2024

randhid Sep 10, 2024

seanavery Sep 10, 2024

randhid Sep 10, 2024

seanavery Sep 10, 2024

randhid Sep 10, 2024

seanavery Sep 10, 2024

hexbabe left a comment

hexbabe Sep 10, 2024

seanavery Sep 10, 2024

hexbabe Sep 10, 2024

seanavery Sep 10, 2024

hexbabe Sep 10, 2024

randhid Sep 10, 2024

seanavery Sep 10, 2024

randhid Sep 10, 2024

seanavery commented Sep 11, 2024

randhid commented Sep 11, 2024

		if newConf.Storage.UploadPath == "" {
		newConf.Storage.UploadPath = filepath.Join(getHomeDir(), defaultUploadPath, vs.name.Name)

[RSDK-8274, RSDK-8624, RSDK-8647, RSDK-8687] Video Storage and Save/Fetch DoCommand #7

[RSDK-8274, RSDK-8624, RSDK-8647, RSDK-8687] Video Storage and Save/Fetch DoCommand #7

Conversation

seanavery commented Sep 4, 2024 • edited Loading

Description

Tests

FFmpeg Refs

Profiling

randhid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanavery Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hexbabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanavery commented Sep 11, 2024

randhid commented Sep 11, 2024

seanavery commented Sep 4, 2024 •

edited

Loading

seanavery Sep 10, 2024 •

edited

Loading