[v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

[PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by Pavan Bobba 4 months, 3 weeks ago

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.

Signed-off-by: Pavan Bobba <opensource206@gmail.com>
---
v1 -> v2 : Added more checks for subsampling combinations per profile.
         : Added a TODO note in the function header for checks to be implemented later.

v2 -> v3 : Patch generated properly with all the changes

 drivers/media/v4l2-core/v4l2-ctrls-core.c | 125 +++++++++++++++++-----
 1 file changed, 100 insertions(+), 25 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index 98b960775e87..fa03341588e4 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -827,39 +827,114 @@ static int validate_av1_frame(struct v4l2_ctrl_av1_frame *f)
 	return 0;
 }
 
+/**
+ * validate_av1_sequence - validate AV1 sequence header fields
+ * @s: control struct from userspace
+ *
+ * Implements AV1 spec §5.5.2 color_config() checks that are
+ * possible with the current v4l2_ctrl_av1_sequence definition.
+ *
+ * TODO: extend validation once additional fields such as
+ *       color_primaries, transfer_characteristics,
+ *       matrix_coefficients, and chroma_sample_position
+ *       are added to the uAPI.
+ *
+ * Returns 0 if valid, -EINVAL otherwise.
+ */
 static int validate_av1_sequence(struct v4l2_ctrl_av1_sequence *s)
 {
-	if (s->flags &
-	~(V4L2_AV1_SEQUENCE_FLAG_STILL_PICTURE |
-	 V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_FILTER_INTRA |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTRA_EDGE_FILTER |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTERINTRA_COMPOUND |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_MASKED_COMPOUND |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_WARPED_MOTION |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_DUAL_FILTER |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_ORDER_HINT |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_JNT_COMP |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_REF_FRAME_MVS |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_SUPERRES |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF |
-	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_RESTORATION |
-	 V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME |
-	 V4L2_AV1_SEQUENCE_FLAG_COLOR_RANGE |
-	 V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X |
-	 V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y |
-	 V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT |
-	 V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q))
-		return -EINVAL;
+	const bool mono  = s->flags & V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME;
+	const bool sx    = s->flags & V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X;
+	const bool sy    = s->flags & V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y;
+	const bool uv_dq = s->flags & V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q;
 
-	if (s->seq_profile == 1 && s->flags & V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME)
+	/* 1. Reject unknown flags */
+	if (s->flags &
+	    ~(V4L2_AV1_SEQUENCE_FLAG_STILL_PICTURE |
+	      V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_FILTER_INTRA |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTRA_EDGE_FILTER |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTERINTRA_COMPOUND |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_MASKED_COMPOUND |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_WARPED_MOTION |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_DUAL_FILTER |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_ORDER_HINT |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_JNT_COMP |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_REF_FRAME_MVS |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_SUPERRES |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF |
+	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_RESTORATION |
+	      V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME |
+	      V4L2_AV1_SEQUENCE_FLAG_COLOR_RANGE |
+	      V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X |
+	      V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y |
+	      V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT |
+	      V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q))
 		return -EINVAL;
 
-	/* reserved */
+	/* 2. Profile range */
 	if (s->seq_profile > 2)
 		return -EINVAL;
 
-	/* TODO: PROFILES */
+	/* 3. Monochrome shortcut */
+	if (mono) {
+		/* Profile 1 forbids monochrome */
+		if (s->seq_profile == 1)
+			return -EINVAL;
+
+		/* Mono → subsampling must look like 4:0:0: sx=1, sy=1 */
+		if (!sx || !sy)
+			return -EINVAL;
+
+		/* separate_uv_delta_q must be 0 */
+		if (uv_dq)
+			return -EINVAL;
+
+		return 0;
+	}
+
+	/* 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;
+		break;
+
+	case 1:
+		/* Profile 1: only 8/10-bit, subsampling=4:4:4 (sx=0, sy=0) */
+		if (s->bit_depth != 8 && s->bit_depth != 10)
+			return -EINVAL;
+		if (sx || sy)
+			return -EINVAL;
+		break;
+
+	case 2:
+		/* Profile 2: 8/10/12-bit allowed */
+		if (s->bit_depth != 8 && s->bit_depth != 10 &&
+		    s->bit_depth != 12)
+			return -EINVAL;
+
+		if (s->bit_depth == 12) {
+			if (!sx) {
+				/* 4:4:4 → sy must be 0 */
+				if (sy)
+					return -EINVAL;
+			} else {
+				/* sx=1 → sy=0 (4:2:2) or sy=1 (4:2:0) */
+				if (sy != 0 && sy != 1)
+					return -EINVAL;
+			}
+		} else {
+			/* 8/10-bit → only 4:2:2 allowed (sx=1, sy=0) */
+			if (!(sx && !sy))
+				return -EINVAL;
+		}
+		break;
+	}
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by Hans Verkuil 3 months, 2 weeks ago

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

> 
> Signed-off-by: Pavan Bobba <opensource206@gmail.com>
> ---
> v1 -> v2 : Added more checks for subsampling combinations per profile.
>          : Added a TODO note in the function header for checks to be implemented later.
> 
> v2 -> v3 : Patch generated properly with all the changes
> 
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 125 +++++++++++++++++-----
>  1 file changed, 100 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index 98b960775e87..fa03341588e4 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -827,39 +827,114 @@ static int validate_av1_frame(struct v4l2_ctrl_av1_frame *f)
>  	return 0;
>  }
>  
> +/**
> + * validate_av1_sequence - validate AV1 sequence header fields
> + * @s: control struct from userspace
> + *
> + * Implements AV1 spec §5.5.2 color_config() checks that are
> + * possible with the current v4l2_ctrl_av1_sequence definition.
> + *
> + * TODO: extend validation once additional fields such as
> + *       color_primaries, transfer_characteristics,
> + *       matrix_coefficients, and chroma_sample_position
> + *       are added to the uAPI.
> + *
> + * Returns 0 if valid, -EINVAL otherwise.
> + */
>  static int validate_av1_sequence(struct v4l2_ctrl_av1_sequence *s)
>  {
> -	if (s->flags &
> -	~(V4L2_AV1_SEQUENCE_FLAG_STILL_PICTURE |
> -	 V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_FILTER_INTRA |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTRA_EDGE_FILTER |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTERINTRA_COMPOUND |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_MASKED_COMPOUND |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_WARPED_MOTION |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_DUAL_FILTER |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_ORDER_HINT |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_JNT_COMP |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_REF_FRAME_MVS |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_SUPERRES |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF |
> -	 V4L2_AV1_SEQUENCE_FLAG_ENABLE_RESTORATION |
> -	 V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME |
> -	 V4L2_AV1_SEQUENCE_FLAG_COLOR_RANGE |
> -	 V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X |
> -	 V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y |
> -	 V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT |
> -	 V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q))
> -		return -EINVAL;
> +	const bool mono  = s->flags & V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME;
> +	const bool sx    = s->flags & V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X;
> +	const bool sy    = s->flags & V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y;
> +	const bool uv_dq = s->flags & V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q;
>  
> -	if (s->seq_profile == 1 && s->flags & V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME)
> +	/* 1. Reject unknown flags */
> +	if (s->flags &
> +	    ~(V4L2_AV1_SEQUENCE_FLAG_STILL_PICTURE |
> +	      V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_FILTER_INTRA |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTRA_EDGE_FILTER |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTERINTRA_COMPOUND |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_MASKED_COMPOUND |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_WARPED_MOTION |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_DUAL_FILTER |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_ORDER_HINT |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_JNT_COMP |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_REF_FRAME_MVS |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_SUPERRES |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF |
> +	      V4L2_AV1_SEQUENCE_FLAG_ENABLE_RESTORATION |
> +	      V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME |
> +	      V4L2_AV1_SEQUENCE_FLAG_COLOR_RANGE |
> +	      V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X |
> +	      V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y |
> +	      V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT |
> +	      V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q))
>  		return -EINVAL;
>  
> -	/* reserved */
> +	/* 2. Profile range */
>  	if (s->seq_profile > 2)
>  		return -EINVAL;
>  
> -	/* TODO: PROFILES */
> +	/* 3. Monochrome shortcut */
> +	if (mono) {
> +		/* Profile 1 forbids monochrome */
> +		if (s->seq_profile == 1)
> +			return -EINVAL;
> +
> +		/* Mono → subsampling must look like 4:0:0: sx=1, sy=1 */
> +		if (!sx || !sy)
> +			return -EINVAL;
> +
> +		/* separate_uv_delta_q must be 0 */
> +		if (uv_dq)
> +			return -EINVAL;
> +
> +		return 0;
> +	}
> +
> +	/* 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;
> +		break;
> +
> +	case 1:
> +		/* Profile 1: only 8/10-bit, subsampling=4:4:4 (sx=0, sy=0) */
> +		if (s->bit_depth != 8 && s->bit_depth != 10)
> +			return -EINVAL;
> +		if (sx || sy)
> +			return -EINVAL;
> +		break;
> +
> +	case 2:
> +		/* Profile 2: 8/10/12-bit allowed */
> +		if (s->bit_depth != 8 && s->bit_depth != 10 &&
> +		    s->bit_depth != 12)
> +			return -EINVAL;
> +
> +		if (s->bit_depth == 12) {
> +			if (!sx) {
> +				/* 4:4:4 → sy must be 0 */
> +				if (sy)
> +					return -EINVAL;
> +			} else {
> +				/* sx=1 → sy=0 (4:2:2) or sy=1 (4:2:0) */
> +				if (sy != 0 && sy != 1)
> +					return -EINVAL;
> +			}
> +		} else {
> +			/* 8/10-bit → only 4:2:2 allowed (sx=1, sy=0) */
> +			if (!(sx && !sy))
> +				return -EINVAL;
> +		}
> +		break;
> +	}
> +
>  	return 0;
>  }
>

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by opensource india 3 months, 2 weeks ago

