Message-ID: <3a565e1b-4d73-4d7d-a6bd-1dd8b7b973b3@kernel.org>
Date: Mon, 27 Oct 2025 16:54:42 +0100
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: opensource india <opensource206@...il.com>
Cc: jc@...esim.co.uk, mchehab@...nel.org, hverkuil@...nel.org,
 ribalda@...omium.org, laurent.pinchart@...asonboard.com, yunkec@...gle.com,
 sakari.ailus@...ux.intel.com, james.cowgill@...ize.com,
 Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in
 validate_av1_sequence()

On 27/10/2025 16:36, opensource india wrote:
> 
> 
> On Thu, Oct 23, 2025 at 6:44 PM Hans Verkuil <hverkuil+cisco@...nel.org <mailto:hverkuil%2Bcisco@...nel.org>> wrote:
>>
>> On 23/10/2025 15:03, John Cox wrote:
>> > On Thu, 23 Oct 2025 at 11:44, Hans Verkuil <hverkuil+cisco@...nel.org <mailto:hverkuil%2Bcisco@...nel.org>> wrote:
>> >>
>> >> On 23/10/2025 12:32, opensource india wrote:
>> >>> On Wed, Oct 22, 2025 at 12:44 PM Hans Verkuil <hverkuil+cisco@...nel.org <mailto:hverkuil%2Bcisco@...nel.org>> 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

Powered by blists - more mailing lists