Re: [PATCH v2 2/4] media: rockchip: Introduce the rkvdec2 driver

[Jianfeng Liu <liujianfeng1994@gmail.com>]

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