diff mbox series

[2/3] media: cedrus: h264: Properly configure reference field

Message ID 20200604185745.23568-3-jernej.skrabec@siol.net (mailing list archive)
State Changes Requested, archived
Delegated to: Hans Verkuil
Headers
Series media: uapi: cedrus: Fix decoding interlaced H264 content |

Commit Message

Jernej Škrabec June 4, 2020, 6:57 p.m. UTC 
  When interlaced H264 content is being decoded, references must indicate
which field is being referenced. Currently this was done by checking
capture buffer flags. However, that is not correct because capture
buffer may hold both fields.

Fix this by checking newly introduced flags in reference lists.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Nicolas Dufresne June 5, 2020, 5:16 p.m. UTC | #1 
Le jeudi 04 juin 2020 à 20:57 +0200, Jernej Skrabec a écrit :
> When interlaced H264 content is being decoded, references must indicate
> which field is being referenced. Currently this was done by checking
> capture buffer flags. However, that is not correct because capture
> buffer may hold both fields.
> 
> Fix this by checking newly introduced flags in reference lists.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Perhaps an additional patch could cleanup the miss-leading comment in
v4l2_h264_dpb_entry definition.

Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

> ---
>  drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> index cce527bbdf86..c87717d17ec5 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> @@ -183,7 +183,6 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  	for (i = 0; i < num_ref; i++) {
>  		const struct v4l2_h264_dpb_entry *dpb;
>  		const struct cedrus_buffer *cedrus_buf;
> -		const struct vb2_v4l2_buffer *ref_buf;
>  		unsigned int position;
>  		int buf_idx;
>  		u8 dpb_idx;
> @@ -198,12 +197,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
>  		if (buf_idx < 0)
>  			continue;
>  
> -		ref_buf = to_vb2_v4l2_buffer(cap_q->bufs[buf_idx]);
> -		cedrus_buf = vb2_v4l2_to_cedrus_buffer(ref_buf);
> +		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
>  		position = cedrus_buf->codec.h264.position;
>  
>  		sram_array[i] |= position << 1;
> -		if (ref_buf->field == V4L2_FIELD_BOTTOM)
> +		if (ref_list[i].flags & V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD)
>  			sram_array[i] |= BIT(0);
>  	}
>
Jernej Škrabec June 5, 2020, 5:26 p.m. UTC | #2 
Dne petek, 05. junij 2020 ob 19:16:35 CEST je Nicolas Dufresne napisal(a):
> Le jeudi 04 juin 2020 à 20:57 +0200, Jernej Skrabec a écrit :
> > When interlaced H264 content is being decoded, references must indicate
> > which field is being referenced. Currently this was done by checking
> > capture buffer flags. However, that is not correct because capture
> > buffer may hold both fields.
> > 
> > Fix this by checking newly introduced flags in reference lists.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> Perhaps an additional patch could cleanup the miss-leading comment in
> v4l2_h264_dpb_entry definition.

I missed that. I think this change actually belongs to patch 1. I'll fix it in 
v2.

Best regards,
Jernej

> 
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> > ---
> > 
> >  drivers/staging/media/sunxi/cedrus/cedrus_h264.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c index
> > cce527bbdf86..c87717d17ec5 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
> > @@ -183,7 +183,6 @@ static void _cedrus_write_ref_list(struct cedrus_ctx
> > *ctx,> 
> >  	for (i = 0; i < num_ref; i++) {
> >  	
> >  		const struct v4l2_h264_dpb_entry *dpb;
> >  		const struct cedrus_buffer *cedrus_buf;
> > 
> > -		const struct vb2_v4l2_buffer *ref_buf;
> > 
> >  		unsigned int position;
> >  		int buf_idx;
> >  		u8 dpb_idx;
> > 
> > @@ -198,12 +197,11 @@ static void _cedrus_write_ref_list(struct cedrus_ctx
> > *ctx,> 
> >  		if (buf_idx < 0)
> >  		
> >  			continue;
> > 
> > -		ref_buf = to_vb2_v4l2_buffer(cap_q->bufs[buf_idx]);
> > -		cedrus_buf = vb2_v4l2_to_cedrus_buffer(ref_buf);
> > +		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
> > 
> >  		position = cedrus_buf->codec.h264.position;
> >  		
> >  		sram_array[i] |= position << 1;
> > 
> > -		if (ref_buf->field == V4L2_FIELD_BOTTOM)
> > +		if (ref_list[i].flags & 
V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD)
> > 
> >  			sram_array[i] |= BIT(0);
> >  	
> >  	}
diff mbox series

Patch

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
index cce527bbdf86..c87717d17ec5 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h264.c
@@ -183,7 +183,6 @@  static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 	for (i = 0; i < num_ref; i++) {
 		const struct v4l2_h264_dpb_entry *dpb;
 		const struct cedrus_buffer *cedrus_buf;
-		const struct vb2_v4l2_buffer *ref_buf;
 		unsigned int position;
 		int buf_idx;
 		u8 dpb_idx;
@@ -198,12 +197,11 @@  static void _cedrus_write_ref_list(struct cedrus_ctx *ctx,
 		if (buf_idx < 0)
 			continue;
 
-		ref_buf = to_vb2_v4l2_buffer(cap_q->bufs[buf_idx]);
-		cedrus_buf = vb2_v4l2_to_cedrus_buffer(ref_buf);
+		cedrus_buf = vb2_to_cedrus_buffer(cap_q->bufs[buf_idx]);
 		position = cedrus_buf->codec.h264.position;
 
 		sram_array[i] |= position << 1;
-		if (ref_buf->field == V4L2_FIELD_BOTTOM)
+		if (ref_list[i].flags & V4L2_H264_REFERENCE_FLAG_BOTTOM_FIELD)
 			sram_array[i] |= BIT(0);
 	}