On Sun, Sep 16, 2018 at 1:48 AM Paul Kocialkowski <contact@xxxxxxxx> wrote: > > Hi, > > On Mon, 2018-09-10 at 10:25 -0300, Ezequiel Garcia wrote: > > Hi Hans, > > > > Thanks for the review. > > > > On Mon, 2018-09-10 at 14:42 +0200, Hans Verkuil wrote: > > > On 09/06/2018 12:00 AM, Ezequiel Garcia wrote: > > > > From: Shunqian Zheng <zhengsq@xxxxxxxxxxxxxx> > > > > > > > > Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace > > > > configure the JPEG quantization tables. > > > > > > > > Signed-off-by: Shunqian Zheng <zhengsq@xxxxxxxxxxxxxx> > > > > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> > > > > --- > > > > .../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. > > > > > 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. > > > 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
