RSDK-8858 Add frame_rate to webcam driver

[jckras]

• Added frame_rate to monitoredWebCam properties
• Added a test in webcam_test.go to check that frame_rate was assigned properly

[randhid]

Thank you for adding the tests!

Stretch goal: look at fake camera's and image_file camera's tests and see if we can mock out the stream source to better test some of webcam's code now! I'm okay with not testing stream, there are other APIs like PointCloud that may be easier to mock up.

[randhid]

one nit! lgtm!

[jckras]

Q: The comment above the Validate function states that it ensures all parts of the config are valid, but the current implementation only checks constraints on the Width and Height fields. Should we also validate the other properties like Format, Path, and FrameRate...or if not why don't we care about them?

[hexbabe]

Q: The comment above the Validate function states that it ensures all parts of the config are valid, but the current implementation only checks constraints on the Width and Height fields. Should we also validate the other properties like Format, Path, and FrameRate...or if not why don't we care about them?

Maybe I'm lacking context but in 99% cases we certainly should

[randhid]

Q: The comment above the Validate function states that it ensures all parts of the config are valid, but the current implementation only checks constraints on the Width and Height fields. Should we also validate the other properties like Format, Path, and FrameRate...or if not why don't we care about them?

Good question, I believe those checks are done in the constructor, and I do believe we try to use media devices to find the best parameters in the constructor, so we allow you to try to create the camera with fewer configured attributes, @seanavery will know more about how mediadeivces is operating.

[seanavery]

Q: The comment above the Validate function states that it ensures all parts of the config are valid, but the current implementation only checks constraints on the Width and Height fields. Should we also validate the other properties like Format, Path, and FrameRate...or if not why don't we care about them?

For format, our mediadevices backend can only parse a certain set of pixel formats. See here.

We also have a hardcoded list of available width/height resolutions in mediadevices. See here.

The mediadevices matcher will attempt to find a driver for the given properties. It is strict, so that if a width/height/format/frame_rate combination is not found then the webcam will fail to initialize.

[seanavery]

LGTM.

It may be worth adding a simple validation to frame_rate in the Validate function as a part of this PR.

[randhid]

LGTM.

It may be worth adding a simple validation to frame_rate in the Validate function as a part of this PR.

The only non-breaking validation would be checking if it's less than zero which we should add. If we make something required the user has to know to add it now. Normally I'm okay with doing this on the fly with other components that have lower useage, but webcam is one of the ones on the viam rover fragment and has that scary component count.

[jckras]

Q: The comment above the Validate function states that it ensures all parts of the config are valid, but the current implementation only checks constraints on the Width and Height fields. Should we also validate the other properties like Format, Path, and FrameRate...or if not why don't we care about them?

For format, our mediadevices backend can only parse a certain set of pixel formats. See here.

We also have a hardcoded list of available width/height resolutions in mediadevices. See here.

The mediadevices matcher will attempt to find a driver for the given properties. It is strict, so that if a width/height/format/frame_rate combination is not found then the webcam will fail to initialize.

Q: The comment above the Validate function states that it ensures all parts of the config are valid, but the current implementation only checks constraints on the Width and Height fields. Should we also validate the other properties like Format, Path, and FrameRate...or if not why don't we care about them?

For format, our mediadevices backend can only parse a certain set of pixel formats. See here.

We also have a hardcoded list of available width/height resolutions in mediadevices. See here.

The mediadevices matcher will attempt to find a driver for the given properties. It is strict, so that if a width/height/format/frame_rate combination is not found then the webcam will fail to initialize.

this might not be the best way to ask this question, but when are those packages being used to validate the properties? I have been clicking through the repo but still don't see where the calls in webcam connect back to those files you linked

nevermind: I see now in makeConstraints

[randhid]

Sorry, now I need the check to change in Validate, in the constructor, it would have been fine to add a less than or equal to zero, but not in Validate

[jckras]

Tested webcam locally with a negative frame rate and got the correct error: