All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] vicodec: check for valid format in v4l2_fwht_en/decode
@ 2018-09-10 15:00 Hans Verkuil
  2018-09-10 15:00 ` [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs Hans Verkuil
  0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2018-09-10 15:00 UTC (permalink / raw)
  To: linux-media; +Cc: Ezequiel Garcia, Hans Verkuil

From: Hans Verkuil <hans.verkuil@cisco.com>

These functions did not return an error if state->info was NULL
or an unsupported pixelformat was selected (should not happen,
but just to be on the safe side).

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vicodec/codec-v4l2-fwht.c | 15 +++++++++++----
 drivers/media/platform/vicodec/codec-v4l2-fwht.h |  7 ++-----
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.c b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
index cfcf84b8574d..1b86eb9868c3 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.c
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.c
@@ -51,8 +51,7 @@ const struct v4l2_fwht_pixfmt_info *v4l2_fwht_get_pixfmt(u32 idx)
 	return v4l2_fwht_pixfmts + idx;
 }
 
-unsigned int v4l2_fwht_encode(struct v4l2_fwht_state *state,
-			      u8 *p_in, u8 *p_out)
+int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 {
 	unsigned int size = state->width * state->height;
 	const struct v4l2_fwht_pixfmt_info *info = state->info;
@@ -62,6 +61,8 @@ unsigned int v4l2_fwht_encode(struct v4l2_fwht_state *state,
 	u32 encoding;
 	u32 flags = 0;
 
+	if (!info)
+		return -EINVAL;
 	rf.width = state->width;
 	rf.height = state->height;
 	rf.luma = p_in;
@@ -137,6 +138,8 @@ unsigned int v4l2_fwht_encode(struct v4l2_fwht_state *state,
 		rf.cr = rf.cb + 2;
 		rf.luma++;
 		break;
+	default:
+		return -EINVAL;
 	}
 
 	cf.width = state->width;
@@ -180,8 +183,7 @@ unsigned int v4l2_fwht_encode(struct v4l2_fwht_state *state,
 	return cf.size + sizeof(*p_hdr);
 }
 
-int v4l2_fwht_decode(struct v4l2_fwht_state *state,
-		     u8 *p_in, u8 *p_out)
+int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out)
 {
 	unsigned int size = state->width * state->height;
 	unsigned int chroma_size = size;
@@ -191,6 +193,9 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state,
 	struct fwht_cframe cf;
 	u8 *p;
 
+	if (!state->info)
+		return -EINVAL;
+
 	p_hdr = (struct fwht_cframe_hdr *)p_in;
 	cf.width = ntohl(p_hdr->width);
 	cf.height = ntohl(p_hdr->height);
@@ -320,6 +325,8 @@ int v4l2_fwht_decode(struct v4l2_fwht_state *state,
 			*p++ = 0;
 		}
 		break;
+	default:
+		return -EINVAL;
 	}
 	return 0;
 }
diff --git a/drivers/media/platform/vicodec/codec-v4l2-fwht.h b/drivers/media/platform/vicodec/codec-v4l2-fwht.h
index 7794c186d905..22ae0e4d7315 100644
--- a/drivers/media/platform/vicodec/codec-v4l2-fwht.h
+++ b/drivers/media/platform/vicodec/codec-v4l2-fwht.h
@@ -41,10 +41,7 @@ struct v4l2_fwht_state {
 const struct v4l2_fwht_pixfmt_info *v4l2_fwht_find_pixfmt(u32 pixelformat);
 const struct v4l2_fwht_pixfmt_info *v4l2_fwht_get_pixfmt(u32 idx);
 
-unsigned int v4l2_fwht_encode(struct v4l2_fwht_state *state,
-			      u8 *p_in, u8 *p_out);
-
-int v4l2_fwht_decode(struct v4l2_fwht_state *state,
-		     u8 *p_in, u8 *p_out);
+int v4l2_fwht_encode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out);
+int v4l2_fwht_decode(struct v4l2_fwht_state *state, u8 *p_in, u8 *p_out);
 
 #endif
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
  2018-09-10 15:00 [PATCH 1/2] vicodec: check for valid format in v4l2_fwht_en/decode Hans Verkuil
@ 2018-09-10 15:00 ` Hans Verkuil
  2018-09-10 15:37   ` Ezequiel Garcia
  0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2018-09-10 15:00 UTC (permalink / raw)
  To: linux-media; +Cc: Ezequiel Garcia, Hans Verkuil

From: Hans Verkuil <hans.verkuil@cisco.com>

state->info was NULL since I completely forgot to set state->info.
Oops.

Reported-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vicodec/vicodec-core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index fdd77441a47b..5d42a8414283 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -176,12 +176,15 @@ static int device_process(struct vicodec_ctx *ctx,
 	}
 
 	if (ctx->is_enc) {
-		unsigned int size = v4l2_fwht_encode(state, p_in, p_out);
-
-		vb2_set_plane_payload(&out_vb->vb2_buf, 0, size);
+		state->info = q_out->info;
+		ret = v4l2_fwht_encode(state, p_in, p_out);
+		if (ret < 0)
+			return ret;
+		vb2_set_plane_payload(&out_vb->vb2_buf, 0, ret);
 	} else {
+		state->info = q_cap->info;
 		ret = v4l2_fwht_decode(state, p_in, p_out);
-		if (ret)
+		if (ret < 0)
 			return ret;
 		vb2_set_plane_payload(&out_vb->vb2_buf, 0, q_cap->sizeimage);
 	}
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
  2018-09-10 15:00 ` [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs Hans Verkuil
@ 2018-09-10 15:37   ` Ezequiel Garcia
  2018-09-10 17:16     ` Nicolas Dufresne
  2018-09-11  6:56     ` Hans Verkuil
  0 siblings, 2 replies; 6+ messages in thread
From: Ezequiel Garcia @ 2018-09-10 15:37 UTC (permalink / raw)
  To: Hans Verkuil, linux-media; +Cc: Hans Verkuil

On Mon, 2018-09-10 at 17:00 +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> state->info was NULL since I completely forgot to set state->info.
> Oops.
> 
> Reported-by: Ezequiel Garcia <ezequiel@collabora.com>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>

For both patches:

Tested-by: Ezequiel Garcia <ezequiel@collabora.com>

With these changes, now this gstreamer pipeline no longer
crashes:

gst-launch-1.0 -v videotestsrc num-buffers=30 ! video/x-raw,width=1280,height=720 ! v4l2fwhtenc capture-io-mode=mmap output-io-mode=mmap ! v4l2fwhtdec
capture-io-mode=mmap output-io-mode=mmap ! fakesink

A few things:

  * You now need to mark "[PATCH] vicodec: fix sparse warning" as invalid.
  * v4l2fwhtenc/v4l2fwhtdec elements are not upstream yet.
  * Gstreamer doesn't end properly; and it seems to negotiate
    different sizes for encoded and decoded unless explicitly set.

Thanks!

>  drivers/media/platform/vicodec/vicodec-core.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
> index fdd77441a47b..5d42a8414283 100644
> --- a/drivers/media/platform/vicodec/vicodec-core.c
> +++ b/drivers/media/platform/vicodec/vicodec-core.c
> @@ -176,12 +176,15 @@ static int device_process(struct vicodec_ctx *ctx,
>  	}
>  
>  	if (ctx->is_enc) {
> -		unsigned int size = v4l2_fwht_encode(state, p_in, p_out);
> -
> -		vb2_set_plane_payload(&out_vb->vb2_buf, 0, size);
> +		state->info = q_out->info;
> +		ret = v4l2_fwht_encode(state, p_in, p_out);
> +		if (ret < 0)
> +			return ret;
> +		vb2_set_plane_payload(&out_vb->vb2_buf, 0, ret);
>  	} else {
> +		state->info = q_cap->info;
>  		ret = v4l2_fwht_decode(state, p_in, p_out);
> -		if (ret)
> +		if (ret < 0)
>  			return ret;
>  		vb2_set_plane_payload(&out_vb->vb2_buf, 0, q_cap->sizeimage);
>  	}

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
  2018-09-10 15:37   ` Ezequiel Garcia
@ 2018-09-10 17:16     ` Nicolas Dufresne
  2018-09-10 20:20       ` Ezequiel Garcia
  2018-09-11  6:56     ` Hans Verkuil
  1 sibling, 1 reply; 6+ messages in thread
From: Nicolas Dufresne @ 2018-09-10 17:16 UTC (permalink / raw)
  To: Ezequiel Garcia, Hans Verkuil, linux-media; +Cc: Hans Verkuil

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

Le lundi 10 septembre 2018 à 12:37 -0300, Ezequiel Garcia a écrit :
> On Mon, 2018-09-10 at 17:00 +0200, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > state->info was NULL since I completely forgot to set state->info.
> > Oops.
> > 
> > Reported-by: Ezequiel Garcia <ezequiel@collabora.com>
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> For both patches:
> 
> Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
> 
> With these changes, now this gstreamer pipeline no longer
> crashes:
> 
> gst-launch-1.0 -v videotestsrc num-buffers=30 ! video/x-
> raw,width=1280,height=720 ! v4l2fwhtenc capture-io-mode=mmap output-
> io-mode=mmap ! v4l2fwhtdec
> capture-io-mode=mmap output-io-mode=mmap ! fakesink
> 
> A few things:
> 
>   * You now need to mark "[PATCH] vicodec: fix sparse warning" as
> invalid.
>   * v4l2fwhtenc/v4l2fwhtdec elements are not upstream yet.
>   * Gstreamer doesn't end properly; and it seems to negotiate

Is the driver missing CMD_STOP implementation ? (draining flow)

>     different sizes for encoded and decoded unless explicitly set.
> 
> Thanks!
> 
> >  drivers/media/platform/vicodec/vicodec-core.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vicodec/vicodec-core.c
> > b/drivers/media/platform/vicodec/vicodec-core.c
> > index fdd77441a47b..5d42a8414283 100644
> > --- a/drivers/media/platform/vicodec/vicodec-core.c
> > +++ b/drivers/media/platform/vicodec/vicodec-core.c
> > @@ -176,12 +176,15 @@ static int device_process(struct vicodec_ctx
> > *ctx,
> >  	}
> >  
> >  	if (ctx->is_enc) {
> > -		unsigned int size = v4l2_fwht_encode(state, p_in,
> > p_out);
> > -
> > -		vb2_set_plane_payload(&out_vb->vb2_buf, 0, size);
> > +		state->info = q_out->info;
> > +		ret = v4l2_fwht_encode(state, p_in, p_out);
> > +		if (ret < 0)
> > +			return ret;
> > +		vb2_set_plane_payload(&out_vb->vb2_buf, 0, ret);
> >  	} else {
> > +		state->info = q_cap->info;
> >  		ret = v4l2_fwht_decode(state, p_in, p_out);
> > -		if (ret)
> > +		if (ret < 0)
> >  			return ret;
> >  		vb2_set_plane_payload(&out_vb->vb2_buf, 0, q_cap-
> > >sizeimage);
> >  	}
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
  2018-09-10 17:16     ` Nicolas Dufresne
@ 2018-09-10 20:20       ` Ezequiel Garcia
  0 siblings, 0 replies; 6+ messages in thread
From: Ezequiel Garcia @ 2018-09-10 20:20 UTC (permalink / raw)
  To: Nicolas Dufresne, Hans Verkuil, linux-media; +Cc: Hans Verkuil

On Mon, 2018-09-10 at 13:16 -0400, Nicolas Dufresne wrote:
> Le lundi 10 septembre 2018 à 12:37 -0300, Ezequiel Garcia a écrit :
> > On Mon, 2018-09-10 at 17:00 +0200, Hans Verkuil wrote:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > state->info was NULL since I completely forgot to set state->info.
> > > Oops.
> > > 
> > > Reported-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > For both patches:
> > 
> > Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
> > 
> > With these changes, now this gstreamer pipeline no longer
> > crashes:
> > 
> > gst-launch-1.0 -v videotestsrc num-buffers=30 ! video/x-
> > raw,width=1280,height=720 ! v4l2fwhtenc capture-io-mode=mmap output-
> > io-mode=mmap ! v4l2fwhtdec
> > capture-io-mode=mmap output-io-mode=mmap ! fakesink
> > 
> > A few things:
> > 
> >   * You now need to mark "[PATCH] vicodec: fix sparse warning" as
> > invalid.
> >   * v4l2fwhtenc/v4l2fwhtdec elements are not upstream yet.
> >   * Gstreamer doesn't end properly; and it seems to negotiate
> 
> Is the driver missing CMD_STOP implementation ? (draining flow)
> 

I think that's the case.

Gstreamer debug log, right before it stalls:

0:00:16.929785442   180 0x5593bcbd18a0 DEBUG           v4l2videodec gstv4l2videodec.c:375:gst_v4l2_video_dec_finish:<v4l2fwhtdec0> Finishing decoding
0:00:16.931866009   180 0x5593bcbd18a0 DEBUG           v4l2videodec gstv4l2videodec.c:340:gst_v4l2_decoder_cmd:<v4l2fwhtdec0> sending v4l2 decoder
command 1 with flags 0
0:00:16.934260349   180 0x5593bcbd18a0 DEBUG           v4l2videodec gstv4l2videodec.c:384:gst_v4l2_video_dec_finish:<v4l2fwhtdec0> Waiting for decoder
stop
[stalls here]

Regards,
Ezequiel

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
  2018-09-10 15:37   ` Ezequiel Garcia
  2018-09-10 17:16     ` Nicolas Dufresne
@ 2018-09-11  6:56     ` Hans Verkuil
  1 sibling, 0 replies; 6+ messages in thread
From: Hans Verkuil @ 2018-09-11  6:56 UTC (permalink / raw)
  To: Ezequiel Garcia, linux-media; +Cc: Hans Verkuil

On 09/10/2018 05:37 PM, Ezequiel Garcia wrote:
> On Mon, 2018-09-10 at 17:00 +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> state->info was NULL since I completely forgot to set state->info.
>> Oops.
>>
>> Reported-by: Ezequiel Garcia <ezequiel@collabora.com>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> For both patches:
> 
> Tested-by: Ezequiel Garcia <ezequiel@collabora.com>
> 
> With these changes, now this gstreamer pipeline no longer
> crashes:
> 
> gst-launch-1.0 -v videotestsrc num-buffers=30 ! video/x-raw,width=1280,height=720 ! v4l2fwhtenc capture-io-mode=mmap output-io-mode=mmap ! v4l2fwhtdec
> capture-io-mode=mmap output-io-mode=mmap ! fakesink
> 
> A few things:
> 
>   * You now need to mark "[PATCH] vicodec: fix sparse warning" as invalid.

I'll wait for that to be merged (it's already in a pending pull request), then
rework this patch and add your other patches for a new pull request.

