Re: [PATCH v5 5/6] media: Add controls for JPEG quantization tables

On Sun, Sep 16, 2018 at 1:48 AM Paul Kocialkowski [off-list ref] wrote:

Hi,

On Mon, 2018-09-10 at 10:25 -0300, Ezequiel Garcia wrote:

quoted

Hi Hans,

Thanks for the review.

On Mon, 2018-09-10 at 14:42 +0200, Hans Verkuil wrote:

quoted

On 09/06/2018 12:00 AM, Ezequiel Garcia wrote:

quoted

From: Shunqian Zheng <redacted>

Add V4L2_CID_JPEG_QUANTIZATION compound control 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>
---
 .../media/uapi/v4l/extended-controls.rst      | 31 +++++++++++++++++++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
 include/uapi/linux/v4l2-controls.h            | 12 +++++++
 include/uapi/linux/videodev2.h                |  1 +
 5 files changed, 55 insertions(+)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
index 9f7312bf3365..1335d27d30f3 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst

@@ -3354,7 +3354,38 @@ 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_QUANTIZATION (struct)``
+    Specifies the luma and chroma quantization matrices for encoding
+    or decoding a V4L2_PIX_FMT_JPEG_RAW format buffer. The :ref:`itu-t81`
+    specification allows 8-bit quantization coefficients for
+    baseline profile images, and 8-bit or 16-bit for extended profile
+    images. Supporting or not 16-bit precision coefficients is driver-specific.
+    Coefficients must be set in JPEG zigzag scan order.
+
+
+.. c:type:: struct v4l2_ctrl_jpeg_quantization
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_ctrl_jpeg_quantization
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - __u8
+      - ``precision``
+      - Specifies the coefficient precision. User shall set 0
+        for 8-bit, and 1 for 16-bit.

So does specifying 1 here switch the HW encoder to use extended profile?
What if the HW only supports baseline? The rockchip driver doesn't appear
to check the precision field at all...

The driver is missing to check that, when the user sets this control.

quoted

I think this needs a bit more thought.

I am not at all sure that this is the right place for the precision field.
This is really about JPEG profiles, so I would kind of expect a JPEG PROFILE
control (just like other codec profiles), or possibly a new pixelformat for
extended profiles.

And based on that the driver would interpret these matrix values as 8 or
16 bits.

Right, the JPEG profile control is definitely needed. I haven't add it because
it wouldn't be used, since this VPU can only do baseline.

Well, I suppose it would still be relevant that you add it for the
encoder and only report baseline there.

quoted

However, the problem is that some JPEGs in the wild have with 8-bit data and
16-bit quantization coefficients, as per [1] and [2]:

[1] https://github.com/martinhath/jpeg-rust/issues/1
[2] https://github.com/libjpeg-turbo/libjpeg-turbo/pull/90

So, in order to support decoding of these images, I've added the precision
field to the quantization control. The user would be able to set a baseline
or extended profile thru a (future) profile control, and if 16-bit
tables are found, and if the hardware supports them, the driver
would be able to support them.

Another option, which might be even better, is have explicit baseline
and extended quantization tables controls, e.g.: V4L2_CID_JPEG_QUANT
and V4L2_CID_JPEG_EXT_QUANT.

I think this makes more sense than a common structure with an indication
bit on how to interpret the data.

However, it seems problematic that userspace can't figure out whether
16-bit quant tables are supported with a baseline profile and just has
to try and see.

Hans, do you think this is an acceptable approach or should we rather
stick to the standard here, at the cost of not supporting these pictures
that were encoded with this common abuse of the standard?

Perhaps we just need a control called V4L2_CID_JPEG_QUANT_PRECISION,
where drivers can set the min/max (e.g. min = 8, max = 16, step = 8)
to the range they support and user space can select the precision it
wants, if the hardware gives a choice?

Best regards,
Tomasz