Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bunch of V4L2 features #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bunch of V4L2 features #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8b3d96c
Subdev: Add very basic ext ctrls support
Nov 13, 2024
60e82d5
uapi: v4l2_ext_control struct must be specifically packed
Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions v4l2/subdev.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import fcntl
import os
from enum import IntFlag
from copy import copy

import v4l2.uapi

Expand Down Expand Up @@ -66,6 +67,8 @@ def __init__(self, dev_path: str) -> None:
except OSError:
self.has_streams = False

self.controls = self.enum_ext_ctrls()

def get_formats(self, pad, stream=0, which=v4l2.uapi.V4L2_SUBDEV_FORMAT_ACTIVE):
val = v4l2.uapi.v4l2_subdev_mbus_code_enum()
val.pad = pad
Expand Down Expand Up @@ -271,3 +274,53 @@ def set_control(self, ctrl_id: int, ctrl_val: int):
v4l2_ctrl.value = ctrl_val

fcntl.ioctl(self.fd, v4l2.uapi.VIDIOC_S_CTRL, v4l2_ctrl, False)

def enum_ext_ctrls(self):
ctrl = v4l2.uapi.v4l2_queryctrl()
ctrl.id = v4l2.uapi.V4L2_CTRL_FLAG_NEXT_CTRL
ext_ctrls = {}
while True:
try:
fcntl.ioctl(self.fd, v4l2.uapi.VIDIOC_QUERYCTRL, ctrl)
except OSError:
return ext_ctrls

if ctrl.type != v4l2.uapi.V4L2_CTRL_TYPE_CTRL_CLASS:
ext_ctrls[ctrl.id] = copy(ctrl)

ctrl.id |= v4l2.uapi.V4L2_CTRL_FLAG_NEXT_CTRL

def get_ext_ctrl(self, ctrl_id: int):
ctrl = v4l2.uapi.v4l2_ext_control()
ctrl.id = ctrl_id
ext_ctrl = v4l2.uapi.v4l2_ext_controls()
ext_ctrl.which = v4l2.uapi.V4L2_CTRL_WHICH_CUR_VAL
ext_ctrl.count = 1
ext_ctrl.controls = ctypes.pointer(ctrl)
fcntl.ioctl(self.fd, v4l2.uapi.VIDIOC_G_EXT_CTRLS, ext_ctrl)
return (
ctrl.value64
if (self.controls[ctrl_id].type == v4l2.uapi.V4L2_CTRL_TYPE_INTEGER64)
else ctrl.value
)
Copy link
Owner

Choose a reason for hiding this comment

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

I've never seen this style used before. Maybe it's just me, but that's very unclear to me. How about just:

if self.controls[ctrl_id].type == v4l2.uapi.V4L2_CTRL_TYPE_INTEGER64:
    v = ctrl.value64
else:
    v = ctrl.value

return v

Copy link
Author

Choose a reason for hiding this comment

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

No, its not you :-), just badly written... Will fix it.
It also raise questions about the type of controls to support (In a first step, I only needed INTEGER64).


def set_ext_ctrl(self, ctrl_id, ctrl_val):
# TODO: handle situation when control is dynamically grabbed
if self.controls[ctrl_id].flags & v4l2.uapi.V4L2_CTRL_FLAG_READ_ONLY:
print(f"Control id [{ctrl_id}] isn't writable")
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should raise an exception.

Copy link
Author

Choose a reason for hiding this comment

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

Yes sure... I realized now the poor quality of this PR...
Let's take it as an opportunity to exchange a bit on some implementation choices:

Regarding ext ctrls, I made the choice to enumerate controls at SubDevice initialization :
self.controls = self.enum_ext_ctrls()
This allow to do additionnal tests (READ_ONLY, GRABBED) in set_ext_ctrl(). However I'm not sure it's in the same philosophy than the rest of the code.
Furthermore it's not fully implemented and doesn't support dynamically grabbed controls (which would requires to enum_ext_ctrls().

Would you mind giving me your opinion and I could propose an updated implementation.

return
if self.controls[ctrl_id].flags & v4l2.uapi.V4L2_CTRL_FLAG_GRABBED:
print(f'Control id [{ctrl_id}] temporarily grabbed')
Copy link
Owner

Choose a reason for hiding this comment

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

And this too.

Copy link
Author

Choose a reason for hiding this comment

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

Same than above :-)

return

ctrl = v4l2.uapi.v4l2_ext_control()
ctrl.id = ctrl_id
if self.controls[ctrl_id].type == v4l2.uapi.V4L2_CTRL_TYPE_INTEGER64:
ctrl.value64 = ctrl_val
else:
ctrl.value = ctrl_val
ext_ctrl = v4l2.uapi.v4l2_ext_controls()
ext_ctrl.which = v4l2.uapi.V4L2_CTRL_WHICH_CUR_VAL
ext_ctrl.count = 1
ext_ctrl.controls = ctypes.pointer(ctrl)
fcntl.ioctl(self.fd, v4l2.uapi.VIDIOC_S_EXT_CTRLS, ext_ctrl)
Copy link
Owner