>   * v4l2fwhtenc/v4l2fwhtdec elements are not upstream yet.
>   * Gstreamer doesn't end properly; and it seems to negotiate
>     different sizes for encoded and decoded unless explicitly set.

As mentioned before, vicodec isn't fully compliant with the upcoming
codec spec, and is also missing certain features (selection support,
support for custom bytesperline values, padding, midstream resolution
changes). Patches are welcome.

If you are working on gstreamer elements for this codec, then it would
be great if you could look at making the driver compliant. I have no
plans to work on vicodec until that codec spec is fully finalized, so
you can go ahead with that if you want to.

Would be really nice, and after all, that's why I wrote vicodec!

Regards,

	Hans

> 
> Thanks!
> 
>>  drivers/media/platform/vicodec/vicodec-core.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
>> index fdd77441a47b..5d42a8414283 100644
>> --- a/drivers/media/platform/vicodec/vicodec-core.c
>> +++ b/drivers/media/platform/vicodec/vicodec-core.c
>> @@ -176,12 +176,15 @@ static int device_process(struct vicodec_ctx *ctx,
>>  	}
>>  
>>  	if (ctx->is_enc) {
>> -		unsigned int size = v4l2_fwht_encode(state, p_in, p_out);
>> -
>> -		vb2_set_plane_payload(&out_vb->vb2_buf, 0, size);
>> +		state->info = q_out->info;
>> +		ret = v4l2_fwht_encode(state, p_in, p_out);
>> +		if (ret < 0)
>> +			return ret;
>> +		vb2_set_plane_payload(&out_vb->vb2_buf, 0, ret);
>>  	} else {
>> +		state->info = q_cap->info;
>>  		ret = v4l2_fwht_decode(state, p_in, p_out);
>> -		if (ret)
>> +		if (ret < 0)
>>  			return ret;
>>  		vb2_set_plane_payload(&out_vb->vb2_buf, 0, q_cap->sizeimage);
>>  	}
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-11 11:54 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-10 15:00 [PATCH 1/2] vicodec: check for valid format in v4l2_fwht_en/decode Hans Verkuil
2018-09-10 15:00 ` [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs Hans Verkuil
2018-09-10 15:37   ` Ezequiel Garcia
2018-09-10 17:16     ` Nicolas Dufresne
2018-09-10 20:20       ` Ezequiel Garcia
2018-09-11  6:56     ` Hans Verkuil

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.