Hi Detlev, On Thu, 20 Jun 2024 10:07:41 -0400, Detlev Casanova wrote: >This feels like hacking the driver to please a specific userspace application, >so I'd like to understand better what chromium is doing. Yes that hack is ugly. I have added log print in chromium to see if they have set frame_mbs_only_flag to zero and found nothing. This sps->flags is initialized 0 by kernel's v4l2 code. I printed all sps values in function rkvdec2_h264_validate_sps and they are all initial values when chromiumn call VIDIOC_STREAMON at the first time. Hi Nicolas, On Thu, 20 Jun 2024 11:03:54 -0400, Nicolas Dufresne wrote: >This falls short of a specification to support the uninitialized usage by >Chromium. That being said, we do make an effort to try and have a valid default >SPS control and OUTPUT format combination. So my suggestion would be to set >V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY in the default compound control init. This >way, 0x0 get translate to 16x16 instead of 16x32, thus will work with more >drivers. Yeah that's the root cause. Vaule of sps->flags is initialized to 0 along with pic_width_in_mbs_minus1 and pic_height_in_map_units_minus1, so this check would fall with minimal decoder size 16x16. >Chromium these days is not being tested on anything else then MTK, which has a >64x64 minimum size, this is why they never get into this issue. This driver >validation is entirely correct. Removing in some cases is unsafe, it would need >to be replaced with STREAMON only validation of the CAPTURE sizes (which >currently is validate by implied propagation of valid SPS/OUTPUT). > >**not even compiled tested, just to illustrate** > >diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2- >core/v4l2-ctrls-core.c >index c4d995f32191..a55e165ea9c3 100644 >--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c >+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c >@@ -111,6 +111,7 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, >u32 idx, > struct v4l2_ctrl_vp9_frame *p_vp9_frame; > struct v4l2_ctrl_fwht_params *p_fwht_params; > struct v4l2_ctrl_h264_scaling_matrix *p_h264_scaling_matrix; >+ struct v4l2_ctrl_h264_sps *p_h264_sps; > struct v4l2_ctrl_av1_sequence *p_av1_sequence; > void *p = ptr.p + idx * ctrl->elem_size; > >@@ -179,6 +180,18 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, >u32 idx, > */ > memset(p_h264_scaling_matrix, 16, >sizeof(*p_h264_scaling_matrix)); > break; >+ case V4L2_CTRL_TYPE_H264_SPS: >+ p_h264_sps = p; >+ /* >+ * Without V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY, >+ * frame_mbs_only_flag set to 0 will translate to a miniumum >+ * height of 32 (see H.264 specification 7-8). Some driver may >+ * have a minimum size lower then 32, which would fail >+ * validation with the SPS value. Set this flag, so that there >+ * is now doubling in the height, allowing a valid default. >+ */ >+ p_h264_sps->flags = V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY; >+ break; > } > } > >Nicolas This patch makes sense and I just confirmed that it works with chromium. Thank you very much! Best regards, Jianfeng
