[v1,22/24] media: hantro: h264: Make dpb entry management more robust

Commit Message

Nicolas Dufresne March 28, 2022, 7:59 p.m. UTC

From: Jonas Karlman <[e-mail]>

The driver maintains stable slot location for reference pictures. This
change makes the code more robust by using the reference_ts as key and
by marking all entries invalid right from the start.

Signed-off-by: Jonas Karlman <[e-mail]>
Signed-off-by: Nicolas Dufresne <[e-mail]>
---
 drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Sebastian Fricke March 30, 2022, 7:59 a.m. UTC | #1

Hey Nicolas,

On 28.03.2022 15:59, Nicolas Dufresne wrote:
>From: Jonas Karlman <[e-mail]>
>
>The driver maintains stable slot location for reference pictures. This

s/slot location/slot locations/

>change makes the code more robust by using the reference_ts as key and
>by marking all entries invalid right from the start.
>
>Signed-off-by: Jonas Karlman <[e-mail]>
>Signed-off-by: Nicolas Dufresne <[e-mail]>
>---
> drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
>index 228629fb3cdf..7377fc26f780 100644
>--- a/drivers/staging/media/hantro/hantro_h264.c
>+++ b/drivers/staging/media/hantro/hantro_h264.c
>@@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> 			    const struct v4l2_h264_dpb_entry *b)
> {
>-	return a->top_field_order_cnt == b->top_field_order_cnt &&
>-	       a->bottom_field_order_cnt == b->bottom_field_order_cnt;
>+	return a->reference_ts == b->reference_ts;
> }
>
> static void update_dpb(struct hantro_ctx *ctx)
>@@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
>
> 	/* Disable all entries by default. */
> 	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
>-		ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
>+		ctx->h264_dec.dpb[i].flags = 0;

Ehm ... we just remove all flags? Can't this have any unwanted side
effects like removing a flag that we actually wanted to keep?
(Like long term or the field flags?)
If we just want to set the DPB entry inactive, then removing the ACTIVE
flag seems like the correct approach to me.
If we want to get rid of the VALID flag as well, then we could just do:
		ctx->h264_dec.dpb[i].flags &=
       ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);

In case we really want to reset all flags, I'd say adjust the comment
above it:
```
- 	/* Disable all entries by default. */
+ 	/* Reset the flags for all entries by default. */
```

Greetings,
Sebastian

>
> 	/* Try to match new DPB entries with existing ones by their POCs. */
> 	for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
> 		const struct v4l2_h264_dpb_entry *ndpb = &dec_param->dpb[i];
>
>-		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
>+		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> 			continue;
>
> 		/*
>@@ -290,8 +289,7 @@ static void update_dpb(struct hantro_ctx *ctx)
> 			struct v4l2_h264_dpb_entry *cdpb;
>
> 			cdpb = &ctx->h264_dec.dpb[j];
>-			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
>-			    !dpb_entry_match(cdpb, ndpb))
>+			if (!dpb_entry_match(cdpb, ndpb))
> 				continue;
>
> 			*cdpb = *ndpb;
>-- 
>2.34.1
>

Nicolas Dufresne March 30, 2022, 3:15 p.m. UTC | #2

Le mercredi 30 mars 2022 à 09:59 +0200, Sebastian Fricke a écrit :
> Hey Nicolas,
> 
> On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > From: Jonas Karlman <[e-mail]>
> > 
> > The driver maintains stable slot location for reference pictures. This
> 
> s/slot location/slot locations/
> 
> > change makes the code more robust by using the reference_ts as key and
> > by marking all entries invalid right from the start.
> > 
> > Signed-off-by: Jonas Karlman <[e-mail]>
> > Signed-off-by: Nicolas Dufresne <[e-mail]>
> > ---
> > drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
> > 1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > index 228629fb3cdf..7377fc26f780 100644
> > --- a/drivers/staging/media/hantro/hantro_h264.c
> > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > @@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> > static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> > 			    const struct v4l2_h264_dpb_entry *b)
> > {
> > -	return a->top_field_order_cnt == b->top_field_order_cnt &&
> > -	       a->bottom_field_order_cnt == b->bottom_field_order_cnt;
> > +	return a->reference_ts == b->reference_ts;
> > }
> > 
> > static void update_dpb(struct hantro_ctx *ctx)
> > @@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
> > 
> > 	/* Disable all entries by default. */
> > 	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
> > -		ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
> > +		ctx->h264_dec.dpb[i].flags = 0;
> 
> Ehm ... we just remove all flags? Can't this have any unwanted side
> effects like removing a flag that we actually wanted to keep?
> (Like long term or the field flags?)

