Message-ID: <7fc65c85-f75e-419c-aa1b-0c85376373d4@kernel.org>
Date: Thu, 23 Oct 2025 15:14:30 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: jc@...esim.co.uk
Cc: opensource india <opensource206@...il.com>, 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 23/10/2025 15:03, John Cox wrote:
> On Thu, 23 Oct 2025 at 11:44, Hans Verkuil <hverkuil+cisco@...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> 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
>>
> 

Powered by blists - more mailing lists