From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1271C3128C8; Mon, 27 Oct 2025 15:54:46 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1761580487; cv=none; b=DFuZ5KpkwyVFX3MWYdsN7u3c+6sAMEQ+yam9on18fkdtDTTpG2yfPXRqHevZIl/UdKus3w7opwdyIEt/qzBHLuQvTFJ2iGIYEpJXG1w2/D6lsJEYF4wY1WJ2rqDG8DQOXAlEzsbI6RKvMr5DdZr0RlEXn6oHYN65kemNLDn+15s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1761580487; c=relaxed/simple; bh=PC/Key79mxSwwFYItT5YNmVyruW8yPwSdYrSrBB7DY4=; h=Message-ID:Date:MIME-Version:From:Subject:To:Cc:References: In-Reply-To:Content-Type; b=Jfy+7KMV8GBNw3A/TdHPG2Wb/eQKz8im+QEld1+/5so5X0i6Fpvi/LVgm/Oyr2KsPR/PsY/dywvflp0Y8KZjafHtsvve2ehK4Yco+ypEpt98Fy9gTrG+OSSckP3zs5ilaZ5vSAuYWxTHl3k5RsABilWbqZxaueG7G75rWs9yFxA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ApZqaUo9; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ApZqaUo9" Received: by smtp.kernel.org (Postfix) with ESMTPSA id DFDB9C4CEF1; Mon, 27 Oct 2025 15:54:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1761580486; bh=PC/Key79mxSwwFYItT5YNmVyruW8yPwSdYrSrBB7DY4=; h=Date:From:Subject:To:Cc:References:In-Reply-To:From; b=ApZqaUo9L25GA8s/FDGqyMbTH+5YIOXSOAWerWPYho5MTuMWFcQ7Urg8Io97zW13m JtBn30Lt/3wYf98gTdzaYPzfHfNHc8vSk8JCUxTAXEN3FVI09cyrVjfq0cqE+TXDLD BfDq6rxsy+a/FCQPbTuOVbZ327TFMNP+xXf24B2dm2o3/s0RASuU4g5cxslJO7sGhc pf/nDp/iETMB51U1AbsSUxKwPwNSBJ7sf65YB01Kr+QzqNB76fhEaKlc4PloJjicuw PfUgYzJmKrzrjX8Eha7cLmM9iegdNPMmqqkLQFlMHy3rIad7ZN2n+hZkrjbpoYegUM e/wEABfWy2cVw== Message-ID: <3a565e1b-4d73-4d7d-a6bd-1dd8b7b973b3@kernel.org> Date: Mon, 27 Oct 2025 16:54:42 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Hans Verkuil Subject: Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence() To: opensource india Cc: jc@kynesim.co.uk, mchehab@kernel.org, hverkuil@kernel.org, ribalda@chromium.org, laurent.pinchart@ideasonboard.com, yunkec@google.com, sakari.ailus@linux.intel.com, james.cowgill@blaize.com, Nicolas Dufresne , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org References: <20250913105252.26886-1-opensource206@gmail.com> <8199bec4-b9e1-4d6e-98da-a4d7eb667437@kernel.org> <7fc65c85-f75e-419c-aa1b-0c85376373d4@kernel.org> Content-Language: en-US, nl In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 27/10/2025 16:36, opensource india wrote: > > > On Thu, Oct 23, 2025 at 6:44 PM Hans Verkuil > wrote: >> >> On 23/10/2025 15:03, John Cox wrote: >> > On Thu, 23 Oct 2025 at 11:44, Hans Verkuil > wrote: >> >> >> >> On 23/10/2025 12:32, opensource india wrote: >> >>> On Wed, Oct 22, 2025 at 12:44 PM Hans Verkuil > wrote: >> >>>> >> >>>> Hi Pavan, >> >>>> >> >>>> On 13/09/2025 12:52, Pavan Bobba wrote: >> >>>>> Complete the "TODO: PROFILES" by enforcing profile-specific and >> >>>>> monochrome constraints as defined by the AV1 specification >> >>>>> (Section 5.5.2, "Color config syntax"). >> >>>>> >> >>>>> The validator now checks: >> >>>>> >> >>>>>  - Flags: reject any unknown bits set in sequence->flags >> >>>>>  - Profile range: only profiles 0..2 are valid >> >>>>>  - Profile 0: 8/10-bit only, subsampling must be 4:2:0 (sx=1, sy=1), >> >>>>>    monochrome allowed >> >>>>>  - Profile 1: 8/10-bit only, subsampling must be 4:4:4 (sx=0, sy=0), >> >>>>>    monochrome forbidden >> >>>>>  - Profile 2: >> >>>>>     * 8/10-bit: only 4:2:2 allowed (sx=1, sy=0) >> >>>>>     * 12-bit: 4:4:4 (sx=0, sy=0), 4:2:2 (sx=1, sy=0), or 4:2:0 (sx=1, sy=1) >> >>>>>       allowed >> >>>>>  - Monochrome path (all profiles except 1): forces subsampling_x=1, >> >>>>>    subsampling_y=1, separate_uv_delta_q=0 >> >>>>> >> >>>>> These checks prevent userspace from providing invalid AV1 sequence >> >>>>> headers that would otherwise be accepted, leading to undefined driver >> >>>>> or hardware behavior. >> >>>> >> >>>> This patch was merged in our media-committers next branch, but I noticed that >> >>>> it now fails the v4l2-compliance test for the visl driver. >> >>>> >> >>>> The cause is that the new validation now fails with the default values for >> >>>> this control as set in std_init_compound(). >> >>>> >> >>>> You can test this yourself by loading the visl driver and then running >> >>>> v4l2-compliance -d /dev/videoX -E --verbose >> >>>> (-E stops at the first error) >> >>>> >> >>>> Can you provide a patch to initialize this control with sane values? >> >>>> >> >>>> Apologies for not noticing this before: there are some issues with the automatic >> >>>> regression tests in our CI, so the tests weren't run. >> >>>> >> >>>> Regards, >> >>>> >> >>>>         Hans >> >>>> >> >>> >> >>> Hi Hans Verkuil, >> >>> >> >>> Thank you so much for the review. >> >>> yes, v4l2-compliance expected to fail indeed since it is sending >> >>> default values which, our newly added code rejects as per >> >>> specification >> >>> >> >>> when you say patch, you mean patch for v4l2-compliance tool with >> >>> proper values so that v4l2 core driver can accept? >> >> >> >> No, std_init_compound() in the kernel needs to be patched so the initial >> >> value of this control passes the new validation tests. The initial control >> >> values should always be sane. >> > >> > Whilst that is a good principle it makes almost no sense in this >> > context. There is almost no chance that a given bitstream will decode >> > against a default sequence header and failing to set it explicitly is >> > going to be a mistake on the users part. It seems to me that it is >> > better to have something that is detectable as unset rather than >> > something that is valid but wrong. >> > >> > I accept that it is the V4L2 way to require "valid" default values for >> > all supported ctrls, but it seems to me to be actively unhelpful for >> > things like SPS / VPS / Tile Group Entry where if not set correctly >> > from bits of the bitstream that the kernel doesn't get to see they >> > will break the stream decode. >> >> I agree, but the V4L2 design (not just controls, but also formats etc.) is >> that they have valid values, even if it makes no sense in the bigger picture. >> >> Now, is that the right design or not? You could argue either way, but the >> fact is that that's how it was designed many years ago. >> >> Changing this for just a single control is worse than just initing with the >> minimum you can get away with. Bonus points if it is somewhat sane :-) >> >> The advantage of always reporting valid values is that the application never >> has to explicitly check if the format/control/etc. has invalid values. >> >> Regards, >> >>         Hans >> >> > >> > I'm not going to argue this point but I felt that I wanted to make it. >> > >> > Regards >> > >> > John Cox >> >> Regards, >> >> >> >>         Hans >> >> >> > >> > Thank you Hans and John. > > actually i have tested with v4l2-compliance tool of version - 1.31 Always compile v4l2-compliance from the git repo https://git.linuxtv.org/v4l-utils.git/ 1.31 is old, and it is important to test with the latest version since it will be kept in sync with the head of the media-committers git repo. > > i can see below log > > info: checking v4l2_query_ext_ctrl of control 'HEVC Decode Mode' (0x00a40a95) > info: checking v4l2_query_ext_ctrl of control 'HEVC Start Code' (0x00a40a96) > info: checking v4l2_query_ext_ctrl of control 'HEVC Entry Point Offsets' (0x00a40a97) > info: checking v4l2_query_ext_ctrl of control 'AV1 Sequence Parameters' (0x00a40af4) > info: checking v4l2_query_ext_ctrl of control 'AV1 Tile Group Entry' (0x00a40af5) > info: checking v4l2_query_ext_ctrl of control 'AV1 Frame Parameters' (0x00a40af6) > info: checking v4l2_query_ext_ctrl of control 'AV1 Film Grain' (0x00a40af9) > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK > test VIDIOC_QUERYCTRL: OK > info: checking control 'Stateless Codec Controls' (0x00a40001) > info: checking control 'H264 Decode Mode' (0x00a40900) > info: checking control 'H264 Start Code' (0x00a40901) > info: checking control 'HEVC Decode Mode' (0x00a40a95) > info: checking control 'HEVC Start Code' (0x00a40a96) > test VIDIOC_G/S_CTRL: OK > info: checking extended control 'Stateless Codec Controls' (0x00a40001) > info: VIDIOC_TRY_EXT_CTRLS1 on node: > *fail: v4l2-test-controls.cpp(971): invalid error index read only control* > > AV1 Sequence Parameters test is passing(this is where api has our patch invoked). > > std_init_compound() is assigning profile 0 with bit depth 8, which is acceptable as per specification. Testing with visl and v4l2-compliance -E --verbose gives me: test VIDIOC_G/S_CTRL: OK info: checking extended control 'Stateless Codec Controls' (0x00a40001) info: checking extended control 'H264 Decode Mode' (0x00a40900) info: checking extended control 'H264 Start Code' (0x00a40901) info: checking extended control 'H264 Sequence Parameter Set' (0x00a40902) info: checking extended control 'H264 Picture Parameter Set' (0x00a40903) info: checking extended control 'H264 Scaling Matrix' (0x00a40904) info: checking extended control 'H264 Prediction Weight Table' (0x00a40905) info: checking extended control 'H264 Slice Parameters' (0x00a40906) info: checking extended control 'H264 Decode Parameters' (0x00a40907) info: checking extended control 'FWHT Stateless Parameters' (0x00a40964) info: checking extended control 'VP8 Frame Parameters' (0x00a409c8) info: checking extended control 'MPEG-2 Sequence Header' (0x00a409dc) info: checking extended control 'MPEG-2 Picture Header' (0x00a409dd) info: checking extended control 'MPEG-2 Quantisation Matrices' (0x00a409de) info: checking extended control 'VP9 Frame Decode Parameters' (0x00a40a2c) info: checking extended control 'VP9 Probabilities Updates' (0x00a40a2d) info: checking extended control 'HEVC Sequence Parameter Set' (0x00a40a90) info: checking extended control 'HEVC Picture Parameter Set' (0x00a40a91) info: checking extended control 'HEVC Slice Parameters' (0x00a40a92) info: checking extended control 'HEVC Scaling Matrix' (0x00a40a93) info: checking extended control 'HEVC Decode Parameters' (0x00a40a94) info: checking extended control 'HEVC Decode Mode' (0x00a40a95) info: checking extended control 'HEVC Start Code' (0x00a40a96) info: checking extended control 'HEVC Entry Point Offsets' (0x00a40a97) info: checking extended control 'AV1 Sequence Parameters' (0x00a40af4) fail: v4l2-test-controls.cpp(939): try_ext_ctrls returned an error (22) Debugging a bit in validate_av1_sequence() shows it fails here: /* 4. Profile-specific rules */ switch (s->seq_profile) { case 0: /* Profile 0: only 8/10-bit, subsampling=4:2:0 (sx=1, sy=1) */ if (s->bit_depth != 8 && s->bit_depth != 10) return -EINVAL; if (!(sx && sy)) return -EINVAL; ^^^^^^^^^^^^^^^^ This is the test that fails the validation break; Regards, Hans