RSDK-8647 - Add save do_command #6

seanavery · 2024-09-03T18:09:46Z

Description

Adds FFmpeg configure flags for static build to support concat demuxing of mp4 files.
Adjusts module configuration to match latest tech spec details.
Adds a concater struct to handle FFmpeg concat demuxer runtime.
- Handles writing/clearing temprorary concat.txt file used for demuxer configuration.
- Handles copying codec configuration from input.
- Adjusts frame timestamps to satisfy a single continuous video.
Adds interface and validation for save do_command.
Adds helpers to match save queries with available storage files.

This PR does not include encoder, segmeneter, and deleter code in the diff.

This contains a simple datetime matcher for storage files. Will need to follow up with a more advanced demxuer that can handle file trimming.

Open to comments/reviews on non-diffed code. Reviewers can expand the encoder.go, segmenter.go, and cam.go files in the Files Changed tab for full context.

Refs

FFmpeg concat demuxer docs

Test

From/To requests outside of storage range fail with error ✅
From/To requests inside storage range are successful ✅
From/To requests with invalid date-time format error ✅
Concated video files are playable ✅
Save do_command response contains uploaded file name ✅

Tests were run with a webcam source and the following config:

{
  "components": [
    {
      "name": "wc-cam",
      "namespace": "rdk",
      "type": "camera",
      "model": "webcam",
      "attributes": {
        "height_px": 480,
        "frame_rate": 25,
        "format": "YUYV",
        "video_path": "/dev/video0",
        "width_px": 640
      }
    },
    {
      "name": "video-store",
      "namespace": "rdk",
      "type": "camera",
      "model": "seanavery:video:storage",
      "attributes": {
        "storage": {
          "segment_seconds": 30,
          "size_gb": 100
        },
        "video": {
          "preset": "medium",
          "format": "mp4",
          "codec": "h264",
          "bitrate": 1000000
        },
        "cam_props": {
          "framerate": 25,
          "width": 640,
          "height": 480
        },
        "camera": "wc-cam",
        "vision": "vision-1"
      },
      "depends_on": [
        "wc-cam"
      ]
    }
  ],
  "services": [
    {
      "name": "data_manager-1",
      "namespace": "rdk",
      "type": "data_manager",
      "attributes": {
        "capture_dir": "",
        "tags": [],
        "additional_sync_paths": [],
        "capture_disabled": true,
        "sync_interval_mins": 0.1
      }
    }
  ],
  "modules": [
    {
      "type": "local",
      "name": "video-storage",
      "executable_path": "/home/viam/video-store",
      "log_level": "debug"
    }
  ]
}

Example save command:

{
  "command": "save",
  "from": "2024-08-29_16-32-44",
  "to": "2024-08-29_17-08-48",
  "metadata": "person-detector_person1-0.96"
}

Response:

{
  "command": "save",
  "file": "video-store-1_2024-08-29_16-32-44_person-detector_person1-0.96.mp4"
}

hexbabe · 2024-09-04T14:08:49Z

Should we review POC first?

seanavery · 2024-09-04T15:05:11Z

Should we review POC first?
@hexbabe Will resubmit this PR against an empty ref for full context of the code in review.

randhid

This is going to be a giant model, I like how you're broken out the file into concater and segmented, can I request that we break out the setup functionality (registration, validation, creator and close) into it's own file too?

I would like to add tests at this point to lock in some of your utils and Validation functionality.

I will look at cam.go in a separate review.

randhid · 2024-09-04T16:10:38Z

cam/cam.go

@@ -19,37 +19,43 @@ import (
 )

 // Model is the model for the video storage camera component.
-// TODO(seanp): Personal module for now, should be movied to viam module in prod.
+// TODO(seanp): Personal module for now, should be moved to viam module in prod.


it's in viam-modules now.

randhid · 2024-09-04T16:11:34Z

cam/cam.go

-	uploadPath string
+	name   resource.Name
+	conf   *Config
+	logger logging.Logger

 	cam    camera.Camera
 	stream gostream.VideoStream

 	workers rdkutils.StoppableWorkers


If you bump rdk, which I suggest you do you'll hae to get this from goutils goutils.NewBackgroudnWorkers is the creator function.

Switched to latest NewBackgroundWorkes in goutils.

randhid · 2024-09-04T19:13:28Z

