| Message ID | 20191213160428.54303-2-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: cedrus: hevc: Add support for scaling matrix and multi-slice frames | expand |
Em Fri, 13 Dec 2019 17:04:25 +0100 Jernej Skrabec <jernej.skrabec@siol.net> escreveu: > HEVC has a scaling matrix concept. Add support for it. > +struct v4l2_ctrl_hevc_scaling_matrix { > + __u8 scaling_list_4x4[6][16]; > + __u8 scaling_list_8x8[6][64]; > + __u8 scaling_list_16x16[6][64]; > + __u8 scaling_list_32x32[2][64]; > + __u8 scaling_list_dc_coef_16x16[6]; > + __u8 scaling_list_dc_coef_32x32[2]; > +}; I never looked at HEVC spec, but the above seems really weird. Please correct me if I am wrong, but each of the above matrixes is independent, and the driver will use just one of the above on any specific time (for a given video output node), right? If so, why would userspace be forced to update lots of matrixes, if would likely use just one at a given time? IMO, the proper way would be, instead, to use an uAPI like: /* * Actually, as this is uAPI, we will use a fixed size integer type, like * unsigned int */ enum hevc_scaling_matrix_type { HEVC_SCALING_MATRIX_4x4, HEVC_SCALING_MATRIX_8x8, ... HEVC_SCALING_MATRIX_DC_COEF_32x32, }; struct v4l2_ctrl_hevc_scaling_matrix { __u32 scaling_type /* as defined by enum hevc_scaling_matrix_type */ union { __u8 scaling_list_4x4[6][16]; __u8 scaling_list_8x8[6][64]; __u8 scaling_list_16x16[6][64]; __u8 scaling_list_32x32[2][64]; __u8 scaling_list_dc_coef_16x16[6]; __u8 scaling_list_dc_coef_32x32[2]; }; }; And let the core use a default for each scaling matrix, if userspace doesn't set it. Cheers, Mauro
Hi Mauro, On Wed 08 Jan 20, 15:11, Mauro Carvalho Chehab wrote: > Em Fri, 13 Dec 2019 17:04:25 +0100 > Jernej Skrabec <jernej.skrabec@siol.net> escreveu: > > > HEVC has a scaling matrix concept. Add support for it. > > > +struct v4l2_ctrl_hevc_scaling_matrix { > > + __u8 scaling_list_4x4[6][16]; > > + __u8 scaling_list_8x8[6][64]; > > + __u8 scaling_list_16x16[6][64]; > > + __u8 scaling_list_32x32[2][64]; > > + __u8 scaling_list_dc_coef_16x16[6]; > > + __u8 scaling_list_dc_coef_32x32[2]; > > +}; > > I never looked at HEVC spec, but the above seems really weird. > > Please correct me if I am wrong, but each of the above matrixes > is independent, and the driver will use just one of the above on > any specific time (for a given video output node), right? I am not too sure about what the specification really entails, but it is my understanding that HEVC allows simultaneous block sizes between 4x4 and 32x32 to exist within the same coding tree and slice. That suggests that it makes sense to have specific coefficients for each case. Note that the hardware also has distinct registers for each scaling list. Cheers, Paul > If so, why would userspace be forced to update lots of matrixes, if would > likely use just one at a given time? > > IMO, the proper way would be, instead, to use an uAPI like: > > /* > * Actually, as this is uAPI, we will use a fixed size integer type, like > * unsigned int > */ > enum hevc_scaling_matrix_type { > HEVC_SCALING_MATRIX_4x4, > HEVC_SCALING_MATRIX_8x8, > ... > HEVC_SCALING_MATRIX_DC_COEF_32x32, > }; > > struct v4l2_ctrl_hevc_scaling_matrix { > __u32 scaling_type /* as defined by enum hevc_scaling_matrix_type */ > > union { > __u8 scaling_list_4x4[6][16]; > __u8 scaling_list_8x8[6][64]; > __u8 scaling_list_16x16[6][64]; > __u8 scaling_list_32x32[2][64]; > __u8 scaling_list_dc_coef_16x16[6]; > __u8 scaling_list_dc_coef_32x32[2]; > }; > }; > > And let the core use a default for each scaling matrix, if userspace doesn't > set it. > > > > Cheers, > Mauro
Hi! Dne sreda, 08. januar 2020 ob 15:43:36 CET je Paul Kocialkowski napisal(a): > Hi Mauro, > > On Wed 08 Jan 20, 15:11, Mauro Carvalho Chehab wrote: > > Em Fri, 13 Dec 2019 17:04:25 +0100 > > > > Jernej Skrabec <jernej.skrabec@siol.net> escreveu: > > > HEVC has a scaling matrix concept. Add support for it. > > > > > > +struct v4l2_ctrl_hevc_scaling_matrix { > > > + __u8 scaling_list_4x4[6][16]; > > > + __u8 scaling_list_8x8[6][64]; > > > + __u8 scaling_list_16x16[6][64]; > > > + __u8 scaling_list_32x32[2][64]; > > > + __u8 scaling_list_dc_coef_16x16[6]; > > > + __u8 scaling_list_dc_coef_32x32[2]; > > > +}; > > > > I never looked at HEVC spec, but the above seems really weird. > > > > Please correct me if I am wrong, but each of the above matrixes > > is independent, and the driver will use just one of the above on > > any specific time (for a given video output node), right? > > I am not too sure about what the specification really entails, but it is my > understanding that HEVC allows simultaneous block sizes between 4x4 and > 32x32 to exist within the same coding tree and slice. That suggests that it > makes sense to have specific coefficients for each case. Specs ITU-T REC. H.265 (06/2019), chapter 7.3.4 shows that multiple different matrices can be present at the same time. If they are not, default values should be used instead. But in general, more than one can be needed at the same time. Only real question is if default values should be also provided by userspace or by kernel. Since place has to be reserved for all different scaling lists anyway, we won't save any space by providing default values in kernel. Cedrus VPU has only bit switch for using default values for all matrices at the same time or all custom. Note that this control contains slightly processed data. Frame has stored these matrices in form of deltas. But because this is the only driver that use this structure I have no idea what is the most proper form of this data (raw values or deltas). That's why this will stay in staging using private headers until we figure this out. Best regards, Jernej > > Note that the hardware also has distinct registers for each scaling list. > > Cheers, > > Paul > > > If so, why would userspace be forced to update lots of matrixes, if would > > likely use just one at a given time? > > > > IMO, the proper way would be, instead, to use an uAPI like: > > > > /* > > > > * Actually, as this is uAPI, we will use a fixed size integer type, like > > * unsigned int > > */ > > > > enum hevc_scaling_matrix_type { > > > > HEVC_SCALING_MATRIX_4x4, > > HEVC_SCALING_MATRIX_8x8, > > > > ... > > > > HEVC_SCALING_MATRIX_DC_COEF_32x32, > > > > }; > > > > struct v4l2_ctrl_hevc_scaling_matrix { > > > > __u32 scaling_type /* as defined by enum hevc_scaling_matrix_type */ > > > > union { > > > > __u8 scaling_list_4x4[6][16]; > > __u8 scaling_list_8x8[6][64]; > > __u8 scaling_list_16x16[6][64]; > > __u8 scaling_list_32x32[2][64]; > > __u8 scaling_list_dc_coef_16x16[6]; > > __u8 scaling_list_dc_coef_32x32[2]; > > > > }; > > > > }; > > > > And let the core use a default for each scaling matrix, if userspace > > doesn't set it. > > > > > > > > Cheers, > > Mauro
On 1/9/20 4:17 PM, Jernej Škrabec wrote: > Hi! > > Dne sreda, 08. januar 2020 ob 15:43:36 CET je Paul Kocialkowski napisal(a): >> Hi Mauro, >> >> On Wed 08 Jan 20, 15:11, Mauro Carvalho Chehab wrote: >>> Em Fri, 13 Dec 2019 17:04:25 +0100 >>> >>> Jernej Skrabec <jernej.skrabec@siol.net> escreveu: >>>> HEVC has a scaling matrix concept. Add support for it. >>>> >>>> +struct v4l2_ctrl_hevc_scaling_matrix { >>>> + __u8 scaling_list_4x4[6][16]; >>>> + __u8 scaling_list_8x8[6][64]; >>>> + __u8 scaling_list_16x16[6][64]; >>>> + __u8 scaling_list_32x32[2][64]; >>>> + __u8 scaling_list_dc_coef_16x16[6]; >>>> + __u8 scaling_list_dc_coef_32x32[2]; >>>> +}; >>> >>> I never looked at HEVC spec, but the above seems really weird. >>> >>> Please correct me if I am wrong, but each of the above matrixes >>> is independent, and the driver will use just one of the above on >>> any specific time (for a given video output node), right? >> >> I am not too sure about what the specification really entails, but it is my >> understanding that HEVC allows simultaneous block sizes between 4x4 and >> 32x32 to exist within the same coding tree and slice. That suggests that it >> makes sense to have specific coefficients for each case. > > Specs ITU-T REC. H.265 (06/2019), chapter 7.3.4 shows that multiple different > matrices can be present at the same time. If they are not, default values > should be used instead. But in general, more than one can be needed at the > same time. > > Only real question is if default values should be also provided by userspace > or by kernel. Since place has to be reserved for all different scaling lists > anyway, we won't save any space by providing default values in kernel. Cedrus > VPU has only bit switch for using default values for all matrices at the same > time or all custom. > > Note that this control contains slightly processed data. Frame has stored > these matrices in form of deltas. But because this is the only driver that use > this structure I have no idea what is the most proper form of this data (raw > values or deltas). That's why this will stay in staging using private headers > until we figure this out. This definitely needs to be documented! Otherwise this will be forgotten. Regards, Hans > > Best regards, > Jernej > >> >> Note that the hardware also has distinct registers for each scaling list. >> >> Cheers, >> >> Paul >> >>> If so, why would userspace be forced to update lots of matrixes, if would >>> likely use just one at a given time? >>> >>> IMO, the proper way would be, instead, to use an uAPI like: >>> >>> /* >>> >>> * Actually, as this is uAPI, we will use a fixed size integer type, like >>> * unsigned int >>> */ >>> >>> enum hevc_scaling_matrix_type { >>> >>> HEVC_SCALING_MATRIX_4x4, >>> HEVC_SCALING_MATRIX_8x8, >>> >>> ... >>> >>> HEVC_SCALING_MATRIX_DC_COEF_32x32, >>> >>> }; >>> >>> struct v4l2_ctrl_hevc_scaling_matrix { >>> >>> __u32 scaling_type /* as defined by enum > hevc_scaling_matrix_type */ >>> >>> union { >>> >>> __u8 scaling_list_4x4[6][16]; >>> __u8 scaling_list_8x8[6][64]; >>> __u8 scaling_list_16x16[6][64]; >>> __u8 scaling_list_32x32[2][64]; >>> __u8 scaling_list_dc_coef_16x16[6]; >>> __u8 scaling_list_dc_coef_32x32[2]; >>> >>> }; >>> >>> }; >>> >>> And let the core use a default for each scaling matrix, if userspace >>> doesn't set it. >>> >>> >>> >>> Cheers, >>> Mauro > > > >
diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst index 28313c0f4e7c..aab1451e54d4 100644 --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst @@ -4180,6 +4180,47 @@ enum v4l2_mpeg_video_hevc_size_of_length_field - - ``padding[6]`` - Applications and drivers must set this to zero. +``V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX (struct)`` + Specifies the scaling matrix (as extracted from the bitstream) for + the associated HEVC slice data. The bitstream parameters are + defined according to :ref:`hevc`, section 7.4.5 "Scaling list + data semantics". For further documentation, refer to the above + specification, unless there is an explicit comment stating + otherwise. + + .. note:: + + This compound control is not yet part of the public kernel API and + it is expected to change. + +.. c:type:: v4l2_ctrl_hevc_scaling_matrix + +.. cssclass:: longtable + +.. flat-table:: struct v4l2_ctrl_hevc_scaling_matrix + :header-rows: 0 + :stub-columns: 0 + :widths: 1 1 2 + + * - __u8 + - ``scaling_list_4x4[6][16]`` + - + * - __u8 + - ``scaling_list_8x8[6][64]`` + - + * - __u8 + - ``scaling_list_16x16[6][64]`` + - + * - __u8 + - ``scaling_list_32x32[2][64]`` + - + * - __u8 + - ``scaling_list_dc_coef_16x16[6]`` + - + * - __u8 + - ``scaling_list_dc_coef_32x32[2]`` + - + ``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE (enum)`` Specifies the decoding mode to use. Currently exposes slice-based and frame-based decoding but new modes might be added later on. diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst b/Documentation/media/uapi/v4l/pixfmt-compressed.rst index 561bda112809..3aabc322daa4 100644 --- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst +++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst @@ -207,6 +207,7 @@ Compressed Formats * ``V4L2_CID_MPEG_VIDEO_HEVC_SPS`` * ``V4L2_CID_MPEG_VIDEO_HEVC_PPS`` * ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS`` + * ``V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX`` See the :ref:`associated Codec Control IDs <v4l2-mpeg-hevc>`. Buffers associated with this pixel format must contain the appropriate number of macroblocks to decode a full corresponding frame. diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c index 2928c5e0a73d..8cc5ef33b8fd 100644 --- a/drivers/media/v4l2-core/v4l2-ctrls.c +++ b/drivers/media/v4l2-core/v4l2-ctrls.c @@ -975,6 +975,7 @@ const char *v4l2_ctrl_get_name(u32 id) case V4L2_CID_MPEG_VIDEO_HEVC_SPS: return "HEVC Sequence Parameter Set"; case V4L2_CID_MPEG_VIDEO_HEVC_PPS: return "HEVC Picture Parameter Set"; case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS: return "HEVC Slice Parameters"; + case V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX: return "HEVC Scaling Matrix"; case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE: return "HEVC Decode Mode"; case V4L2_CID_MPEG_VIDEO_HEVC_START_CODE: return "HEVC Start Code"; @@ -1407,6 +1408,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type, case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS: *type = V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS; break; + case V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX: + *type = V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX; + break; case V4L2_CID_UNIT_CELL_SIZE: *type = V4L2_CTRL_TYPE_AREA; *flags |= V4L2_CTRL_FLAG_READ_ONLY; @@ -1857,6 +1861,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, zero_padding(*p_hevc_slice_params); break; + case V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX: + break; + case V4L2_CTRL_TYPE_AREA: area = p; if (!area->width || !area->height) @@ -2546,6 +2553,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl, case V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS: elem_size = sizeof(struct v4l2_ctrl_hevc_slice_params); break; + case V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX: + elem_size = sizeof(struct v4l2_ctrl_hevc_scaling_matrix); + break; case V4L2_CTRL_TYPE_AREA: elem_size = sizeof(struct v4l2_area); break; diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h index 1009cf0891cc..1592e52c3614 100644 --- a/include/media/hevc-ctrls.h +++ b/include/media/hevc-ctrls.h @@ -19,6 +19,7 @@ #define V4L2_CID_MPEG_VIDEO_HEVC_SPS (V4L2_CID_MPEG_BASE + 1008) #define V4L2_CID_MPEG_VIDEO_HEVC_PPS (V4L2_CID_MPEG_BASE + 1009) #define V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS (V4L2_CID_MPEG_BASE + 1010) +#define V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX (V4L2_CID_MPEG_BASE + 1011) #define V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE (V4L2_CID_MPEG_BASE + 1015) #define V4L2_CID_MPEG_VIDEO_HEVC_START_CODE (V4L2_CID_MPEG_BASE + 1016) @@ -26,6 +27,7 @@ #define V4L2_CTRL_TYPE_HEVC_SPS 0x0120 #define V4L2_CTRL_TYPE_HEVC_PPS 0x0121 #define V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS 0x0122 +#define V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX 0x0123 enum v4l2_mpeg_video_hevc_decode_mode { V4L2_MPEG_VIDEO_HEVC_DECODE_MODE_SLICE_BASED, @@ -209,4 +211,13 @@ struct v4l2_ctrl_hevc_slice_params { __u64 flags; }; +struct v4l2_ctrl_hevc_scaling_matrix { + __u8 scaling_list_4x4[6][16]; + __u8 scaling_list_8x8[6][64]; + __u8 scaling_list_16x16[6][64]; + __u8 scaling_list_32x32[2][64]; + __u8 scaling_list_dc_coef_16x16[6]; + __u8 scaling_list_dc_coef_32x32[2]; +}; + #endif
HEVC has a scaling matrix concept. Add support for it. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- .../media/uapi/v4l/ext-ctrls-codec.rst | 41 +++++++++++++++++++ .../media/uapi/v4l/pixfmt-compressed.rst | 1 + drivers/media/v4l2-core/v4l2-ctrls.c | 10 +++++ include/media/hevc-ctrls.h | 11 +++++ 4 files changed, 63 insertions(+)