On 09/03/2018 05:27 PM, Ezequiel Garcia wrote: > Hi Ian, Hans: > > On Mon, 2018-09-03 at 14:29 +0100, Ian Arkver wrote: >> Hi, >> >> On 03/09/2018 10:50, Hans Verkuil wrote: >>> On 08/31/2018 05:52 PM, 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 | 23 +++++++++++++++++++ >>>> .../media/videodev2.h.rst.exceptions | 1 + >>>> drivers/media/v4l2-core/v4l2-ctrls.c | 10 ++++++++ >>>> include/uapi/linux/v4l2-controls.h | 5 ++++ >>>> include/uapi/linux/videodev2.h | 1 + >>>> 5 files changed, 40 insertions(+) >>>> >>>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst >>>> index 9f7312bf3365..e0dd03e452de 100644 >>>> --- a/Documentation/media/uapi/v4l/extended-controls.rst >>>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst >>>> @@ -3354,7 +3354,30 @@ 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 two matrices >>>> + must be set in JPEG zigzag order, as per the JPEG specification. >>> >>> Can you change "JPEG specification" to a reference to the JPEG spec entry >>> in bibio.rst? >>> >>>> + >>>> + >>>> +.. 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 >>>> + - ``luma_quantization_matrix[64]`` >>>> + - Sets the luma quantization table. >>>> + >>>> + * - __u8 >>>> + - ``chroma_quantization_matrix[64]`` >>>> + - Sets the chroma quantization table. >>> >>> Just checking: the JPEG standard specifies this as unsigned 8-bit values as well? >> > > I thought this was already discussed, but I think the only thing I've added > is this comment in one of the driver's headers: > > JPEG encoder > ------------ > The VPU JPEG encoder produces JPEG baseline sequential format. > The quantization coefficients are 8-bit values, complying with > the baseline specification. Therefore, it requires application-defined > luma and chroma quantization tables. The hardware does entrophy > encoding using internal Huffman tables, as specified in the JPEG > specification. > > Certainly controls should be specified better. > >> As far as I can see ISO/IEC 10918-1 does not specify the precision or >> signedness of the quantisation value Qvu. The default tables for 8-bit >> baseline JPEG all fit into __u8 though. >> > > Paragraph 4.7 of that spec, indicates the "sample" precision: > 8-bit for baseline; 8-bit or 12-bit for extended. > > For the quantization coefficients, the DQT segment contains a bit > that indicates if the quantization coefficients are 8-bit or 16-bit. > See B.2.4.1 for details. > >> However there can be four sets of tables in non-baseline JPEG and it's > > You lost me here, which four sets of tables are you refering to? > >> not clear (to me) whether 12-bit JPEG would need more precision (I'd >> guess it would). > > It seems it would. From B.2.4.1: > > "An 8-bit DCT-based process shall not use a 16-bit precision quantization table." > >> Since this patch is defining UAPI I think it might be >> good to build in some additional information, eg. number of tables, >> element size. Maybe this can all be inferred from the selected pixel >> format? If so then it would need documented that the above structure >> only applies to baseline. >> > > For quantization coefficients, I can only see two tables: one for luma > one for chroma. Huffman coefficients are a different story and we are > not really adding them here. Since (if I understand this correctly) we would need u16 for extended precision JPEG, shouldn't we use u16 instead of u8? That makes the control more generic. BTW, are the coefficients always unsigned? I think so, but I never read the JPEG spec. Regards, Hans > > Thanks, > Eze >
