diff mbox series

[v13,03/13] media: v4l2-jpeg: Export reference quantization and huffman tables

Message ID 20240607132831.3551333-1-devarsht@ti.com
State New
Headers show
Series Add V4L2 M2M Driver for E5010 JPEG Encoder | expand

Commit Message

Devarsh Thakkar June 7, 2024, 1:28 p.m. UTC 
Export reference quantization and huffman tables as provided in ITU-T.81 so
that they can be re-used by other JPEG drivers.

These are example tables provided in ITU-T.81 as reference tables and the
JPEG encoders are free to use either these or their own proprietary tables.

Also add necessary prefixes to be used for huffman tables in global header
file.

Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
---
V13: Fix smatch/sparse related below warning by using static global
variable for arrays:
Logs:
+drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'luma_qt' was not
declared. Should it be static?
+drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'chroma_qt' was
not declared. Should it be static?
+drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'zigzag' was not
declared. Should it be static?
+drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'luma_dc_ht' was
not declared. Should it be static?
+drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'luma_ac_ht' was
not declared. Should it be static?
+drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'chroma_dc_ht' was
not declared. Should it be static?
+drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'chroma_ac_ht' was
not declared. Should it be static?"

V12: Fix html-doc warnings by removing * from args
V11: No change
V10: Add description for new macros introduced in this patchset
V1->V9: No change (Patch introduced in V7)
 drivers/media/v4l2-core/v4l2-jpeg.c | 162 +++++++++++++++++++++++++++-
 include/media/v4l2-jpeg.h           |  28 +++++
 2 files changed, 189 insertions(+), 1 deletion(-)

Comments

Hans Verkuil June 13, 2024, 10:08 a.m. UTC | #1 
Hi Devarsh,

On 07/06/2024 15:28, Devarsh Thakkar wrote:
> Export reference quantization and huffman tables as provided in ITU-T.81 so
> that they can be re-used by other JPEG drivers.
> 
> These are example tables provided in ITU-T.81 as reference tables and the
> JPEG encoders are free to use either these or their own proprietary tables.
> 
> Also add necessary prefixes to be used for huffman tables in global header
> file.
> 
> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
> ---
> V13: Fix smatch/sparse related below warning by using static global
> variable for arrays:
> Logs:
> +drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'luma_qt' was not
> declared. Should it be static?
> +drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'chroma_qt' was
> not declared. Should it be static?
> +drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'zigzag' was not
> declared. Should it be static?
> +drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'luma_dc_ht' was
> not declared. Should it be static?
> +drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'luma_ac_ht' was
> not declared. Should it be static?
> +drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'chroma_dc_ht' was
> not declared. Should it be static?
> +drivers/media/v4l2-core/v4l2-jpeg.c: warning: symbol 'chroma_ac_ht' was
> not declared. Should it be static?"
> 
> V12: Fix html-doc warnings by removing * from args
> V11: No change
> V10: Add description for new macros introduced in this patchset
> V1->V9: No change (Patch introduced in V7)
>  drivers/media/v4l2-core/v4l2-jpeg.c | 162 +++++++++++++++++++++++++++-
>  include/media/v4l2-jpeg.h           |  28 +++++
>  2 files changed, 189 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c
> index 94435a7b6816..bd6305c92e0e 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -16,7 +16,7 @@
>  #include <linux/types.h>
>  #include <media/v4l2-jpeg.h>
>  
> -MODULE_DESCRIPTION("V4L2 JPEG header parser helpers");
> +MODULE_DESCRIPTION("V4L2 JPEG helpers");
>  MODULE_AUTHOR("Philipp Zabel <kernel@pengutronix.de>");
>  MODULE_LICENSE("GPL");
>  
> @@ -52,6 +52,115 @@ MODULE_LICENSE("GPL");
>  #define COM	0xfffe	/* comment */
>  #define TEM	0xff01	/* temporary */
>  
> +/* Luma and chroma qp tables to achieve 50% compression quality
> + * This is as per example in Annex K.1 of ITU-T.81
> + */
> +static const u8 luma_qt[] = {
> +	16, 11, 10, 16, 24, 40, 51, 61,
> +	12, 12, 14, 19, 26, 58, 60, 55,
> +	14, 13, 16, 24, 40, 57, 69, 56,
> +	14, 17, 22, 29, 51, 87, 80, 62,
> +	18, 22, 37, 56, 68, 109, 103, 77,
> +	24, 35, 55, 64, 81, 104, 113, 92,
> +	49, 64, 78, 87, 103, 121, 120, 101,
> +	72, 92, 95, 98, 112, 100, 103, 99
> +};

Why make this so complicated?

Just do this:

const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK] = {
	16, 11, 10, 16,  24,  40,  51,  61,
	12, 12, 14, 19,  26,  58,  60,  55,
	14, 13, 16, 24,  40,  57,  69,  56,
	14, 17, 22, 29,  51,  87,  80,  62,
	18, 22, 37, 56,  68, 109, 103,  77,
	24, 35, 55, 64,  81, 104, 113,  92,
	49, 64, 78, 87, 103, 121, 120, 101,
	72, 92, 95, 98, 112, 100, 103,  99
};
EXPORT_SYMBOL_GPL(v4l2_jpeg_table_luma_qt);