This is a local copy of the dpb, the DPB is fully passed for every decode. So
these flags will be fully restored lower when we copy the found entry. In fact,
holding a state here would not represent well the userland intention and can
have negative side effect on the decoding. Flags are not immutable between
decode and can change. This simplify the code, and make things less error prone.
This part of the code is already a bit complex, no need for an extra layer.

> If we just want to set the DPB entry inactive, then removing the ACTIVE
> flag seems like the correct approach to me.
> If we want to get rid of the VALID flag as well, then we could just do:
> 		ctx->h264_dec.dpb[i].flags &=
>        ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);
> 
> In case we really want to reset all flags, I'd say adjust the comment
> above it:
> ```
> - 	/* Disable all entries by default. */
> + 	/* Reset the flags for all entries by default. */
> ```

This reads the same to me, but I can do that yes. understand that VALID means
the reference exist and the TS should point to some existing past reference
(unless there was some decode error, which the userland may not be aware yet as
this is asynchronous). While ACTIVE means that it is used as a reference. FFMPEG
is known not to filter inactive references. ACTIVE is just a flag without bunch
of other flags that can change for every decode. So none of this make sense
between 2 decodes.

> 
> Greetings,
> Sebastian
> 
> > 
> > 	/* Try to match new DPB entries with existing ones by their POCs. */
> > 	for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
> > 		const struct v4l2_h264_dpb_entry *ndpb = &dec_param->dpb[i];
> > 
> > -		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > +		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
> > 			continue;
> > 
> > 		/*
> > @@ -290,8 +289,7 @@ static void update_dpb(struct hantro_ctx *ctx)
> > 			struct v4l2_h264_dpb_entry *cdpb;
> > 
> > 			cdpb = &ctx->h264_dec.dpb[j];
> > -			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
> > -			    !dpb_entry_match(cdpb, ndpb))
> > +			if (!dpb_entry_match(cdpb, ndpb))
> > 				continue;
> > 
> > 			*cdpb = *ndpb;
> > -- 
> > 2.34.1
> >

Ezequiel Garcia March 30, 2022, 11:43 p.m. UTC | #3

On Wed, Mar 30, 2022 at 12:16 PM Nicolas Dufresne
<[e-mail]> wrote:
>
> Le mercredi 30 mars 2022 à 09:59 +0200, Sebastian Fricke a écrit :
> > Hey Nicolas,
> >
> > On 28.03.2022 15:59, Nicolas Dufresne wrote:
> > > From: Jonas Karlman <[e-mail]>
> > >
> > > The driver maintains stable slot location for reference pictures. This
> >
> > s/slot location/slot locations/
> >
> > > change makes the code more robust by using the reference_ts as key and
> > > by marking all entries invalid right from the start.
> > >
> > > Signed-off-by: Jonas Karlman <[e-mail]>
> > > Signed-off-by: Nicolas Dufresne <[e-mail]>
> > > ---
> > > drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
> > > 1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
> > > index 228629fb3cdf..7377fc26f780 100644
> > > --- a/drivers/staging/media/hantro/hantro_h264.c
> > > +++ b/drivers/staging/media/hantro/hantro_h264.c
> > > @@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
> > > static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
> > >                         const struct v4l2_h264_dpb_entry *b)
> > > {
> > > -   return a->top_field_order_cnt == b->top_field_order_cnt &&
> > > -          a->bottom_field_order_cnt == b->bottom_field_order_cnt;
> > > +   return a->reference_ts == b->reference_ts;
> > > }
> > >
> > > static void update_dpb(struct hantro_ctx *ctx)
> > > @@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
> > >
> > >     /* Disable all entries by default. */
> > >     for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
> > > -           ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
> > > +           ctx->h264_dec.dpb[i].flags = 0;
> >
> > Ehm ... we just remove all flags? Can't this have any unwanted side
> > effects like removing a flag that we actually wanted to keep?
> > (Like long term or the field flags?)
>
> This is a local copy of the dpb, the DPB is fully passed for every decode. So
> these flags will be fully restored lower when we copy the found entry. In fact,
> holding a state here would not represent well the userland intention and can
> have negative side effect on the decoding. Flags are not immutable between
> decode and can change. This simplify the code, and make things less error prone.
> This part of the code is already a bit complex, no need for an extra layer.
>
> > If we just want to set the DPB entry inactive, then removing the ACTIVE
> > flag seems like the correct approach to me.
> > If we want to get rid of the VALID flag as well, then we could just do:
> >               ctx->h264_dec.dpb[i].flags &=
> >        ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);
> >
> > In case we really want to reset all flags, I'd say adjust the comment
> > above it:
> > ```
> > -     /* Disable all entries by default. */
> > +     /* Reset the flags for all entries by default. */
> > ```
>
> This reads the same to me, but I can do that yes. understand that VALID means
> the reference exist and the TS should point to some existing past reference
> (unless there was some decode error, which the userland may not be aware yet as
> this is asynchronous). While ACTIVE means that it is used as a reference. FFMPEG
> is known not to filter inactive references. ACTIVE is just a flag without bunch
> of other flags that can change for every decode. So none of this make sense
> between 2 decodes.
>