Choose a reason for hiding this comment

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

It would help me to understand what you need if you would describe in the commit desc what you use this for (similarly for exporting CaptureStreamer). Looks like you just use it to set/get a single 32/64 bit value?

I'm not super familiar with v4l2 controls as I haven't used them too much, but I think I'd really like to be able to expose a unified API for the users. No "ext" vs "non-ext" stuff, but just a way to get and set controls, regardless if the driver supports ext or not.

So, we already have the "naive" set_control(), I'd like to see it extended so it just works, instead of adding set_ext_ctrl(), requiring the caller to know which one to use.

Copy link
Author

Choose a reason for hiding this comment

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

Well, As explained above, I have a WIP 'mediapipe' branch (https://github.com/sylpe/pyv4l2/tree/mediapipe).
On top of this I have some pytests which allows me to tests camera driver : mostly iterating over all the formats (width, heigth, pixelformat), and play with controls.

Please find below an example of how I use it :

@pytest.mark.parametrize('duration', [2, 10], ids=duration_name)
@pytest.mark.parametrize('tp', [VD56G3_TP_HGREY, VD56G3_TP_VGREY], ids=tp_name)
def test_stream_tp(media_conf, duration, tp, request):
    """
    Test that the sensor stream correctly with Grey gradient Test Patterns.

    Ensure the output framerate is correct.
    Ensure the obtained image fit with format width and height.
    Ensure the orientation of the test pattern comply with the one configured.
    Save last frame as an image (for human inspection).
    """
    # Init : reset frame holder
    frames_holder.clear()
    # Init : setup media pipe
    media_pipe = MediaPipe(media_conf)
    media_pipe.setup_links()
    media_pipe.configure_subdevs()
    # Init : reset/set camera subdevice controls
    camera_subdev = media_pipe.subdevices[media_conf.get_src_entity().name]
    reset_camera_subdev_ctrls(camera_subdev)
    camera_subdev.set_ext_ctrl(v4l2.uapi.V4L2_CID_TEST_PATTERN, tp)
    # Init : gather test values
    (width, height, _) = get_subdev_source_fmt(media_conf)
    expected_fps = get_theorical_fps(camera_subdev)

    # Stream for 'duration' secs
    media_pipe.start_stream(on_frame_cb=collect_frame)
    sleep(duration)
    media_pipe.stop_stream()

    # Normalize collected frame in np arrays
    normalize_frames_holder()

    # Analyze output : framerate
    assert duration * expected_fps - 2 <= len(frames_holder) <= duration * expected_fps + 2
    # Analyze output : image format (width, height)
    assert frames_holder[0][2] == width
    assert frames_holder[0][3] == height
    # Analyze output : check images orientation (for first and last frame captured)
    orientation, magnitude = avr_gradient_direction(frames_holder[0][0])
    assert orientation <= 0.01 if tp == VD56G3_TP_HGREY else 89.99 < orientation < 90.01
    orientation, magnitude = avr_gradient_direction(frames_holder[-1][0])
    assert orientation <= 0.01 if tp == VD56G3_TP_HGREY else 89.99 < orientation < 90.01
    # Save last image present in the frame_holder
    test_name = request.node.name[5:]
    save_to_file(test_name, -1)

This could help understand better the context.

I would have like to use the existing set_control(), but for some of the controls (especially integer64) and flags, It was possible to go through the "legacy" control API.

I'm totally aligned with you, better to improve the existing API. Depending of your thoughts I could proposed a better implementation fitting with pyv4l2 philosophy.

1 change: 1 addition & 0 deletions v4l2/uapi/v4l2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3613,6 +3613,7 @@ class struct_v4l2_ext_control(Structure):
'reserved2',
'unnamed_1',
]
struct_v4l2_ext_control._pack_ = 1
Copy link
Owner

Choose a reason for hiding this comment

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

This file is generated, you can't modify it as the modification will get overwritten. If this is really needed, then something needs to be done either in the generator script or init.py. Or maybe there's a bug in the ctypesgen, which needs to be fixed.

It's also not clear to me why this is needed or how it helps... struct_v4l2_ext_control has u32 fields, and the last one is an union. How does it get packed without this change? Which field gets extra alignment without this?

Copy link
Author

Choose a reason for hiding this comment

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

Honestly I didn't really understand why this was needed...
To be honest, I used previously on my side another python library (pyv4l2 alternative) which were working correctly with ext ctrls ... I had to replicate this packing option to make pyv4l2 work correctly...
I would have been happy if you had a start of explanation :-D

Copy link
Owner

Choose a reason for hiding this comment

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

The union_anon_15 is 8 bytes, and ctypes will align that to 8 bytes, which means 4 bytes of padding before the union. This increases the size of the struct from 20 to 24 bytes. The c-header is marked with attribute ((packed)), so this looks like a bug in ctypesgen...

Copy link
Owner

Choose a reason for hiding this comment

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

ctypesgen/ctypesgen#218

struct_v4l2_ext_control._anonymous_ = [
'unnamed_1',
]
Expand Down
Loading