README.md

        "storage": {
-            "clip_seconds": 30,
+            "segment_seconds": 30,
            "size_gb": 100


Does the save DoCommand have an optional directory for video storage and sync upload?

Not in do_command, those are optional attrs in the module - will include it in the README example.

randhid · 2024-09-04T19:14:56Z

README.md

-        "additional_sync_paths": [
-          "/home/viam/.viam/video-upload/"
-        ],
+        "additional_sync_paths": [],


Call out that this is custom, as stated in the scope, right now there's no clue as to why this is int he example config.

randhid · 2024-09-04T19:16:32Z

cam/cam.go

 	defaultStoragePath    = ".viam/video-storage"
+
+	defaultLogLevel = "info"
+	deleterInterval = 60 // seconds
 )

 type videostore struct {
 	resource.AlwaysRebuild
 	resource.TriviallyCloseable


You have a Close implemented, which is what I expect since we're going on and on on a loop saving video. You can delete this line, which just returns nil when you call close.

Suggested change

resource.TriviallyCloseable

randhid · 2024-09-04T20:02:50Z

cam/cam.go

 	ffmppegLogLevel(logLevel)

+	// Create encoder to handle encoding of frames.
 	// TODO(seanp): Forcing h264 for now until h265 is supported.
 	if newConf.Video.Codec != "h264" {


Let's premptively factor this into an enum.

Ok, added enum+handlers for codec type.

randhid · 2024-09-04T20:04:20Z

cam/cam.go

@@ -138,7 +144,6 @@ func newvideostore(
 	if newConf.Video.Format == "" {
 		newConf.Video.Format = defaultVideoFormat


I like the pattern of

s.thingy = defaultThingy if newConf.Thingy != nil/empty{ s.thingy = newConf.Thingy }

But it might be messy here unless you make a new encoder with the logger first. You can also move this logic to the newSegmenter and newEncoder functions since you're passing in the config variables anyway.

randhid · 2024-09-04T20:09:28Z

cam/cam.go

@@ -19,37 +19,43 @@ import (
 )

 // Model is the model for the video storage camera component.
-// TODO(seanp): Personal module for now, should be movied to viam module in prod.
+// TODO(seanp): Personal module for now, should be moved to viam module in prod.
 var Model = resource.ModelNamespace("seanavery").WithFamily("video").WithModel("storage")


it's okay to use the viam namespace, it's also okay to transfer a release later, just try to only upload release candidates to the registry for now.

ok, moved to viam namespace

randhid · 2024-09-04T20:25:13Z

cam/utils.go

+	}
+	return matchedFiles
+}
+


Let's add some test coverage for correctly formatting these filenames at this point. That's a good thing to check on and will test your utils, which are all helper functions that should not change much. It will also allow you to test out some weird user config silliness.

randhid · 2024-09-04T20:29:04Z

cam/cam.go

-	vs.seg.Close()
+	vs.enc.close()
+	vs.seg.close()
+	vs.conc.close()
 	return nil


I don't see validation logic for the data manager being present in the configuration anywhere unless I missed it; that is part of the scope. However, I think that should be a separate ticket, I don't seeit mentioned in the epic's current tasks.

Good call - still need to add this. May need to amend the scope doc to include the data manager service as a mandatory dependency in the config.

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

I don't need to review this right? Looks like a lot of iterations have been made since this in the other PR

randhid · 2024-09-10T19:03:04Z

I don't need to review this right? Looks like a lot of iterations have been made since this in the other PR

@seanavery please close this.

seanavery added 26 commits August 28, 2024 14:39

Remove debug ffmpeg logger

065b157

Add concat and h264 parser

fcdf787

Concat initialization

d2b40fc

Add annexb bitstream filter

e2aeb08

Concat writes working

d9e52ac

Input validation

58392d0

Validate and match by time range

39c871c

Handle metadata string

fae564e

Abstract datetime parser

ca23a8d

Create new concat struct

f5881e3

Cleanup

30762d9

Delete fetch comp name

a5654e8

close

d752970

Comment helpers

aaea5ec

concat

e5a1c4c

Add comments to concat flow

0422835

Specify concat file location

3322c46

Fix upload path

ce73a77

Update readme

2db1577

Lint

63b4f06

Ignore bin

0129fed

New line

991dbf5

Breakup long pointer arithmetic lines

1173352

Add more comments to concat operation

5029eb0

Improve validate flow

8093d04

cleanup

df64d71

seanavery requested review from randhid and hexbabe September 3, 2024 20:47

seanavery changed the title ~~Save cmd~~ RSDK-8147 - Add save do_command Sep 4, 2024

Fix upload file name

94f4c19

seanavery mentioned this pull request Sep 4, 2024

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

Closed

randhid requested changes Sep 4, 2024

View reviewed changes

seanavery closed this Sep 10, 2024

seanavery changed the title ~~RSDK-8147 - Add save do_command~~ RSDK-8647 - Add save do_command Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-8647 - Add save do_command #6

RSDK-8647 - Add save do_command #6

seanavery commented Sep 3, 2024 •

edited

Loading

hexbabe commented Sep 4, 2024

seanavery commented Sep 4, 2024

randhid left a comment

randhid Sep 4, 2024

randhid Sep 4, 2024

seanavery Sep 5, 2024

randhid Sep 4, 2024

seanavery Sep 5, 2024

randhid Sep 4, 2024

randhid Sep 4, 2024

randhid Sep 4, 2024

seanavery Sep 5, 2024

randhid Sep 4, 2024

randhid Sep 4, 2024

seanavery Sep 5, 2024

randhid Sep 4, 2024

randhid Sep 4, 2024

seanavery Sep 5, 2024

hexbabe commented Sep 10, 2024

randhid commented Sep 10, 2024

		@@ -138,7 +144,6 @@ func newvideostore(
		if newConf.Video.Format == "" {
		newConf.Video.Format = defaultVideoFormat

RSDK-8647 - Add save do_command #6

RSDK-8647 - Add save do_command #6

Conversation

seanavery commented Sep 3, 2024 • edited Loading

Description

Refs

Test

hexbabe commented Sep 4, 2024

seanavery commented Sep 4, 2024

randhid 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

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 commented Sep 10, 2024

randhid commented Sep 10, 2024

seanavery commented Sep 3, 2024 •

edited

Loading