Bunch of V4L2 features

[sylpe]

• First commits are basics v4l2 features.
• Ext Ctrl Support is quite basic
• Stride issue has been observed on exotic resolutions but may be fixed with new handling of stride in pixutils

[tomba]

Why do you need this? The commit description should explain why the patch is needed, unless it's very obvious.

[sylpe]

Yes, sorry, it lacks the background ... In fact I have a WIP 'mediapipe' branch (https://github.com/sylpe/pyv4l2/tree/mediapipe) where I tried to abstract a bit the media pipeline (and for this I use CaptureStreamer class).
So you're right this commit has nothing to do here; I'll move it.

[tomba]

I think this should raise an exception.

[sylpe]

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.

[tomba]

And this too.

[sylpe]

Same than above :-)

[tomba]

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

[sylpe]

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).

[tomba]

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.

[sylpe]

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.

[tomba]

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?

[sylpe]

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

[tomba]

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...

[tomba]

ctypesgen/ctypesgen#218

[tomba]

I've pushed the first two patches. I'll reply to the rest later.