and in the header add:

extern const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK];

Same for the other tables.

And in the header add:

extern const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK];

It's similar to e.g. 'const u8 v4l2_vp9_kf_y_mode_prob[10][10][9];'
in v4l2-vp9.c/h.

It also ensures that the compiler knows the size of each array,
so it can detect out-of-bounds errors. And you can drop the accessor
functions, as there is no longer any need for that.

I really want this out-of-bounds detection, the code as it is now is too
risky. So please make a v14.

> +
> +static const u8 chroma_qt[] = {

Just to make it clear: don't use [] here, use the actual define for the
array size. That way you get a compiler warning if you missed an entry
in the initialization.

Apologies for the late review, I only noticed this when I checked the
pull request.

Regards,

	Hans

> +	17, 18, 24, 47, 99, 99, 99, 99,
> +	18, 21, 26, 66, 99, 99, 99, 99,
> +	24, 26, 56, 99, 99, 99, 99, 99,
> +	47, 66, 99, 99, 99, 99, 99, 99,
> +	99, 99, 99, 99, 99, 99, 99, 99,
> +	99, 99, 99, 99, 99, 99, 99, 99,
> +	99, 99, 99, 99, 99, 99, 99, 99,
> +	99, 99, 99, 99, 99, 99, 99, 99
> +};
> +
> +/* Zigzag scan pattern */
> +static const u8 zigzag[] = {
> +	0,   1,  8, 16,  9,  2,  3, 10,
> +	17, 24, 32, 25, 18, 11,  4,  5,
> +	12, 19, 26, 33, 40, 48, 41, 34,
> +	27, 20, 13,  6,  7, 14, 21, 28,
> +	35, 42, 49, 56, 57, 50, 43, 36,
> +	29, 22, 15, 23, 30, 37, 44, 51,
> +	58, 59, 52, 45, 38, 31, 39, 46,
> +	53, 60, 61, 54, 47, 55, 62, 63
> +};
> +
> +/*
> + * Contains the data that needs to be sent in the marker segment of an
> + * interchange format JPEG stream or an abbreviated format table specification
> + * data stream. Specifies the huffman table used for encoding the luminance DC
> + * coefficient differences. The table represents Table K.3 of ITU-T.81
> + */
> +static const u8 luma_dc_ht[] = {
> +	0x00, 0x01, 0x05, 0x01, 0x01, 0x01, 0x01, 0x01,
> +	0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B
> +};
> +
> +/*
> + * Contains the data that needs to be sent in the marker segment of an
> + * interchange format JPEG stream or an abbreviated format table specification
> + * data stream. Specifies the huffman table used for encoding the luminance AC
> + * coefficients. The table represents Table K.5 of ITU-T.81
> + */
> +static const u8 luma_ac_ht[] = {
> +	0x00, 0x02, 0x01, 0x03, 0x03, 0x02, 0x04, 0x03, 0x05, 0x05, 0x04, 0x04,
> +	0x00, 0x00, 0x01, 0x7D, 0x01, 0x02, 0x03, 0x00, 0x04, 0x11, 0x05, 0x12,
> +	0x21, 0x31, 0x41, 0x06, 0x13, 0x51, 0x61, 0x07, 0x22, 0x71, 0x14, 0x32,
> +	0x81, 0x91, 0xA1, 0x08, 0x23, 0x42, 0xB1, 0xC1, 0x15, 0x52, 0xD1, 0xF0,
> +	0x24, 0x33, 0x62, 0x72, 0x82, 0x09, 0x0A, 0x16, 0x17, 0x18, 0x19, 0x1A,
> +	0x25, 0x26, 0x27, 0x28, 0x29, 0x2A, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39,
> +	0x3A, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 0x49, 0x4A, 0x53, 0x54, 0x55,
> +	0x56, 0x57, 0x58, 0x59, 0x5A, 0x63, 0x64, 0x65, 0x66, 0x67, 0x68, 0x69,
> +	0x6A, 0x73, 0x74, 0x75, 0x76, 0x77, 0x78, 0x79, 0x7A, 0x83, 0x84, 0x85,
> +	0x86, 0x87, 0x88, 0x89, 0x8A, 0x92, 0x93, 0x94, 0x95, 0x96, 0x97, 0x98,
> +	0x99, 0x9A, 0xA2, 0xA3, 0xA4, 0xA5, 0xA6, 0xA7, 0xA8, 0xA9, 0xAA, 0xB2,
> +	0xB3, 0xB4, 0xB5, 0xB6, 0xB7, 0xB8, 0xB9, 0xBA, 0xC2, 0xC3, 0xC4, 0xC5,
> +	0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6, 0xD7, 0xD8,
> +	0xD9, 0xDA, 0xE1, 0xE2, 0xE3, 0xE4, 0xE5, 0xE6, 0xE7, 0xE8, 0xE9, 0xEA,
> +	0xF1, 0xF2, 0xF3, 0xF4, 0xF5, 0xF6, 0xF7, 0xF8, 0xF9, 0xFA
> +};
> +
> +/*
> + * Contains the data that needs to be sent in the marker segment of an interchange format JPEG
> + * stream or an abbreviated format table specification data stream.
> + * Specifies the huffman table used for encoding the chrominance DC coefficient differences.
> + * The table represents Table K.4 of ITU-T.81
> + */
> +static const u8 chroma_dc_ht[] = {
> +	0x00, 0x03, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
> +	0x01, 0x01, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
> +	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B
> +};
> +
> +/*
> + * Contains the data that needs to be sent in the marker segment of an
> + * interchange format JPEG stream or an abbreviated format table specification
> + * data stream. Specifies the huffman table used for encoding the chrominance
> + * AC coefficients. The table represents Table K.6 of ITU-T.81
> + */
> +static const u8 chroma_ac_ht[] = {
> +	0x00, 0x02, 0x01, 0x02, 0x04, 0x04, 0x03, 0x04, 0x07, 0x05, 0x04, 0x04,
> +	0x00, 0x01, 0x02, 0x77, 0x00, 0x01, 0x02, 0x03, 0x11, 0x04, 0x05, 0x21,
> +	0x31, 0x06, 0x12, 0x41, 0x51, 0x07, 0x61, 0x71, 0x13, 0x22, 0x32, 0x81,
> +	0x08, 0x14, 0x42, 0x91, 0xA1, 0xB1, 0xC1, 0x09, 0x23, 0x33, 0x52, 0xF0,
> +	0x15, 0x62, 0x72, 0xD1, 0x0A, 0x16, 0x24, 0x34, 0xE1, 0x25, 0xF1, 0x17,
> +	0x18, 0x19, 0x1A, 0x26, 0x27, 0x28, 0x29, 0x2A, 0x35, 0x36, 0x37, 0x38,
> +	0x39, 0x3A, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 0x49, 0x4A, 0x53, 0x54,
> +	0x55, 0x56, 0x57, 0x58, 0x59, 0x5A, 0x63, 0x64, 0x65, 0x66, 0x67, 0x68,
> +	0x69, 0x6A, 0x73, 0x74, 0x75, 0x76, 0x77, 0x78, 0x79, 0x7A, 0x82, 0x83,
> +	0x84, 0x85, 0x86, 0x87, 0x88, 0x89, 0x8A, 0x92, 0x93, 0x94, 0x95, 0x96,
> +	0x97, 0x98, 0x99, 0x9A, 0xA2, 0xA3, 0xA4, 0xA5, 0xA6, 0xA7, 0xA8, 0xA9,
> +	0xAA, 0xB2, 0xB3, 0xB4, 0xB5, 0xB6, 0xB7, 0xB8, 0xB9, 0xBA, 0xC2, 0xC3,
> +	0xC4, 0xC5, 0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6,
> +	0xD7, 0xD8, 0xD9, 0xDA, 0xE2, 0xE3, 0xE4, 0xE5, 0xE6, 0xE7, 0xE8, 0xE9,
> +	0xEA, 0xF2, 0xF3, 0xF4, 0xF5, 0xF6, 0xF7, 0xF8, 0xF9, 0xFA
> +};
> +
>  /**
>   * struct jpeg_stream - JPEG byte stream
>   * @curr: current position in stream
> @@ -675,3 +784,54 @@ int v4l2_jpeg_parse_huffman_tables(void *buf, size_t len,
>  	return jpeg_parse_huffman_tables(&stream, huffman_tables);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_jpeg_parse_huffman_tables);
> +
> +/**
> + * v4l2_jpeg_get_reference_quantization_tables - Get reference quantization
> + *						 tables as defined in ITU-T.81
> + * @ref_luma_qt: Output variable pointing to luma quantization table
> + * @ref_chroma_qt: Output variable pointing to chroma quantization table
> + */
> +void v4l2_jpeg_get_reference_quantization_tables(const u8 **ref_luma_qt, const
> +						 u8 **ref_chroma_qt)
> +{
> +	if (ref_luma_qt)
> +		*ref_luma_qt = luma_qt;
> +	if (ref_chroma_qt)
> +		*ref_chroma_qt = chroma_qt;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_jpeg_get_reference_quantization_tables);
> +
> +/**
> + * v4l2_jpeg_get_zig_zag_scan - Get zigzag scan table as defined in ITU-T.81
> + * @ref_zigzag: Output variable pointing to zigzag scan table
> + */
> +void v4l2_jpeg_get_zig_zag_scan(const u8 **ref_zigzag)
> +{
> +	if (ref_zigzag)
> +		*ref_zigzag = zigzag;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_jpeg_get_zig_zag_scan);
> +
> +/**
> + * v4l2_jpeg_get_reference_huffman_tables - Get reference huffman tables as
> + *					    defined in ITU-T.81
> + * @ref_luma_dc_ht : Output variable pointing to huffman table for luma DC
> + * @ref_luma_ac_ht : Output variable pointing to huffman table for luma AC
> + * @ref_chroma_dc_ht : Output variable pointing to huffman table for chroma DC
> + * @ref_chroma_ac_ht : Output variable pointing to huffman table for chroma AC
> + */
> +void v4l2_jpeg_get_reference_huffman_tables(const u8 **ref_luma_dc_ht,
> +					    const u8 **ref_luma_ac_ht,
> +					    const u8 **ref_chroma_dc_ht,
> +					    const u8 **ref_chroma_ac_ht)
> +{
> +	if (ref_luma_dc_ht)
> +		*ref_luma_dc_ht = luma_dc_ht;
> +	if (ref_luma_ac_ht)
> +		*ref_luma_ac_ht = luma_ac_ht;
> +	if (ref_chroma_dc_ht)
> +		*ref_chroma_dc_ht = chroma_dc_ht;
> +	if (ref_chroma_ac_ht)
> +		*ref_chroma_ac_ht = chroma_ac_ht;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_jpeg_get_reference_huffman_tables);
> diff --git a/include/media/v4l2-jpeg.h b/include/media/v4l2-jpeg.h
> index 2dba843ce3bd..b470bbffb73f 100644
> --- a/include/media/v4l2-jpeg.h
> +++ b/include/media/v4l2-jpeg.h
> @@ -14,6 +14,30 @@
>  
>  #define V4L2_JPEG_MAX_COMPONENTS	4
>  #define V4L2_JPEG_MAX_TABLES		4
> +/*
> + * Prefixes used to generate huffman table class and destination identifiers as
> + * described below:
> + *
> + * V4L2_JPEG_LUM_HT | V4L2_JPEG_DC_HT : Prefix for Luma DC coefficients
> + *					huffman table
> + * V4L2_JPEG_LUM_HT | V4L2_JPEG_AC_HT : Prefix for Luma AC coefficients
> + *					huffman table
> + * V4L2_JPEG_CHR_HT | V4L2_JPEG_DC_HT : Prefix for Chroma DC coefficients
> + *					huffman table
> + * V4L2_JPEG_CHR_HT | V4L2_JPEG_AC_HT : Prefix for Chroma AC coefficients
> + *					huffman table
> + */
> +#define V4L2_JPEG_LUM_HT		0x00
> +#define V4L2_JPEG_CHR_HT		0x01
> +#define V4L2_JPEG_DC_HT			0x00
> +#define V4L2_JPEG_AC_HT			0x10
> +
> +/* Length of reference huffman tables as provided in Table K.3 of ITU-T.81 */
> +#define V4L2_JPEG_REF_HT_AC_LEN		178
> +#define V4L2_JPEG_REF_HT_DC_LEN		28
> +
> +/* Array size for 8x8 block of samples or DCT coefficient */
> +#define V4L2_JPEG_PIXELS_IN_BLOCK	64
>  
>  /**
>   * struct v4l2_jpeg_reference - reference into the JPEG buffer
> @@ -154,4 +178,8 @@ int v4l2_jpeg_parse_quantization_tables(void *buf, size_t len, u8 precision,
>  int v4l2_jpeg_parse_huffman_tables(void *buf, size_t len,
>  				   struct v4l2_jpeg_reference *huffman_tables);
>  
> +void v4l2_jpeg_get_reference_quantization_tables(const u8 **luma_qt, const u8 **chroma_qt);
> +void v4l2_jpeg_get_zig_zag_scan(const u8 **zigzag);
> +void v4l2_jpeg_get_reference_huffman_tables(const u8 **luma_dc_ht, const u8 **luma_ac_ht,
> +					    const u8 **chroma_dc_ht, const u8 **chroma_ac_ht);
>  #endif
Devarsh Thakkar June 19, 2024, 2:19 p.m. UTC | #2 
Hi Hans, Sebastian,

Thanks for the review Hans.

On 13/06/24 15:38, Hans Verkuil wrote:
> Hi Devarsh,

[..]
> 
> Why make this so complicated?
> 
> Just do this:
> 
> const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK] = {
> 	16, 11, 10, 16,  24,  40,  51,  61,
> 	12, 12, 14, 19,  26,  58,  60,  55,
> 	14, 13, 16, 24,  40,  57,  69,  56,
> 	14, 17, 22, 29,  51,  87,  80,  62,
> 	18, 22, 37, 56,  68, 109, 103,  77,
> 	24, 35, 55, 64,  81, 104, 113,  92,
> 	49, 64, 78, 87, 103, 121, 120, 101,
> 	72, 92, 95, 98, 112, 100, 103,  99
> };
> EXPORT_SYMBOL_GPL(v4l2_jpeg_table_luma_qt);
> 
> and in the header add:
> 
> extern const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK];
> 
> Same for the other tables.
> 
> And in the header add:
> 
> extern const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK];
> 
> It's similar to e.g. 'const u8 v4l2_vp9_kf_y_mode_prob[10][10][9];'
> in v4l2-vp9.c/h.
> 
> It also ensures that the compiler knows the size of each array,
> so it can detect out-of-bounds errors. And you can drop the accessor
> functions, as there is no longer any need for that.
> 
> I really want this out-of-bounds detection, the code as it is now is too
> risky. So please make a v14.
>

Yes agreed, initially I had a similar thought to use extern declared variables
but somehow couldn't find any good examples as you shared so thought to have
wrapper functions but anyways have fixed this in v14.
 >> +
>> +static const u8 chroma_qt[] = {
> 
> Just to make it clear: don't use [] here, use the actual define for the
> array size. That way you get a compiler warning if you missed an entry
> in the initialization.
> 
> Apologies for the late review, I only noticed this when I checked the
> pull request.
> 

No worries for the delay, these are good comments and I have fixed them in v14
[1] appreciate if it's possible to have a quick review and if it looks good
possible to pull it in this week's RC cycle ? This will help me plan to send
math.h and rounding related patches (patch 7/13 to patch 12/13) from v13 [2]
as separate series more quickly as aligned. Also there was a new suggestion
[3] to use guard(mutex) in remove method, I was thinking to evaluate that and
pull that in as a separate patchset after this series gets merged and include
as part of next set of patches involving math.h and rounding macros discussed
above so that I can test them all together all at once since I am running a
bunch of manual and automated tests so wanted to reduce the cycles, will that
be fine ?

Kindly let me know your opinion.

[1]: https://lore.kernel.org/all/20240618193651.2771478-1-devarsht@ti.com/
[2]: https://lore.kernel.org/all/20240607131900.3535250-1-devarsht@ti.com/
[3]: https://lore.kernel.org/all/2fed4937-e9ea-4635-a061-5c5a0533b152@web.de/

Regards
Devarsh
Hans Verkuil June 20, 2024, 7:02 a.m. UTC | #3 
On 19/06/2024 16:19, Devarsh Thakkar wrote:
> Hi Hans, Sebastian,
> 
> Thanks for the review Hans.
> 
> On 13/06/24 15:38, Hans Verkuil wrote:
>> Hi Devarsh,
> 
> [..]
>>
>> Why make this so complicated?
>>
>> Just do this:
>>
>> const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK] = {
>> 	16, 11, 10, 16,  24,  40,  51,  61,
>> 	12, 12, 14, 19,  26,  58,  60,  55,
>> 	14, 13, 16, 24,  40,  57,  69,  56,
>> 	14, 17, 22, 29,  51,  87,  80,  62,
>> 	18, 22, 37, 56,  68, 109, 103,  77,
>> 	24, 35, 55, 64,  81, 104, 113,  92,
>> 	49, 64, 78, 87, 103, 121, 120, 101,
>> 	72, 92, 95, 98, 112, 100, 103,  99
>> };
>> EXPORT_SYMBOL_GPL(v4l2_jpeg_table_luma_qt);
>>
>> and in the header add:
>>
>> extern const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK];
>>
>> Same for the other tables.
>>
>> And in the header add:
>>
>> extern const u8 v4l2_jpeg_table_luma_qt[V4L2_JPEG_PIXELS_IN_BLOCK];
>>
>> It's similar to e.g. 'const u8 v4l2_vp9_kf_y_mode_prob[10][10][9];'
>> in v4l2-vp9.c/h.
>>
>> It also ensures that the compiler knows the size of each array,
>> so it can detect out-of-bounds errors. And you can drop the accessor
>> functions, as there is no longer any need for that.
>>
>> I really want this out-of-bounds detection, the code as it is now is too
>> risky. So please make a v14.
>>
> 
> Yes agreed, initially I had a similar thought to use extern declared variables
> but somehow couldn't find any good examples as you shared so thought to have
> wrapper functions but anyways have fixed this in v14.
>  >> +
>>> +static const u8 chroma_qt[] = {
>>
>> Just to make it clear: don't use [] here, use the actual define for the
>> array size. That way you get a compiler warning if you missed an entry
>> in the initialization.
>>
>> Apologies for the late review, I only noticed this when I checked the
>> pull request.
>>
> 
> No worries for the delay, these are good comments and I have fixed them in v14
> [1] appreciate if it's possible to have a quick review and if it looks good
> possible to pull it in this week's RC cycle ? This will help me plan to send
> math.h and rounding related patches (patch 7/13 to patch 12/13) from v13 [2]
> as separate series more quickly as aligned. Also there was a new suggestion
> [3] to use guard(mutex) in remove method, I was thinking to evaluate that and
> pull that in as a separate patchset after this series gets merged and include
> as part of next set of patches involving math.h and rounding macros discussed
> above so that I can test them all together all at once since I am running a
> bunch of manual and automated tests so wanted to reduce the cycles, will that
> be fine ?

I've reviewed v14 3/6, so Sebastian when Sebastian posts a new PR I can process
it.

Ignore any reviews by Markus Elfring, he's a troll.

Regards,

	Hans
Markus Elfring June 20, 2024, 7:22 a.m. UTC | #4 
> Ignore any reviews by Markus Elfring,

There are some communication difficulties involved.


> he's a troll.

Some contributors occasionally got an other impression, didn't they?

Regards,
Markus
Devarsh Thakkar June 21, 2024, 8:52 a.m. UTC | #5 
Hi Sebastian,

On 20/06/24 12:32, Hans Verkuil wrote:
> On 19/06/2024 16:19, Devarsh Thakkar wrote:
[..]
> I've reviewed v14 3/6, so Sebastian when Sebastian posts a new PR I can process
> it.
> 
> Ignore any reviews by Markus Elfring, he's a troll.
> 

Just wanted to check if we are good w.r.t V14 [1] and it's possible for you to
create a PR with V14 as discussed above?

[1]: https://lore.kernel.org/all/20240618193651.2771478-1-devarsht@ti.com/

Regards
Devarsh
Sebastian Fricke June 21, 2024, 9:28 a.m. UTC | #6 
Hey Devarsh,

On 21.06.2024 14:22, Devarsh Thakkar wrote:
>Hi Sebastian,
>
>On 20/06/24 12:32, Hans Verkuil wrote:
>> On 19/06/2024 16:19, Devarsh Thakkar wrote:
>[..]
>> I've reviewed v14 3/6, so Sebastian when Sebastian posts a new PR I can process
>> it.
>>
>> Ignore any reviews by Markus Elfring, he's a troll.
>>
>
>Just wanted to check if we are good w.r.t V14 [1] and it's possible for you to
>create a PR with V14 as discussed above?

Yup looks good so far, as soon as I have a timeslot I'll send it out.

>
>[1]: https://lore.kernel.org/all/20240618193651.2771478-1-devarsht@ti.com/
>
>Regards
>Devarsh

Regards,
Sebastian
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c b/drivers/media/v4l2-core/v4l2-jpeg.c
index 94435a7b6816..bd6305c92e0e 100644
--- a/drivers/media/v4l2-core/v4l2-jpeg.c
+++ b/drivers/media/v4l2-core/v4l2-jpeg.c
@@ -16,7 +16,7 @@ 
 #include <linux/types.h>
 #include <media/v4l2-jpeg.h>
 
-MODULE_DESCRIPTION("V4L2 JPEG header parser helpers");
+MODULE_DESCRIPTION("V4L2 JPEG helpers");
 MODULE_AUTHOR("Philipp Zabel <kernel@pengutronix.de>");
 MODULE_LICENSE("GPL");
 
@@ -52,6 +52,115 @@  MODULE_LICENSE("GPL");
 #define COM	0xfffe	/* comment */
 #define TEM	0xff01	/* temporary */
 
