[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20240620173830.277022-1-liujianfeng1994@gmail.com>
Date: Fri, 21 Jun 2024 01:38:30 +0800
From: Jianfeng Liu <liujianfeng1994@...il.com>
To: nicolas.dufresne@...labora.com
Cc: andy.yan@...k-chips.com,
benjamin.gaignard@...labora.com,
boris.brezillon@...labora.com,
conor+dt@...nel.org,
daniel.almeida@...labora.com,
detlev.casanova@...labora.com,
devicetree@...r.kernel.org,
didi.debian@...ow.org,
dsimic@...jaro.org,
ezequiel@...guardiasur.com.ar,
gregkh@...uxfoundation.org,
heiko@...ech.de,
hverkuil-cisco@...all.nl,
jonas@...boo.se,
knaerzche@...il.com,
krzk+dt@...nel.org,
linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org,
linux-media@...r.kernel.org,
linux-rockchip@...ts.infradead.org,
linux-staging@...ts.linux.dev,
liujianfeng1994@...il.com,
mchehab@...nel.org,
paul.kocialkowski@...tlin.com,
robh@...nel.org,
sebastian.reichel@...labora.com
Subject: Re: [PATCH v2 2/4] media: rockchip: Introduce the rkvdec2 driver
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
Powered by blists - more mailing lists