Please leave the comment as-is, 'Disable all entries' is readable.

As much as I'd love to have very clear comments everywhere,
we have to accept that only people with insight on codec details
are expected to make sense of driver details :-)

Thanks,
Ezequiel

Sebastian Fricke March 31, 2022, 6:54 a.m. UTC | #4

Hey Nicolas & Ezequiel,

On 30.03.2022 20:43, Ezequiel Garcia wrote:
>On Wed, Mar 30, 2022 at 12:16 PM Nicolas Dufresne
><[e-mail]> wrote:
>>
>> Le mercredi 30 mars 2022 à 09:59 +0200, Sebastian Fricke a écrit :
>> > Hey Nicolas,
>> >
>> > On 28.03.2022 15:59, Nicolas Dufresne wrote:
>> > > From: Jonas Karlman <[e-mail]>
>> > >
>> > > The driver maintains stable slot location for reference pictures. This
>> >
>> > s/slot location/slot locations/
>> >
>> > > change makes the code more robust by using the reference_ts as key and
>> > > by marking all entries invalid right from the start.
>> > >
>> > > Signed-off-by: Jonas Karlman <[e-mail]>
>> > > Signed-off-by: Nicolas Dufresne <[e-mail]>
>> > > ---
>> > > drivers/staging/media/hantro/hantro_h264.c | 10 ++++------
>> > > 1 file changed, 4 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
>> > > index 228629fb3cdf..7377fc26f780 100644
>> > > --- a/drivers/staging/media/hantro/hantro_h264.c
>> > > +++ b/drivers/staging/media/hantro/hantro_h264.c
>> > > @@ -258,8 +258,7 @@ static void prepare_table(struct hantro_ctx *ctx)
>> > > static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
>> > >                         const struct v4l2_h264_dpb_entry *b)
>> > > {
>> > > -   return a->top_field_order_cnt == b->top_field_order_cnt &&
>> > > -          a->bottom_field_order_cnt == b->bottom_field_order_cnt;
>> > > +   return a->reference_ts == b->reference_ts;
>> > > }
>> > >
>> > > static void update_dpb(struct hantro_ctx *ctx)
>> > > @@ -273,13 +272,13 @@ static void update_dpb(struct hantro_ctx *ctx)
>> > >
>> > >     /* Disable all entries by default. */
>> > >     for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
>> > > -           ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
>> > > +           ctx->h264_dec.dpb[i].flags = 0;
>> >
>> > Ehm ... we just remove all flags? Can't this have any unwanted side
>> > effects like removing a flag that we actually wanted to keep?
>> > (Like long term or the field flags?)
>>
>> This is a local copy of the dpb, the DPB is fully passed for every decode. So
>> these flags will be fully restored lower when we copy the found entry. In fact,
>> holding a state here would not represent well the userland intention and can
>> have negative side effect on the decoding. Flags are not immutable between
>> decode and can change. This simplify the code, and make things less error prone.
>> This part of the code is already a bit complex, no need for an extra layer.
>>
>> > If we just want to set the DPB entry inactive, then removing the ACTIVE
>> > flag seems like the correct approach to me.
>> > If we want to get rid of the VALID flag as well, then we could just do:
>> >               ctx->h264_dec.dpb[i].flags &=
>> >        ~(V4L2_H264_DPB_ENTRY_FLAG_ACTIVE | V4L2_H264_DPB_ENTRY_FLAG_VALID);
>> >
>> > In case we really want to reset all flags, I'd say adjust the comment
>> > above it:
>> > ```
>> > -     /* Disable all entries by default. */
>> > +     /* Reset the flags for all entries by default. */
>> > ```
>>
>> This reads the same to me, but I can do that yes. understand that VALID means
>> the reference exist and the TS should point to some existing past reference
>> (unless there was some decode error, which the userland may not be aware yet as
>> this is asynchronous). While ACTIVE means that it is used as a reference. FFMPEG
>> is known not to filter inactive references. ACTIVE is just a flag without bunch
>> of other flags that can change for every decode. So none of this make sense
>> between 2 decodes.
>>
>
>Please leave the comment as-is, 'Disable all entries' is readable.
>
>As much as I'd love to have very clear comments everywhere,
>we have to accept that only people with insight on codec details
>are expected to make sense of driver details :-)

