| Message ID | 20180802200010.24365-6-ezequiel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Rockchip VPU JPEG encoder | expand |
Hi Ezequiel, On Fri, Aug 3, 2018 at 5:00 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > From: Shunqian Zheng <zhengsq@rock-chips.com> > > Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace > configure the JPEG quantization tables. > > Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > 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. > > 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? > > .. 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
On 17/08/18 04:10, Tomasz Figa wrote: > Hi Ezequiel, > > On Fri, Aug 3, 2018 at 5:00 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: >> >> From: Shunqian Zheng <zhengsq@rock-chips.com> >> >> Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace >> configure the JPEG quantization tables. >> >> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> >> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >> --- >> 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. > >> >> 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? > >> >> .. 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? I noticed this when reviewing. I have a slight preference for setting the dims and min/max in the core. It's pretty standard how this should behave after all. Regards, Hans
On Fri, 2018-08-17 at 11:10 +0900, Tomasz Figa wrote: > Hi Ezequiel, > > On Fri, Aug 3, 2018 at 5:00 AM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > > From: Shunqian Zheng <zhengsq@rock-chips.com> > > > > Add V4L2_CID_JPEG_LUMA/CHROMA_QUANTIZATION controls to allow userspace > > configure the JPEG quantization tables. > > > > Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > 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. > > > > > 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? > Yes, could be. > > + > > +``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? > In spec documentation I usually find it's clearer for readers to see stuff repeated. Better to have an excess of clarity :-) > > > > .. 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? > Since Hans agrees to move this to the core, let's give it a try. I'll address this in v3. Thanks for the feedback! Eze
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. + +``V4L2_CID_JPEG_CHROMA_QUANTIZATION (__u8 matrix)`` + Sets the chroma quantization table. .. 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: *type = V4L2_CTRL_TYPE_U8; break; case V4L2_CID_DETECT_MD_THRESHOLD_GRID: diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h index e4ee10ee917d..a466acf40625 100644 --- a/include/uapi/linux/v4l2-controls.h +++ b/include/uapi/linux/v4l2-controls.h @@ -987,6 +987,9 @@ enum v4l2_jpeg_chroma_subsampling { #define V4L2_JPEG_ACTIVE_MARKER_DQT (1 << 17) #define V4L2_JPEG_ACTIVE_MARKER_DHT (1 << 18) +#define V4L2_CID_JPEG_LUMA_QUANTIZATION (V4L2_CID_JPEG_CLASS_BASE + 5) +#define V4L2_CID_JPEG_CHROMA_QUANTIZATION (V4L2_CID_JPEG_CLASS_BASE + 6) + /* Image source controls */ #define V4L2_CID_IMAGE_SOURCE_CLASS_BASE (V4L2_CTRL_CLASS_IMAGE_SOURCE | 0x900)