Re: [PATCH v2 5/6] media: Add controls for jpeg quantization tables
From: Tomasz Figa <hidden>
Date: 2018-08-17 02:10:00
Also in:
linux-media, linux-rockchip
Hi Ezequiel, On Fri, Aug 3, 2018 at 5:00 AM Ezequiel Garcia [off-list ref] wrote:
From: Shunqian Zheng <redacted> Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace configure the JPEG quantization tables. Signed-off-by: Shunqian Zheng <redacted> Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> --- Documentation/media/uapi/v4l/extended-controls.rst | 9 +++++++++ drivers/media/v4l2-core/v4l2-ctrls.c | 4 ++++ include/uapi/linux/v4l2-controls.h | 3 +++ 3 files changed, 16 insertions(+)
Thanks for this series and sorry for being late with review. Please see my comments inline.
quoted hunk
diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst index 9f7312bf3365..80e26f81900b 100644 --- a/Documentation/media/uapi/v4l/extended-controls.rst +++ b/Documentation/media/uapi/v4l/extended-controls.rst@@ -3354,6 +3354,15 @@ 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 + expected to be in JPEG zigzag order, as per the JPEG specification.
Should we also specify this to be 8x8?
+ +``V4L2_CID_JPEG_CHROMA_QUANTIZATION (__u8 matrix)`` + Sets the chroma quantization table.
nit: I guess we aff something like "See also V4L2_CID_JPEG_LUMA_QUANTIZATION for details." to avoid repeating the V4L2_PIX_FMT_JPEG_RAW and zigzag order bits? Or maybe just repeating is better?
quoted hunk
.. flat-table::diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 599c1cbff3b9..5c62c3101851 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! */@@ -1284,6 +1286,8 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, *flags |= V4L2_CTRL_FLAG_READ_ONLY; break; case V4L2_CID_DETECT_MD_REGION_GRID: + case V4L2_CID_JPEG_LUMA_QUANTIZATION: + case V4L2_CID_JPEG_CHROMA_QUANTIZATION:
It looks like with this setup, the driver has to explicitly set dims
to { 8, 8 } and min/max to 0/255.
At least for min and max, we could set them here. For dims, i don't
see it handled in generic code, so I guess we can leave it to the
driver now and add move into generic code, if another driver shows up.
Hans, what do you think?
Best regards,
Tomasz