Fair enough, I just feel like the code should try his best to have as
few hidden details as possible. And in this case our intention is to
disable the entries (removing the active and valid flag) but what we do
is that we remove all flags.

The hidden details at this point are:
- flags will be restored automatically later on and we do not want to
hold the state here
- flags are not immutable between decodes
- this was a decision by the programmer to simplify the code and not a rule in the specification.

All of these things are not obvious from the code (from my POV) and
there are no pointers to the specification, which means the reader is
seemingly expected to know the specification on an expert level. I think
really understanding the H264 specification is hard enough on its own.
And as we do that change right now, I think we might as well take some
time to document as much hidden details as possible and I really think
that it will not lower the code quality when we add another 20-40 lines
of comments.

I might be wrong (as I still learn about H264) but I would love if we
can write code (especially open-source code) in a manner that enables
others to learn quickly.

>
>Thanks,
>Ezequiel

Greetings,
Sebastian

Patch

diff --git a/drivers/staging/media/hantro/hantro_h264.c b/drivers/staging/media/hantro/hantro_h264.c
index 228629fb3cdf..7377fc26f780 100644
--- a/drivers/staging/media/hantro/hantro_h264.c
+++ b/drivers/staging/media/hantro/hantro_h264.c
@@ -258,8 +258,7 @@  static void prepare_table(struct hantro_ctx *ctx)
 static bool dpb_entry_match(const struct v4l2_h264_dpb_entry *a,
 			    const struct v4l2_h264_dpb_entry *b)
 {
-	return a->top_field_order_cnt == b->top_field_order_cnt &&
-	       a->bottom_field_order_cnt == b->bottom_field_order_cnt;
+	return a->reference_ts == b->reference_ts;
 }
 
 static void update_dpb(struct hantro_ctx *ctx)
@@ -273,13 +272,13 @@  static void update_dpb(struct hantro_ctx *ctx)
 
 	/* Disable all entries by default. */
 	for (i = 0; i < ARRAY_SIZE(ctx->h264_dec.dpb); i++)
-		ctx->h264_dec.dpb[i].flags &= ~V4L2_H264_DPB_ENTRY_FLAG_ACTIVE;
+		ctx->h264_dec.dpb[i].flags = 0;
 
 	/* Try to match new DPB entries with existing ones by their POCs. */
 	for (i = 0; i < ARRAY_SIZE(dec_param->dpb); i++) {
 		const struct v4l2_h264_dpb_entry *ndpb = &dec_param->dpb[i];
 
-		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
+		if (!(ndpb->flags & V4L2_H264_DPB_ENTRY_FLAG_VALID))
 			continue;
 
 		/*
@@ -290,8 +289,7 @@  static void update_dpb(struct hantro_ctx *ctx)
 			struct v4l2_h264_dpb_entry *cdpb;
 
 			cdpb = &ctx->h264_dec.dpb[j];
-			if (cdpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE ||
-			    !dpb_entry_match(cdpb, ndpb))
+			if (!dpb_entry_match(cdpb, ndpb))
 				continue;
 
 			*cdpb = *ndpb;