+/* Luma and chroma qp tables to achieve 50% compression quality
+ * This is as per example in Annex K.1 of ITU-T.81
+ */
+static const u8 luma_qt[] = {
+	16, 11, 10, 16, 24, 40, 51, 61,
+	12, 12, 14, 19, 26, 58, 60, 55,
+	14, 13, 16, 24, 40, 57, 69, 56,
+	14, 17, 22, 29, 51, 87, 80, 62,
+	18, 22, 37, 56, 68, 109, 103, 77,
+	24, 35, 55, 64, 81, 104, 113, 92,
+	49, 64, 78, 87, 103, 121, 120, 101,
+	72, 92, 95, 98, 112, 100, 103, 99
+};
+
+static const u8 chroma_qt[] = {
+	17, 18, 24, 47, 99, 99, 99, 99,
+	18, 21, 26, 66, 99, 99, 99, 99,
+	24, 26, 56, 99, 99, 99, 99, 99,
+	47, 66, 99, 99, 99, 99, 99, 99,
+	99, 99, 99, 99, 99, 99, 99, 99,
+	99, 99, 99, 99, 99, 99, 99, 99,
+	99, 99, 99, 99, 99, 99, 99, 99,
+	99, 99, 99, 99, 99, 99, 99, 99
+};
+
+/* Zigzag scan pattern */
+static const u8 zigzag[] = {
+	0,   1,  8, 16,  9,  2,  3, 10,
+	17, 24, 32, 25, 18, 11,  4,  5,
+	12, 19, 26, 33, 40, 48, 41, 34,
+	27, 20, 13,  6,  7, 14, 21, 28,
+	35, 42, 49, 56, 57, 50, 43, 36,
+	29, 22, 15, 23, 30, 37, 44, 51,
+	58, 59, 52, 45, 38, 31, 39, 46,
+	53, 60, 61, 54, 47, 55, 62, 63
+};
+
+/*
+ * Contains the data that needs to be sent in the marker segment of an
+ * interchange format JPEG stream or an abbreviated format table specification
+ * data stream. Specifies the huffman table used for encoding the luminance DC
+ * coefficient differences. The table represents Table K.3 of ITU-T.81
+ */
+static const u8 luma_dc_ht[] = {
+	0x00, 0x01, 0x05, 0x01, 0x01, 0x01, 0x01, 0x01,
+	0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B
+};
+
+/*
+ * Contains the data that needs to be sent in the marker segment of an
+ * interchange format JPEG stream or an abbreviated format table specification
+ * data stream. Specifies the huffman table used for encoding the luminance AC
+ * coefficients. The table represents Table K.5 of ITU-T.81
+ */
+static const u8 luma_ac_ht[] = {
+	0x00, 0x02, 0x01, 0x03, 0x03, 0x02, 0x04, 0x03, 0x05, 0x05, 0x04, 0x04,
+	0x00, 0x00, 0x01, 0x7D, 0x01, 0x02, 0x03, 0x00, 0x04, 0x11, 0x05, 0x12,
+	0x21, 0x31, 0x41, 0x06, 0x13, 0x51, 0x61, 0x07, 0x22, 0x71, 0x14, 0x32,
+	0x81, 0x91, 0xA1, 0x08, 0x23, 0x42, 0xB1, 0xC1, 0x15, 0x52, 0xD1, 0xF0,
+	0x24, 0x33, 0x62, 0x72, 0x82, 0x09, 0x0A, 0x16, 0x17, 0x18, 0x19, 0x1A,
+	0x25, 0x26, 0x27, 0x28, 0x29, 0x2A, 0x34, 0x35, 0x36, 0x37, 0x38, 0x39,
+	0x3A, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 0x49, 0x4A, 0x53, 0x54, 0x55,
+	0x56, 0x57, 0x58, 0x59, 0x5A, 0x63, 0x64, 0x65, 0x66, 0x67, 0x68, 0x69,
+	0x6A, 0x73, 0x74, 0x75, 0x76, 0x77, 0x78, 0x79, 0x7A, 0x83, 0x84, 0x85,
+	0x86, 0x87, 0x88, 0x89, 0x8A, 0x92, 0x93, 0x94, 0x95, 0x96, 0x97, 0x98,
+	0x99, 0x9A, 0xA2, 0xA3, 0xA4, 0xA5, 0xA6, 0xA7, 0xA8, 0xA9, 0xAA, 0xB2,
+	0xB3, 0xB4, 0xB5, 0xB6, 0xB7, 0xB8, 0xB9, 0xBA, 0xC2, 0xC3, 0xC4, 0xC5,
+	0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6, 0xD7, 0xD8,
+	0xD9, 0xDA, 0xE1, 0xE2, 0xE3, 0xE4, 0xE5, 0xE6, 0xE7, 0xE8, 0xE9, 0xEA,
+	0xF1, 0xF2, 0xF3, 0xF4, 0xF5, 0xF6, 0xF7, 0xF8, 0xF9, 0xFA
+};
+
+/*
+ * Contains the data that needs to be sent in the marker segment of an interchange format JPEG
+ * stream or an abbreviated format table specification data stream.
+ * Specifies the huffman table used for encoding the chrominance DC coefficient differences.
+ * The table represents Table K.4 of ITU-T.81
+ */
+static const u8 chroma_dc_ht[] = {
+	0x00, 0x03, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01,
+	0x01, 0x01, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B
+};
+
+/*
+ * Contains the data that needs to be sent in the marker segment of an
+ * interchange format JPEG stream or an abbreviated format table specification
+ * data stream. Specifies the huffman table used for encoding the chrominance
+ * AC coefficients. The table represents Table K.6 of ITU-T.81
+ */
+static const u8 chroma_ac_ht[] = {
+	0x00, 0x02, 0x01, 0x02, 0x04, 0x04, 0x03, 0x04, 0x07, 0x05, 0x04, 0x04,
+	0x00, 0x01, 0x02, 0x77, 0x00, 0x01, 0x02, 0x03, 0x11, 0x04, 0x05, 0x21,
+	0x31, 0x06, 0x12, 0x41, 0x51, 0x07, 0x61, 0x71, 0x13, 0x22, 0x32, 0x81,
+	0x08, 0x14, 0x42, 0x91, 0xA1, 0xB1, 0xC1, 0x09, 0x23, 0x33, 0x52, 0xF0,
+	0x15, 0x62, 0x72, 0xD1, 0x0A, 0x16, 0x24, 0x34, 0xE1, 0x25, 0xF1, 0x17,
+	0x18, 0x19, 0x1A, 0x26, 0x27, 0x28, 0x29, 0x2A, 0x35, 0x36, 0x37, 0x38,
+	0x39, 0x3A, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 0x49, 0x4A, 0x53, 0x54,
+	0x55, 0x56, 0x57, 0x58, 0x59, 0x5A, 0x63, 0x64, 0x65, 0x66, 0x67, 0x68,
+	0x69, 0x6A, 0x73, 0x74, 0x75, 0x76, 0x77, 0x78, 0x79, 0x7A, 0x82, 0x83,
+	0x84, 0x85, 0x86, 0x87, 0x88, 0x89, 0x8A, 0x92, 0x93, 0x94, 0x95, 0x96,
+	0x97, 0x98, 0x99, 0x9A, 0xA2, 0xA3, 0xA4, 0xA5, 0xA6, 0xA7, 0xA8, 0xA9,
+	0xAA, 0xB2, 0xB3, 0xB4, 0xB5, 0xB6, 0xB7, 0xB8, 0xB9, 0xBA, 0xC2, 0xC3,
+	0xC4, 0xC5, 0xC6, 0xC7, 0xC8, 0xC9, 0xCA, 0xD2, 0xD3, 0xD4, 0xD5, 0xD6,
+	0xD7, 0xD8, 0xD9, 0xDA, 0xE2, 0xE3, 0xE4, 0xE5, 0xE6, 0xE7, 0xE8, 0xE9,
+	0xEA, 0xF2, 0xF3, 0xF4, 0xF5, 0xF6, 0xF7, 0xF8, 0xF9, 0xFA
+};
+
 /**
  * struct jpeg_stream - JPEG byte stream
  * @curr: current position in stream
@@ -675,3 +784,54 @@  int v4l2_jpeg_parse_huffman_tables(void *buf, size_t len,
 	return jpeg_parse_huffman_tables(&stream, huffman_tables);
 }
 EXPORT_SYMBOL_GPL(v4l2_jpeg_parse_huffman_tables);
+
+/**
+ * v4l2_jpeg_get_reference_quantization_tables - Get reference quantization
+ *						 tables as defined in ITU-T.81
+ * @ref_luma_qt: Output variable pointing to luma quantization table
+ * @ref_chroma_qt: Output variable pointing to chroma quantization table
+ */
+void v4l2_jpeg_get_reference_quantization_tables(const u8 **ref_luma_qt, const
+						 u8 **ref_chroma_qt)
+{
+	if (ref_luma_qt)
+		*ref_luma_qt = luma_qt;
+	if (ref_chroma_qt)
+		*ref_chroma_qt = chroma_qt;
+}
+EXPORT_SYMBOL_GPL(v4l2_jpeg_get_reference_quantization_tables);
+
+/**
+ * v4l2_jpeg_get_zig_zag_scan - Get zigzag scan table as defined in ITU-T.81
+ * @ref_zigzag: Output variable pointing to zigzag scan table
+ */
+void v4l2_jpeg_get_zig_zag_scan(const u8 **ref_zigzag)
+{
+	if (ref_zigzag)
+		*ref_zigzag = zigzag;
+}
+EXPORT_SYMBOL_GPL(v4l2_jpeg_get_zig_zag_scan);
+
+/**
+ * v4l2_jpeg_get_reference_huffman_tables - Get reference huffman tables as
+ *					    defined in ITU-T.81
+ * @ref_luma_dc_ht : Output variable pointing to huffman table for luma DC
+ * @ref_luma_ac_ht : Output variable pointing to huffman table for luma AC
+ * @ref_chroma_dc_ht : Output variable pointing to huffman table for chroma DC
+ * @ref_chroma_ac_ht : Output variable pointing to huffman table for chroma AC
+ */
+void v4l2_jpeg_get_reference_huffman_tables(const u8 **ref_luma_dc_ht,
+					    const u8 **ref_luma_ac_ht,
+					    const u8 **ref_chroma_dc_ht,
+					    const u8 **ref_chroma_ac_ht)
+{
+	if (ref_luma_dc_ht)
+		*ref_luma_dc_ht = luma_dc_ht;
+	if (ref_luma_ac_ht)
+		*ref_luma_ac_ht = luma_ac_ht;
+	if (ref_chroma_dc_ht)
+		*ref_chroma_dc_ht = chroma_dc_ht;
+	if (ref_chroma_ac_ht)
+		*ref_chroma_ac_ht = chroma_ac_ht;
+}
+EXPORT_SYMBOL_GPL(v4l2_jpeg_get_reference_huffman_tables);
diff --git a/include/media/v4l2-jpeg.h b/include/media/v4l2-jpeg.h
index 2dba843ce3bd..b470bbffb73f 100644
--- a/include/media/v4l2-jpeg.h
+++ b/include/media/v4l2-jpeg.h
@@ -14,6 +14,30 @@ 
 
 #define V4L2_JPEG_MAX_COMPONENTS	4
 #define V4L2_JPEG_MAX_TABLES		4