On Wed, Oct 22, 2025 at 12:44 PM Hans Verkuil <hverkuil+cisco@kernel.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?

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by Hans Verkuil 3 months, 2 weeks ago

On 23/10/2025 12:32, opensource india wrote:
> On Wed, Oct 22, 2025 at 12:44 PM Hans Verkuil <hverkuil+cisco@kernel.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.

Regards,

	Hans

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by John Cox 3 months, 2 weeks ago

On Thu, 23 Oct 2025 at 11:44, Hans Verkuil <hverkuil+cisco@kernel.org> wrote:
>
> On 23/10/2025 12:32, opensource india wrote:
> > On Wed, Oct 22, 2025 at 12:44 PM Hans Verkuil <hverkuil+cisco@kernel.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'm not going to argue this point but I felt that I wanted to make it.

Regards

John Cox
> Regards,
>
>         Hans
>

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by Hans Verkuil 3 months, 2 weeks ago

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

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by opensource india 4 months, 1 week ago

On Sat, Sep 13, 2025 at 4:23 PM Pavan Bobba <opensource206@gmail.com> 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.
>
> Signed-off-by: Pavan Bobba <opensource206@gmail.com>
> ---
> v1 -> v2 : Added more checks for subsampling combinations per profile.
>          : Added a TODO note in the function header for checks to be implemented later.
>
> v2 -> v3 : Patch generated properly with all the changes
>
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 125 +++++++++++++++++-----
>  1 file changed, 100 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index 98b960775e87..fa03341588e4 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -827,39 +827,114 @@ static int validate_av1_frame(struct v4l2_ctrl_av1_frame *f)
>         return 0;
>  }
>
> +/**
> + * validate_av1_sequence - validate AV1 sequence header fields
> + * @s: control struct from userspace
> + *
> + * Implements AV1 spec §5.5.2 color_config() checks that are
> + * possible with the current v4l2_ctrl_av1_sequence definition.
> + *
> + * TODO: extend validation once additional fields such as
> + *       color_primaries, transfer_characteristics,
> + *       matrix_coefficients, and chroma_sample_position
> + *       are added to the uAPI.
> + *
> + * Returns 0 if valid, -EINVAL otherwise.
> + */
>  static int validate_av1_sequence(struct v4l2_ctrl_av1_sequence *s)
>  {
> -       if (s->flags &
> -       ~(V4L2_AV1_SEQUENCE_FLAG_STILL_PICTURE |
> -        V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_FILTER_INTRA |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTRA_EDGE_FILTER |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTERINTRA_COMPOUND |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_MASKED_COMPOUND |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_WARPED_MOTION |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_DUAL_FILTER |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_ORDER_HINT |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_JNT_COMP |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_REF_FRAME_MVS |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_SUPERRES |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF |
> -        V4L2_AV1_SEQUENCE_FLAG_ENABLE_RESTORATION |
> -        V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME |
> -        V4L2_AV1_SEQUENCE_FLAG_COLOR_RANGE |
> -        V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X |
> -        V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y |
> -        V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT |
> -        V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q))
> -               return -EINVAL;
> +       const bool mono  = s->flags & V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME;
> +       const bool sx    = s->flags & V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X;
> +       const bool sy    = s->flags & V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y;
> +       const bool uv_dq = s->flags & V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q;
>
> -       if (s->seq_profile == 1 && s->flags & V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME)
> +       /* 1. Reject unknown flags */
> +       if (s->flags &
> +           ~(V4L2_AV1_SEQUENCE_FLAG_STILL_PICTURE |
> +             V4L2_AV1_SEQUENCE_FLAG_USE_128X128_SUPERBLOCK |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_FILTER_INTRA |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTRA_EDGE_FILTER |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_INTERINTRA_COMPOUND |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_MASKED_COMPOUND |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_WARPED_MOTION |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_DUAL_FILTER |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_ORDER_HINT |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_JNT_COMP |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_REF_FRAME_MVS |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_SUPERRES |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_CDEF |
> +             V4L2_AV1_SEQUENCE_FLAG_ENABLE_RESTORATION |
> +             V4L2_AV1_SEQUENCE_FLAG_MONO_CHROME |
> +             V4L2_AV1_SEQUENCE_FLAG_COLOR_RANGE |
> +             V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_X |
> +             V4L2_AV1_SEQUENCE_FLAG_SUBSAMPLING_Y |
> +             V4L2_AV1_SEQUENCE_FLAG_FILM_GRAIN_PARAMS_PRESENT |
> +             V4L2_AV1_SEQUENCE_FLAG_SEPARATE_UV_DELTA_Q))
>                 return -EINVAL;
>
> -       /* reserved */
> +       /* 2. Profile range */
>         if (s->seq_profile > 2)
>                 return -EINVAL;
>
> -       /* TODO: PROFILES */
> +       /* 3. Monochrome shortcut */
> +       if (mono) {
> +               /* Profile 1 forbids monochrome */
> +               if (s->seq_profile == 1)
> +                       return -EINVAL;
> +
> +               /* Mono → subsampling must look like 4:0:0: sx=1, sy=1 */
> +               if (!sx || !sy)
> +                       return -EINVAL;
> +
> +               /* separate_uv_delta_q must be 0 */
> +               if (uv_dq)
> +                       return -EINVAL;
> +
> +               return 0;
> +       }
> +
> +       /* 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;
> +               break;
> +
> +       case 1:
> +               /* Profile 1: only 8/10-bit, subsampling=4:4:4 (sx=0, sy=0) */
> +               if (s->bit_depth != 8 && s->bit_depth != 10)
> +                       return -EINVAL;
> +               if (sx || sy)
> +                       return -EINVAL;
> +               break;
> +
> +       case 2:
> +               /* Profile 2: 8/10/12-bit allowed */
> +               if (s->bit_depth != 8 && s->bit_depth != 10 &&
> +                   s->bit_depth != 12)
> +                       return -EINVAL;
> +
> +               if (s->bit_depth == 12) {
> +                       if (!sx) {
> +                               /* 4:4:4 → sy must be 0 */
> +                               if (sy)
> +                                       return -EINVAL;
> +                       } else {
> +                               /* sx=1 → sy=0 (4:2:2) or sy=1 (4:2:0) */
> +                               if (sy != 0 && sy != 1)
> +                                       return -EINVAL;
> +                       }
> +               } else {
> +                       /* 8/10-bit → only 4:2:2 allowed (sx=1, sy=0) */
> +                       if (!(sx && !sy))
> +                               return -EINVAL;
> +               }
> +               break;
> +       }
> +
>         return 0;
>  }
>
> --
> 2.43.0
>
Hi all,
  It has been a couple of weeks since I sent this patch. Could anyone
