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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux