| Message ID | 20180822165937.8700-7-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Rockchip VPU JPEG encoder | expand |
Hi, On Wed, 2018-08-22 at 13:59 -0300, Ezequiel Garcia wrote: > From: Shunqian Zheng <zhengsq@rock-chips.com> > > Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace > configure the JPEG quantization tables. How about having a single control for quantization? In MPEG-2/H.264/H.265, we have a single control exposed as a structure, which contains the tables for both luma and chroma. In the case of JPEG, it's not that big a deal, but for advanced video formats, it would be too much hassle to have one control per table. In order to keep the interface consistent, I think it'd be best to merge both matrices into a single control. What do you think? Paul > Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > Documentation/media/uapi/v4l/extended-controls.rst | 13 +++++++++++++ > drivers/media/v4l2-core/v4l2-ctrls.c | 13 +++++++++++++ > include/uapi/linux/v4l2-controls.h | 3 +++ > 3 files changed, 29 insertions(+) > > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst > index 9f7312bf3365..1189750a022c 100644 > --- a/Documentation/media/uapi/v4l/extended-controls.rst > +++ b/Documentation/media/uapi/v4l/extended-controls.rst > @@ -3354,6 +3354,19 @@ JPEG Control IDs > Specify which JPEG markers are included in compressed stream. This > control is valid only for encoders. > > +.. _jpeg-quant-tables-control: > + > +``V4L2_CID_JPEG_LUMA_QUANTIZATION (__u8 matrix)`` > + Sets the luma quantization table to be used for encoding > + or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. This table is > + a 8x8-byte matrix, and it's expected to be in JPEG zigzag order, > + as per the JPEG specification. > + > +``V4L2_CID_JPEG_CHROMA_QUANTIZATION (__u8 matrix)`` > + Sets the chroma quantization table to be used for encoding > + or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. This table is > + a 8x8-byte matrix, and it's expected to be in JPEG zigzag order, > + as per the JPEG specification. > > > .. flat-table:: > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c > index 6ab15f4a0fea..677af8069bfc 100644 > --- a/drivers/media/v4l2-core/v4l2-ctrls.c > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c > @@ -999,6 +999,8 @@ const char *v4l2_ctrl_get_name(u32 id) > case V4L2_CID_JPEG_RESTART_INTERVAL: return "Restart Interval"; > case V4L2_CID_JPEG_COMPRESSION_QUALITY: return "Compression Quality"; > case V4L2_CID_JPEG_ACTIVE_MARKER: return "Active Markers"; > + case V4L2_CID_JPEG_LUMA_QUANTIZATION: return "Luminance Quantization Matrix"; > + case V4L2_CID_JPEG_CHROMA_QUANTIZATION: return "Chrominance Quantization Matrix"; > > /* Image source controls */ > /* Keep the order of the 'case's the same as in v4l2-controls.h! */ > @@ -1286,6 +1288,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, > case V4L2_CID_DETECT_MD_REGION_GRID: > *type = V4L2_CTRL_TYPE_U8; > break; > + case V4L2_CID_JPEG_LUMA_QUANTIZATION: > + case V4L2_CID_JPEG_CHROMA_QUANTIZATION: > + *min = *def = 0; > + *max = 0xff; > + *step = 1; > + *type = V4L2_CTRL_TYPE_U8; > + if (dims) { > + memset(dims, 0, V4L2_CTRL_MAX_DIMS * sizeof(dims[0])); > + dims[0] = dims[1] = 8; > + } > + break; > case V4L2_CID_DETECT_MD_THRESHOLD_GRID: > *type = V4L2_CTRL_TYPE_U16; > break; > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h > index e4ee10ee917d..a466acf40625 100644 > --- a/include/uapi/linux/v4l2-controls.h > +++ b/include/uapi/linux/v4l2-controls.h > @@ -987,6 +987,9 @@ enum v4l2_jpeg_chroma_subsampling { > #define V4L2_JPEG_ACTIVE_MARKER_DQT (1 << 17) > #define V4L2_JPEG_ACTIVE_MARKER_DHT (1 << 18) > > +#define V4L2_CID_JPEG_LUMA_QUANTIZATION (V4L2_CID_JPEG_CLASS_BASE + 5) > +#define V4L2_CID_JPEG_CHROMA_QUANTIZATION (V4L2_CID_JPEG_CLASS_BASE + 6) > + > > /* Image source controls */ > #define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)
On Mon, 2018-08-27 at 09:47 +0200, Paul Kocialkowski wrote: > Hi, > > On Wed, 2018-08-22 at 13:59 -0300, Ezequiel Garcia wrote: > > From: Shunqian Zheng <zhengsq@rock-chips.com> > > > > Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace > > configure the JPEG quantization tables. > > How about having a single control for quantization? > > In MPEG-2/H.264/H.265, we have a single control exposed as a structure, > which contains the tables for both luma and chroma. In the case of JPEG, > it's not that big a deal, but for advanced video formats, it would be > too much hassle to have one control per table. > > In order to keep the interface consistent, I think it'd be best to merge > both matrices into a single control. > > What do you think? > I think it makes a lot of sense. I don't see the benefit in having luma and chroma separated, and consistency is good. I guess the more consistent solution would be to expose a compound control, similar to the video quantization one. struct v4l2_ctrl_jpeg_quantization { __u8 luma_quantization_matrix[64]; __u8 chroma_quantization_matrix[64]; }; Thanks! Eze
On Wed, Aug 29, 2018 at 11:50 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > On Mon, 2018-08-27 at 09:47 +0200, Paul Kocialkowski wrote: > > Hi, > > > > On Wed, 2018-08-22 at 13:59 -0300, Ezequiel Garcia wrote: > > > From: Shunqian Zheng <zhengsq@rock-chips.com> > > > > > > Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace > > > configure the JPEG quantization tables. > > > > How about having a single control for quantization? > > > > In MPEG-2/H.264/H.265, we have a single control exposed as a structure, > > which contains the tables for both luma and chroma. In the case of JPEG, > > it's not that big a deal, but for advanced video formats, it would be > > too much hassle to have one control per table. > > > > In order to keep the interface consistent, I think it'd be best to merge > > both matrices into a single control. > > > > What do you think? > > > > I think it makes a lot of sense. I don't see the benefit in having luma > and chroma separated, and consistency is good. > > I guess the more consistent solution would be to expose a compound > control, similar to the video quantization one. > > struct v4l2_ctrl_jpeg_quantization { > __u8 luma_quantization_matrix[64]; > __u8 chroma_quantization_matrix[64]; > }; Makes sense indeed. It also lets us avoid the hassle of setting .min/.max/.dims and other array control stuff, since everything is already defined by the C struct itself. Best regards, Tomasz
diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst index 9f7312bf3365..1189750a022c 100644 --- a/Documentation/media/uapi/v4l/extended-controls.rst +++ b/Documentation/media/uapi/v4l/extended-controls.rst @@ -3354,6 +3354,19 @@ JPEG Control IDs Specify which JPEG markers are included in compressed stream. This control is valid only for encoders. +.. _jpeg-quant-tables-control: + +``V4L2_CID_JPEG_LUMA_QUANTIZATION (__u8 matrix)`` + Sets the luma quantization table to be used for encoding + or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. This table is + a 8x8-byte matrix, and it's expected to be in JPEG zigzag order, + as per the JPEG specification. + +``V4L2_CID_JPEG_CHROMA_QUANTIZATION (__u8 matrix)`` + Sets the chroma quantization table to be used for encoding + or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. This table is + a 8x8-byte matrix, and it's expected to be in JPEG zigzag order, + as per the JPEG specification. .. flat-table:: diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 6ab15f4a0fea..677af8069bfc 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -999,6 +999,8 @@ const char *v4l2_ctrl_get_name(u32 id) case V4L2_CID_JPEG_RESTART_INTERVAL: return "Restart Interval"; case V4L2_CID_JPEG_COMPRESSION_QUALITY: return "Compression Quality"; case V4L2_CID_JPEG_ACTIVE_MARKER: return "Active Markers"; + case V4L2_CID_JPEG_LUMA_QUANTIZATION: return "Luminance Quantization Matrix"; + case V4L2_CID_JPEG_CHROMA_QUANTIZATION: return "Chrominance Quantization Matrix"; /* Image source controls */ /* Keep the order of the 'case's the same as in v4l2-controls.h! */ @@ -1286,6 +1288,17 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, case V4L2_CID_DETECT_MD_REGION_GRID: *type = V4L2_CTRL_TYPE_U8; break; + case V4L2_CID_JPEG_LUMA_QUANTIZATION: + case V4L2_CID_JPEG_CHROMA_QUANTIZATION: + *min = *def = 0; + *max = 0xff; + *step = 1; + *type = V4L2_CTRL_TYPE_U8; + if (dims) { + memset(dims, 0, V4L2_CTRL_MAX_DIMS * sizeof(dims[0])); + dims[0] = dims[1] = 8; + } + break; case V4L2_CID_DETECT_MD_THRESHOLD_GRID: *type = V4L2_CTRL_TYPE_U16; break; diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h index e4ee10ee917d..a466acf40625 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -987,6 +987,9 @@ enum v4l2_jpeg_chroma_subsampling { #define V4L2_JPEG_ACTIVE_MARKER_DQT (1 << 17) #define V4L2_JPEG_ACTIVE_MARKER_DHT (1 << 18) +#define V4L2_CID_JPEG_LUMA_QUANTIZATION (V4L2_CID_JPEG_CLASS_BASE + 5) +#define V4L2_CID_JPEG_CHROMA_QUANTIZATION (V4L2_CID_JPEG_CLASS_BASE + 6) + /* Image source controls */ #define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)