Hi Hans.
On 06/02/17 12:58, Hans Verkuil wrote:
On 02/06/2017 12:37 PM, Dave Stevenson wrote:
Hi Hans.
On 06/02/17 09:08, Hans Verkuil wrote:
Hi Eric,
Great to see this driver appearing for upstream merging!
See below for my review comments, focusing mostly on V4L2 specifics.
<snip>
+ f->fmt.pix.pixelformat = dev->capture.fmt->fourcc;
+ f->fmt.pix.bytesperline = dev->capture.stride;
+ f->fmt.pix.sizeimage = dev->capture.buffersize;
+
+ if (dev->capture.fmt->fourcc == V4L2_PIX_FMT_RGB24)
+ f->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB;
+ else if (dev->capture.fmt->fourcc == V4L2_PIX_FMT_JPEG)
+ f->fmt.pix.colorspace = V4L2_COLORSPACE_JPEG;
+ else
+ f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M;
Colorspace has nothing to do with the pixel format. It should come from the
sensor/video receiver.
If this information is not available, then COLORSPACE_SRGB is generally a
good fallback.
I would if I could, but then I fail v4l2-compliance on V4L2_PIX_FMT_JPEG
https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-formats.cpp#n329
The special case for JPEG therefore has to remain.
Correct. Sorry, my fault, I forgot about that.
It looks like I tripped over the subtlety between V4L2_COLORSPACE_,
V4L2_XFER_FUNC_, V4L2_YCBCR_ENC_, and V4L2_QUANTIZATION_, and Y'CbCr
encoding vs colourspace.
The ISP coefficients are set up for BT601 limited range, and any
conversion back to RGB is done based on that. That seemed to fit
SMPTE170M rather than SRGB.
Colorspace refers to the primary colors + whitepoint that are used to
create the colors (basically this answers the question to which colors
R, G and B exactly refer to). The SMPTE170M has different primaries
compared to sRGB (and a different default transfer function as well).
RGB vs Y'CbCr is just an encoding and it doesn't change the underlying
colorspace. Unfortunately, the term 'colorspace' is often abused to just
refer to RGB vs Y'CbCr.
If the colorspace is SRGB, then when the pixelformat is a Y'CbCr encoding,
then the BT601 limited range encoding is implied, unless overridden via
the ycbcr_enc and/or quantization fields in struct v4l2_pix_format.
In other words, this does already the right thing.
https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/pixfmt-007.html#colorspace-srgb-v4l2-colorspace-srgb
"The default transfer function is V4L2_XFER_FUNC_SRGB. The default
Y’CbCr encoding is V4L2_YCBCR_ENC_601. The default Y’CbCr quantization
is full range."
So full range or limited?
The JPEG colorspace is a short-hand for V4L2_COLORSPACE_SRGB, V4L2_YCBCR_ENC_601
and V4L2_QUANTIZATION_FULL_RANGE. It's historical that this colorspace exists.
If I would redesign this this JPEG colorspace would be dropped.
For a lot more colorspace information see:
https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html
I do note that as there is now support for more RGB formats (BGR24 and
BGR32) the first "if" needs extending to cover those. Or I don't care
and only special case JPEG with all others just reporting SRGB.
Only special case JPEG.
But as I said, this information really needs to come from the sensor or
video receiver since this driver has no knowledge of this.
OK.
<snip>
This is IMHO unnecessarily complex.
My recommendation is that controls are added with a set of v4l2_ctrl_new_std* calls
or if you really want to by walking a struct v4l2_ctrl_config array and adding controls
via v4l2_ctrl_new_custom.
The s_ctrl is a switch that calls the 'setter' function.
No need for arrays, callbacks, etc. Just keep it simple.
I can look into that, but I'm not sure I fully follow what you are
suggesting.
In the current implementation things like V4L2_CID_SATURATION,
V4L2_CID_SHARPNESS, V4L2_CID_CONTRAST, and V4L2_CID_BRIGHTNESS all use
the one common ctrl_set_rational setter function because the only thing
different in setting is the MMAL_PARAMETER_xxx value. I guess that could
move into the common setter based on V4L2_CID_xxx, but then the control
configuration is split between multiple places which feels less well
contained.
See e.g. samples/v4l/v4l2-pci-skeleton.c: in the probe function (or in a
function called from there if there are a lot of controls) you add the
controls, and in s_ctrl you handle them.
But this is just my preference.
So in s_ctrl you would see something like this:
switch (ctrl->id) {
case V4L2_CID_SATURATION:
ctrl_set_rational(ctrl->val, MMAL_PARAMETER_SAT);
break;
case V4L2_CID_BRIGHTNESS:
ctrl_set_rational(ctrl->val, MMAL_PARAMETER_BRIGHTNESS);
break;
...
}
OK, thanks for the clarification. That can be done.
<snip>
Final question: did you run v4l2-compliance over this driver? Before this driver can
be moved out of staging it should pass the compliance tests. Note: always compile
this test from the main repository, don't rely on distros. That ensures you use the
latest code.
The compliance test is part of the v4l-utils repo (https://git.linuxtv.org/v4l-utils.git/).
If you have any questions about the v4l2-compliance output (it can be a bit obscure at
times), just mail me or ask the question on the #v4l irc channel.
I haven't checked this version, but the downstream version has 43
passes, 0 failures, 0 warnings.
The full output:
v4l2-compliance SHA : 99306f20cc7e76cf2161e3059de4da245aed2130
That's pretty recent. Good to know you've used this.
Also test with v4l2-compliance -f: this tests all available formats.
OK.
Driver Info:
Driver name : bm2835 mmal
Card type : mmal service 16.1
Bus info : platform:bcm2835-v4l2
Driver version: 4.4.45
Capabilities : 0x85200005
Video Capture
Video Overlay
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps : 0x05200005
Video Capture
Video Overlay
Read/Write
Streaming
Extended Pix Format
Compliance test for device /dev/video0 (not using libv4l2):
Required ioctls:
test VIDIOC_QUERYCAP: OK
Allow for multiple opens:
test second video open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK
Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK
Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0
Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0
Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)
Test input 0:
Control ioctls:
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 33 Private Controls: 0
Format ioctls:
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
test Cropping: OK (Not Supported)
test Composing: OK (Not Supported)
test Scaling: OK
Is scaling supported? Just checking.
The cropping/composing/scaling API is not currently supported.
The hardware can do it, but I need to work out how it should be set up,
and what resolutions to quote via V4L2_SEL_TGT_CROP_BOUNDS and similar.
It just needs a bit of time.
Codec ioctls:
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
Buffer ioctls:
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK (Not Supported)
Test input 0:
Total: 43, Succeeded: 43, Failed: 0, Warnings: 0
Note that v4l2-compliance does very limited testing of overlay handling.
You should test this manually to make sure it functions properly.
OK, thanks for the heads up. It's not one that gets used that often in
the wild either from what I can tell.
Regards,
Hans
Thanks,
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
