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@xxxxxx>
> ---
> 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@xxxxxxxxxxxxxx>");
> 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