+/*
+ * Prefixes used to generate huffman table class and destination identifiers as
+ * described below:
+ *
+ * V4L2_JPEG_LUM_HT | V4L2_JPEG_DC_HT : Prefix for Luma DC coefficients
+ *					huffman table
+ * V4L2_JPEG_LUM_HT | V4L2_JPEG_AC_HT : Prefix for Luma AC coefficients
+ *					huffman table
+ * V4L2_JPEG_CHR_HT | V4L2_JPEG_DC_HT : Prefix for Chroma DC coefficients
+ *					huffman table
+ * V4L2_JPEG_CHR_HT | V4L2_JPEG_AC_HT : Prefix for Chroma AC coefficients
+ *					huffman table
+ */
+#define V4L2_JPEG_LUM_HT		0x00
+#define V4L2_JPEG_CHR_HT		0x01
+#define V4L2_JPEG_DC_HT			0x00
+#define V4L2_JPEG_AC_HT			0x10
+
+/* Length of reference huffman tables as provided in Table K.3 of ITU-T.81 */
+#define V4L2_JPEG_REF_HT_AC_LEN		178
+#define V4L2_JPEG_REF_HT_DC_LEN		28
+
+/* Array size for 8x8 block of samples or DCT coefficient */
+#define V4L2_JPEG_PIXELS_IN_BLOCK	64
 
 /**
  * struct v4l2_jpeg_reference - reference into the JPEG buffer
@@ -154,4 +178,8 @@  int v4l2_jpeg_parse_quantization_tables(void *buf, size_t len, u8 precision,
 int v4l2_jpeg_parse_huffman_tables(void *buf, size_t len,
 				   struct v4l2_jpeg_reference *huffman_tables);
 
+void v4l2_jpeg_get_reference_quantization_tables(const u8 **luma_qt, const u8 **chroma_qt);
+void v4l2_jpeg_get_zig_zag_scan(const u8 **zigzag);
+void v4l2_jpeg_get_reference_huffman_tables(const u8 **luma_dc_ht, const u8 **luma_ac_ht,
+					    const u8 **chroma_dc_ht, const u8 **chroma_ac_ht);
 #endif