please review it?

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by Daniel Almeida 4 months, 1 week ago

Hi, I’ll review this in the coming weeks.

> On 27 Sep 2025, at 06:51, opensource india <opensource206@gmail.com> wrote:
> 
> On Sat, Sep 13, 2025 at 4:23 PM Pavan Bobba <opensource206@gmail.com> 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.

Mauro,

A reminder that I have been warning about this for quite a while [0], which
includes mentioning that patches like this, although welcome, do not solve the
root issue completely.

I keep working on what I believe to be the solution [1][2]. I would appreciate if
we could restart this discussion.

[0]: https://lwn.net/Articles/970565/
[1]: https://lore.kernel.org/linux-media/20250818-v4l2-v1-0-6887e772aac2@collabora.com/
[2]: https://lwn.net/Articles/963966/

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by opensource india 4 months ago

On Sat, Sep 27, 2025 at 2:27 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi, I’ll review this in the coming weeks.

Hi Daneil, did you get a chance to review this?

> > On 27 Sep 2025, at 06:51, opensource india <opensource206@gmail.com> wrote:
> >
> > On Sat, Sep 13, 2025 at 4:23 PM Pavan Bobba <opensource206@gmail.com> 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.
>
> Mauro,
>
> A reminder that I have been warning about this for quite a while [0], which
> includes mentioning that patches like this, although welcome, do not solve the
> root issue completely.
>
> I keep working on what I believe to be the solution [1][2]. I would appreciate if
> we could restart this discussion.
>
> [0]: https://lwn.net/Articles/970565/
> [1]: https://lore.kernel.org/linux-media/20250818-v4l2-v1-0-6887e772aac2@collabora.com/
> [2]: https://lwn.net/Articles/963966/
>

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by Daniel Almeida 4 months ago


> On 7 Oct 2025, at 07:50, opensource india <opensource206@gmail.com> wrote:
> 
> On Sat, Sep 27, 2025 at 2:27 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>> 
>> Hi, I’ll review this in the coming weeks.
> 
> Hi Daneil, did you get a chance to review this?
> 

Not yet.

— Daniel

Re: [PATCH v3] media: v4l2-ctrls: add full AV1 profile validation in validate_av1_sequence()

Posted by opensource india 3 months, 3 weeks ago

On Tue, Oct 7, 2025 at 7:20 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
>
>
> > On 7 Oct 2025, at 07:50, opensource india <opensource206@gmail.com> wrote:
> >
> > On Sat, Sep 27, 2025 at 2:27 PM Daniel Almeida
> > <daniel.almeida@collabora.com> wrote:
> >>
> >> Hi, I’ll review this in the coming weeks.
> >
> > Hi Daneil, did you get a chance to review this?
> >
>
> Not yet.
>
> — Daniel

Hi Daniel, is it possible for you to review this in the coming couple of weeks?