Skip to content

RSDK-13042 — video-store using GetImages continues even if input is invalid repeatedly#87

Open
hexbabe wants to merge 2 commits intoviam-modules:mainfrom
hexbabe:RSDK-13042-storing-stale-frames
Open

RSDK-13042 — video-store using GetImages continues even if input is invalid repeatedly#87
hexbabe wants to merge 2 commits intoviam-modules:mainfrom
hexbabe:RSDK-13042-storing-stale-frames

Conversation

@hexbabe
Copy link
Collaborator

@hexbabe hexbabe commented Mar 12, 2026

RSDK-13042

Bug Summary

Steps to reproduce :

Use depth camera e.g. Realsense and hook it up as the underlying camera to a video-store.

Configure the camera to return only a color image at first and store some video, then change it to return only a depth map in its GetImages response e.g. RealSense.

Expected behavior :

Returns depth mime type invalid error, and stops storing video.

Actual behavior :

Returns depth mime type invalid error, but continues storing stale color frame as video.

Manual Tests

Config:

{
  "components": [
    {
      "name": "vs",
      "api": "rdk:component:camera",
      "model": "viam:video:storage",
      "attributes": {
        "camera": "oak-d",
        "sync": "data_manager-1",
        "storage": {
          "size_gb": 1
        }
      },
      "depends_on": [
        "data_manager-1"
      ]
    },
    {
      "name": "oak-d",
      "api": "rdk:component:camera",
      "model": "viam:luxonis:oak-d",
      "attributes": {
        "sensors": [
          "color"
        ]
      }
    }
  ],
  "services": [
    {
      "name": "data_manager-1",
      "api": "rdk:service:data_manager",
      "model": "rdk:builtin:builtin",
      "attributes": {
        "additional_sync_paths": [],
        "sync_interval_mins": 0.1,
        "capture_dir": "",
        "tags": []
      }
    }
  ],
  "modules": [
    {
      "type": "registry",
      "name": "viam_oak",
      "module_id": "viam:oak",
      "version": "latest"
    },
    {
      "type": "local",
      "name": "local-vs",
      "executable_path": "/Users/seanyu/Projects/sean-video-store/bin/darwin-arm64/video-store"
    }
  ]
}

Verified video saves well. Change sensors from ["color"] to ["depth]. Observed:

3/12/2026, 1:45:28 PM warn local-vs.rdk:component:camera/vs     videostore/videostore.go:583 expected image mime type image/jpeg got image/vnd.viam.dep:   log_ts 2026-03-12T17:45:28.138Z

and no more video saved.

After changing back to ["color"] there is one corrupted video, but the video resource recovers and keeps writing segments.

PR

2 reviewers

This was not vibe coded sorry for the slop earlier

TODO: make an RC, bump viamrtsp, and consume this change there

@hexbabe hexbabe marked this pull request as ready for review March 12, 2026 17:46
Copy link
Collaborator

@seanavery seanavery left a comment

Choose a reason for hiding this comment

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

Manual tests looking good, but wondering about the case in which the camera GetImages endpoint recovers back to color within a segment boundary. Does the encoder and resulting video file recover?

}
}
})

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a test case here for valid jpeg mime_type but invalid image bytes? to make sure we clear in that case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes I will add.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looked into it: there's no explicit path in our code that checks and propagates a sentinel error we own on bad bytes. Want me to add that? Or would you rather assert on err string from underlying lib?

// clearLatestFrame sets the latest frame atomic to a nil byte slice.
// Useful to remove stale frames when underlying cam is unhealthy.
func (vs *videostore) clearLatestFrame() {
vs.latestFrame.Store([]byte(nil))

Choose a reason for hiding this comment

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

[nit] We may get a small time win here if we only store a nil byte if its content is not nil already (avoiding the atomic operations). But totally fine the way this is.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@hexbabe
Copy link
Collaborator Author

hexbabe commented Mar 13, 2026

Manual tests looking good, but wondering about the case in which the camera GetImages endpoint recovers back to color within a segment boundary. Does the encoder and resulting video file recover?

yes I actually tested this but forgot to include in the writeup. Thanks for asking!

edit: Description updated.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants