[PATCH 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()

[Pavan Bobba]

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()

[opensource india]

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()

[Daniel Almeida]

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()

[opensource india]

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()

[Daniel Almeida]

> 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()

[opensource india]

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?

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

[Hans Verkuil]

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()

[opensource india]

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()

[Hans Verkuil]

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()

[John Cox]

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()

[Hans Verkuil]

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()

[Hans Verkuil]

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

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

[opensource india]

Hi Hans,
On Mon, Oct 27, 2025 at 9:24 PM Hans Verkuil <hverkuil+cisco@kernel.org> wrote:
>
> 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.

Apologies. i upgraded my v4l-utils repo to recent(version 1.33) and i
can see the error now

>
> 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

yes. subsampling flags are set to 0 , which are not allowed as per our code.
tested v4l2-compliance successfully with below fix. please review

https://lore.kernel.org/linux-media/20251028062623.12700-1-opensource206@gmail.com/T/#u

Regards,
Pavan