| Message ID | 20200616131244.70308-8-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, On Tue, Jun 16, 2020 at 10:12:36PM +0900, Paul Elder wrote: > Fix v4l2-compliance format tests, for enum_fmt, try_fmt, and g/s_fmt. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index a5fa478..fd2785f 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -179,6 +179,9 @@ void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig) > curV4L2Format_.fmt.pix.width, > curV4L2Format_.fmt.pix.height); > curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > + curV4L2Format_.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > + curV4L2Format_.fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; > + curV4L2Format_.fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; Any specific reason why you chose these encoding and quantization ? Why not simply use the _DEFAULT ones ? > } > > unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig) > @@ -283,11 +286,15 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > arg->index >= streamConfig_.formats().pixelformats().size()) > return -EINVAL; > > + /* \todo Set V4L2_FMT_FLAG_COMPRESSED for compressed formats. */ > + arg->flags = 0; > /* \todo Add map from format to description. */ > - utils::strlcpy(reinterpret_cast<char *>(arg->description), "Video Format Description", > - sizeof(arg->description)); > + utils::strlcpy(reinterpret_cast<char *>(arg->description), > + "Video Format Description", sizeof(arg->description)); > arg->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]); > > + memset(arg->reserved, 0, sizeof(arg->reserved)); > + If I were to be picky, I would have split this to a separate patch. At least please expand the commit message to mention what you're fixing Thanks j > return 0; > } > > @@ -330,6 +337,9 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg) > arg->fmt.pix.width, > arg->fmt.pix.height); > arg->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > + arg->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > + arg->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; > + arg->fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; > } > > int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg) > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Wed, Jun 17, 2020 at 04:59:31PM +0200, Jacopo Mondi wrote: > On Tue, Jun 16, 2020 at 10:12:36PM +0900, Paul Elder wrote: > > Fix v4l2-compliance format tests, for enum_fmt, try_fmt, and g/s_fmt. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > index a5fa478..fd2785f 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -179,6 +179,9 @@ void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig) > > curV4L2Format_.fmt.pix.width, > > curV4L2Format_.fmt.pix.height); > > curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > > + curV4L2Format_.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; Ah, here's the V4L2_PIX_FMT_PRIV_MAGIC I requested in a previous patch :-) > > + curV4L2Format_.fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; > > + curV4L2Format_.fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; > > Any specific reason why you chose these encoding and quantization ? > Why not simply use the _DEFAULT ones ? The _DEFAULT values are meant for userspace to ask the kernel to pick a default. The kernel should return real values. > > } > > > > unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig) > > @@ -283,11 +286,15 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > > arg->index >= streamConfig_.formats().pixelformats().size()) > > return -EINVAL; > > > > + /* \todo Set V4L2_FMT_FLAG_COMPRESSED for compressed formats. */ > > + arg->flags = 0; > > /* \todo Add map from format to description. */ > > - utils::strlcpy(reinterpret_cast<char *>(arg->description), "Video Format Description", > > - sizeof(arg->description)); > > + utils::strlcpy(reinterpret_cast<char *>(arg->description), > > + "Video Format Description", sizeof(arg->description)); > > arg->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]); > > > > + memset(arg->reserved, 0, sizeof(arg->reserved)); > > + > > If I were to be picky, I would have split this to a separate patch. > At least please expand the commit message to mention what you're > fixing I'd indeed move the memset to the patch that adds support for V4L2_CAP_EXT_PIX_FORMAT. It should be done for all the format-related ioctls (of not already there). > > return 0; > > } > > > > @@ -330,6 +337,9 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg) > > arg->fmt.pix.width, > > arg->fmt.pix.height); > > arg->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > > + arg->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > > + arg->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; > > + arg->fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; > > } > > > > int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg)
Hi Jacopo, On Wed, Jun 17, 2020 at 06:01:21PM +0200, Jacopo Mondi wrote: > On Wed, Jun 17, 2020 at 06:49:11PM +0300, Laurent Pinchart wrote: > > On Wed, Jun 17, 2020 at 04:59:31PM +0200, Jacopo Mondi wrote: > >> On Tue, Jun 16, 2020 at 10:12:36PM +0900, Paul Elder wrote: > >>> Fix v4l2-compliance format tests, for enum_fmt, try_fmt, and g/s_fmt. > >>> > >>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >>> --- > >>> src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++++-- > >>> 1 file changed, 12 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > >>> index a5fa478..fd2785f 100644 > >>> --- a/src/v4l2/v4l2_camera_proxy.cpp > >>> +++ b/src/v4l2/v4l2_camera_proxy.cpp > >>> @@ -179,6 +179,9 @@ void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig) > >>> curV4L2Format_.fmt.pix.width, > >>> curV4L2Format_.fmt.pix.height); > >>> curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > >>> + curV4L2Format_.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > > > > Ah, here's the V4L2_PIX_FMT_PRIV_MAGIC I requested in a previous patch > > :-) > > > >>> + curV4L2Format_.fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; > >>> + curV4L2Format_.fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; > >> > >> Any specific reason why you chose these encoding and quantization ? > >> Why not simply use the _DEFAULT ones ? > > > > The _DEFAULT values are meant for userspace to ask the kernel to pick a > > default. The kernel should return real values. > > > > Are you sure ? > > $ git grep QUANTIZATION_DEFAULT drivers/media/i2c/ | wc -l > 16 > $ git grep V4L2_YCBCR_ENC_DEFAULT drivers/media/i2c/ | wc -l > 13 > > and they're all assignements. > > The V4L2 documentation reports: > > V4L2_QUANTIZATION_DEFAULT > Use the default quantization encoding as defined by the colorspace. > This is always full range for R’G’B’ (except for the BT.2020 > colorspace) and HSV. It is usually limited range for Y’CbCr. > > V4L2_YCBCR_ENC_DEFAULT > Use the default Y’CbCr encoding as defined by the colorspace. You may be right. V4L2_COLORSPACE_DEFAULT is not allowed to be reported by drivers, but for the other fields, it may be fine. Let's ask Hans. > >>> } > >>> > >>> unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig) > >>> @@ -283,11 +286,15 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > >>> arg->index >= streamConfig_.formats().pixelformats().size()) > >>> return -EINVAL; > >>> > >>> + /* \todo Set V4L2_FMT_FLAG_COMPRESSED for compressed formats. */ > >>> + arg->flags = 0; > >>> /* \todo Add map from format to description. */ > >>> - utils::strlcpy(reinterpret_cast<char *>(arg->description), "Video Format Description", > >>> - sizeof(arg->description)); > >>> + utils::strlcpy(reinterpret_cast<char *>(arg->description), > >>> + "Video Format Description", sizeof(arg->description)); > >>> arg->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]); > >>> > >>> + memset(arg->reserved, 0, sizeof(arg->reserved)); > >>> + > >> > >> If I were to be picky, I would have split this to a separate patch. > >> At least please expand the commit message to mention what you're > >> fixing > > > > I'd indeed move the memset to the patch that adds support for > > V4L2_CAP_EXT_PIX_FORMAT. It should be done for all the format-related > > ioctls (of not already there). > > > >>> return 0; > >>> } > >>> > >>> @@ -330,6 +337,9 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg) > >>> arg->fmt.pix.width, > >>> arg->fmt.pix.height); > >>> arg->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > >>> + arg->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > >>> + arg->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; > >>> + arg->fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; > >>> } > >>> > >>> int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg)
Hi Laurent, On Wed, Jun 17, 2020 at 06:49:11PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Wed, Jun 17, 2020 at 04:59:31PM +0200, Jacopo Mondi wrote: > > On Tue, Jun 16, 2020 at 10:12:36PM +0900, Paul Elder wrote: > > > Fix v4l2-compliance format tests, for enum_fmt, try_fmt, and g/s_fmt. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++++-- > > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > > index a5fa478..fd2785f 100644 > > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > > @@ -179,6 +179,9 @@ void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig) > > > curV4L2Format_.fmt.pix.width, > > > curV4L2Format_.fmt.pix.height); > > > curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > > > + curV4L2Format_.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > > Ah, here's the V4L2_PIX_FMT_PRIV_MAGIC I requested in a previous patch > :-) > > > > + curV4L2Format_.fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; > > > + curV4L2Format_.fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; > > > > Any specific reason why you chose these encoding and quantization ? > > Why not simply use the _DEFAULT ones ? > > The _DEFAULT values are meant for userspace to ask the kernel to pick a > default. The kernel should return real values. > Are you sure ? $ git grep QUANTIZATION_DEFAULT drivers/media/i2c/ | wc -l 16 $ git grep V4L2_YCBCR_ENC_DEFAULT drivers/media/i2c/ | wc -l 13 and they're all assignements. The V4L2 documentation reports: V4L2_QUANTIZATION_DEFAULT Use the default quantization encoding as defined by the colorspace. This is always full range for R’G’B’ (except for the BT.2020 colorspace) and HSV. It is usually limited range for Y’CbCr. V4L2_YCBCR_ENC_DEFAULT Use the default Y’CbCr encoding as defined by the colorspace. > > > } > > > > > > unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig) > > > @@ -283,11 +286,15 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) > > > arg->index >= streamConfig_.formats().pixelformats().size()) > > > return -EINVAL; > > > > > > + /* \todo Set V4L2_FMT_FLAG_COMPRESSED for compressed formats. */ > > > + arg->flags = 0; > > > /* \todo Add map from format to description. */ > > > - utils::strlcpy(reinterpret_cast<char *>(arg->description), "Video Format Description", > > > - sizeof(arg->description)); > > > + utils::strlcpy(reinterpret_cast<char *>(arg->description), > > > + "Video Format Description", sizeof(arg->description)); > > > arg->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]); > > > > > > + memset(arg->reserved, 0, sizeof(arg->reserved)); > > > + > > > > If I were to be picky, I would have split this to a separate patch. > > At least please expand the commit message to mention what you're > > fixing > > I'd indeed move the memset to the patch that adds support for > V4L2_CAP_EXT_PIX_FORMAT. It should be done for all the format-related > ioctls (of not already there). > > > > return 0; > > > } > > > > > > @@ -330,6 +337,9 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg) > > > arg->fmt.pix.width, > > > arg->fmt.pix.height); > > > arg->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; > > > + arg->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; > > > + arg->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; > > > + arg->fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; > > > } > > > > > > int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg) > > -- > Regards, > > Laurent Pinchart
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index a5fa478..fd2785f 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -179,6 +179,9 @@ void V4L2CameraProxy::setFmtFromConfig(StreamConfiguration &streamConfig) curV4L2Format_.fmt.pix.width, curV4L2Format_.fmt.pix.height); curV4L2Format_.fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; + curV4L2Format_.fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; + curV4L2Format_.fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; + curV4L2Format_.fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; } unsigned int V4L2CameraProxy::calculateSizeImage(StreamConfiguration &streamConfig) @@ -283,11 +286,15 @@ int V4L2CameraProxy::vidioc_enum_fmt(int fd, struct v4l2_fmtdesc *arg) arg->index >= streamConfig_.formats().pixelformats().size()) return -EINVAL; + /* \todo Set V4L2_FMT_FLAG_COMPRESSED for compressed formats. */ + arg->flags = 0; /* \todo Add map from format to description. */ - utils::strlcpy(reinterpret_cast<char *>(arg->description), "Video Format Description", - sizeof(arg->description)); + utils::strlcpy(reinterpret_cast<char *>(arg->description), + "Video Format Description", sizeof(arg->description)); arg->pixelformat = drmToV4L2(streamConfig_.formats().pixelformats()[arg->index]); + memset(arg->reserved, 0, sizeof(arg->reserved)); + return 0; } @@ -330,6 +337,9 @@ void V4L2CameraProxy::tryFormat(struct v4l2_format *arg) arg->fmt.pix.width, arg->fmt.pix.height); arg->fmt.pix.colorspace = V4L2_COLORSPACE_SRGB; + arg->fmt.pix.priv = V4L2_PIX_FMT_PRIV_MAGIC; + arg->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_601; + arg->fmt.pix.quantization = V4L2_QUANTIZATION_LIM_RANGE; } int V4L2CameraProxy::vidioc_s_fmt(int fd, struct v4l2_format *arg)
Fix v4l2-compliance format tests, for enum_fmt, try_fmt, and g/s_fmt. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/v4l2/v4l2_camera_proxy.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)