| Message ID | 20240325-obsbot-quirk-fix-relative-ptz-speed-v1-1-0eb1387d98c7@securitylive.com |
|---|---|
| State | New |
| Headers | show |
| Series | uvcvideo: Remo OBSBOT quirk fix for incorrect relative min pan/tilt/zoom speeds | expand |
Hi John, On Mon, 2024-03-25 at 19:38 -0500, John Bauer via B4 Relay wrote: > From: John Bauer <johnebgood@securitylive.com> > > The OBSBOT series of cameras misreports the minimum relative > pan_speed, tilt_speed and zoom_continuous v4l2 controls resulting > in the inability to control the camera with finesse with an analog > stick or other programmatic methods. This patch applies to all > Remo (OBSBOT) vendor cameras with the vendor ID of 0x3564. If the > vendor fixes the firmware this behavior should still remain valid. > With this broad vendor fix when new devices are released the kernel > module won't need to be modified. When the vendor fixes the firmware the > device list can be modified with finer grained device filters. I don't think they misreports the values. The UVC 1.5 spec says: The bPanRelative field is used to specify the pan direction to move. A value of 0 indicates to stop the pan, a value of 1 indicates to start moving clockwise direction, and a value of 0xFF indicates to start moving counterclockwise direction. <<The GET_DEF, GET_MIN, GET_MAX and GET_RES requests will return zero for this field.>> The bTiltRelative field is used to specify the tilt direction to move. A value of zero indicates to stop the tilt, a value of 1 indicates that the camera point the imaging plane up, and a value of 0xFF indicates that the camera point the imaging plane down. <<The GET_DEF, GET_MIN, GET_MAX and GET_RES requests will return zero for this field.>> The bZoom field indicates whether the zoom lens group is stopped or the direction of the zoom lens. A value of 1 indicates that the zoom lens is moved towards the telephoto direction. A value of zero indicates that the zoom lens is stopped, and a value of 0xFF indicates that the zoom lens is moved towards the wide-angle direction. <<The GET_MIN, GET_MAX, GET_RES and GET_DEF requests will return zero for this field.>> So they can't report negative values for GET_MIN, therefore the GET_MIN of bPanSpeed, bTiltSpeed, b(Zoom)Speed will be the minimum speed of any direction. I think it will work with any PTZ camera without the quirk. Regards, Gergo
Hi Jon, Hi Gergo On Tue, 26 Mar 2024 at 07:23, John Bauer <john@oxt.co> wrote: > > After looking through the current implementation all of the proper checks are done in the getter and setter for the pan/tilt/zoom controls so the only change needed is the 2 locations to get/check/set the minimum when needed. Thankfully all the code that does the hard work is already implemented. I'll be submitting another patch that summarizes our findings. My issue with the spec is that it is not clear about what GET_MIN should return. Is it the minimum absolute value for that control, or the minimum value in that direction? In other words, can we have a device with a range (-10,20) (-A,B), or only (-20,20) (-A,A) is allowed. If there is no device that supports (-A,B), then we do not need a quirk. Regards! > > Thanks, > John > > > > On Mon, Mar 25, 2024 at 10:42 PM John Bauer <john@oxt.co> wrote: >> >> Ok, I get you now Gergo, I think I got lucky and I think you're right! Digging into the UVC 1.5 spec I can see why this works, the first byte in each 2 byte pair signifying the direction is just getting the signed bit set when a negative value is applied to both bytes so there should probably be some checks. >> >> Here from the UVC 1.5 spec: CT_PANTILT_RELATIVE_CONTROL >> +--------+---------------+------+---------------+------------------------------------------------+ >> | Offset | Field | Size | Value | Description | >> +--------+---------------+------+---------------+------------------------------------------------+ >> | 0 | bPanRelative | 1 | Signed Number | 0: Stop, 1: clockwise, 0xFF: counter-clockwise | >> | 1 | bPanSpeed | 1 | Number | Speed of the Pan movement | >> | 2 | bTiltRelative | 1 | Signed Number | 0: Stop, 1: tilt up, 0xFF: tilt down | >> | 3 | bTiltSpeed | 1 | Number | Speed for the Tilt movement | >> +--------+---------------+------+---------------+------------------------------------------------+ >> >> I think it is the direction of the original implementation which is way easier to use than having 2 controls anyway, I would say it's preferred, it's how I have all my analog stick controls mappings. >> >> While the OBSBOT firmware implementation may handle any signed negative value in the direction byte we should probably check and make sure it conforms to spec with 0xFF for counter clockwise and down. >> >> In the current implementation both pan and tilt each use 2 bytes: >> >> { >> .id = V4L2_CID_PAN_SPEED, >> .entity = UVC_GUID_UVC_CAMERA, >> .selector = UVC_CT_PANTILT_RELATIVE_CONTROL, >> .size = 16, >> .offset = 0, >> .v4l2_type = V4L2_CTRL_TYPE_INTEGER, >> .data_type = UVC_CTRL_DATA_TYPE_SIGNED, >> .get = uvc_ctrl_get_rel_speed, >> .set = uvc_ctrl_set_rel_speed, >> }, >> { >> .id = V4L2_CID_TILT_SPEED, >> .entity = UVC_GUID_UVC_CAMERA, >> .selector = UVC_CT_PANTILT_RELATIVE_CONTROL, >> .size = 16, >> .offset = 16, >> .v4l2_type = V4L2_CTRL_TYPE_INTEGER, >> .data_type = UVC_CTRL_DATA_TYPE_SIGNED, >> .get = uvc_ctrl_get_rel_speed, >> .set = uvc_ctrl_set_rel_speed, >> }, >> >> Going to do some testing and report back. >> >> Thanks, >> John >> >> >> >> On Mon, Mar 25, 2024 at 9:23 PM Gergo Koteles <soyer@irl.hu> wrote: >>> >>> Hi John, >>> >>> On Mon, 2024-03-25 at 20:51 -0500, John Bauer wrote: >>> >>> I understand this patch might not be the ideal or proper solution; but it works. I don't think the UVC >>> implementation can be trusted on these cameras, just like the Windows DirectShow implementation is off. >>> I put this patch out there as I have encountered many Linux users who are struggling to get proper >>> control of these awesome cameras. If the patch dies here for now, that's OK, at least there's a possible >>> patch for those in need. >>> >>> >>> Sorry, maybe I didn't phrase it well. Based on the UVC specs, I think your patch is good for all UVC PTZ cameras, so you don't need to use UVC_QUIRK_OBSBOT_MIN_SETTINGS quirk entry, just apply the quirk changes to all cameras. >>> >>> Thanks for doing this! >>> >>> Regards, >>> Gergo >>> -- Ricardo Ribalda
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index e59a463c2761..1951e06527cf 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1322,10 +1322,23 @@ static int __uvc_query_v4l2_ctrl(struct uvc_video_chain *chain, break; } - if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) + if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MIN) { v4l2_ctrl->minimum = mapping->get(mapping, UVC_GET_MIN, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); + if (chain->dev->quirks & UVC_QUIRK_OBSBOT_MIN_SETTINGS) { + switch (v4l2_ctrl->id) { + case V4L2_CID_ZOOM_CONTINUOUS: + case V4L2_CID_PAN_SPEED: + case V4L2_CID_TILT_SPEED: + v4l2_ctrl->minimum = -1 * mapping->get(mapping, UVC_GET_MAX, + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); + default: + break; + } + } + } + if (ctrl->info.flags & UVC_CTRL_FLAG_GET_MAX) v4l2_ctrl->maximum = mapping->get(mapping, UVC_GET_MAX, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); @@ -1914,6 +1927,18 @@ int uvc_ctrl_set(struct uvc_fh *handle, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MIN)); max = mapping->get(mapping, UVC_GET_MAX, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_MAX)); + + if (chain->dev->quirks & UVC_QUIRK_OBSBOT_MIN_SETTINGS) { + switch (xctrl->id) { + case V4L2_CID_ZOOM_CONTINUOUS: + case V4L2_CID_PAN_SPEED: + case V4L2_CID_TILT_SPEED: + min = max * -1; + default: + break; + } + } + step = mapping->get(mapping, UVC_GET_RES, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_RES)); if (step == 0) diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index bbd90123a4e7..d4edc1adb11b 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -3120,6 +3120,14 @@ static const struct usb_device_id uvc_ids[] = { .bInterfaceSubClass = 1, .bInterfaceProtocol = 0, .driver_info = UVC_INFO_META(V4L2_META_FMT_D4XX) }, + /* OBSBOT pan, tilt, zoom min settings quirk */ + { .match_flags = USB_DEVICE_ID_MATCH_VENDOR + | USB_DEVICE_ID_MATCH_INT_INFO, + .idVendor = 0x3564, + .bInterfaceClass = USB_CLASS_VIDEO, + .bInterfaceSubClass = 1, + .bInterfaceProtocol = 0, + .driver_info = UVC_INFO_QUIRK(UVC_QUIRK_OBSBOT_MIN_SETTINGS) }, /* Generic USB Video Class */ { USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_UNDEFINED) }, { USB_INTERFACE_INFO(USB_CLASS_VIDEO, 1, UVC_PC_PROTOCOL_15) }, diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 6fb0a78b1b00..0e2f083a5c0e 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -73,6 +73,7 @@ #define UVC_QUIRK_FORCE_Y8 0x00000800 #define UVC_QUIRK_FORCE_BPP 0x00001000 #define UVC_QUIRK_WAKE_AUTOSUSPEND 0x00002000 +#define UVC_QUIRK_OBSBOT_MIN_SETTINGS 0x00004000 /* Format flags */ #define UVC_FMT_FLAG_COMPRESSED 0x00000001