Re: [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
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
>>
>> state->info was NULL since I completely forgot to set state->info.
>> Oops.
>>
>> Reported-by: Ezequiel Garcia
>> Signed-off-by: Hans Verkuil
>
> For both patches:
>
> Tested-by: Ezequiel Garcia
>
> 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);
>> }
>
Re: [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
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 > > > > > > state->info was NULL since I completely forgot to set state->info. > > > Oops. > > > > > > Reported-by: Ezequiel Garcia > > > Signed-off-by: Hans Verkuil > > > > For both patches: > > > > Tested-by: Ezequiel Garcia > > > > 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: Finishing decoding 0:00:16.931866009 180 0x5593bcbd18a0 DEBUG v4l2videodec gstv4l2videodec.c:340:gst_v4l2_decoder_cmd: sending v4l2 decoder command 1 with flags 0 0:00:16.934260349 180 0x5593bcbd18a0 DEBUG v4l2videodec gstv4l2videodec.c:384:gst_v4l2_video_dec_finish: Waiting for decoder stop [stalls here] Regards, Ezequiel
Re: [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
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
> >
> > state->info was NULL since I completely forgot to set state->info.
> > Oops.
> >
> > Reported-by: Ezequiel Garcia
> > Signed-off-by: Hans Verkuil
>
> For both patches:
>
> Tested-by: Ezequiel Garcia
>
> 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);
> > }
>
>
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
On Mon, 2018-09-10 at 17:00 +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> state->info was NULL since I completely forgot to set state->info.
> Oops.
>
> Reported-by: Ezequiel Garcia
> Signed-off-by: Hans Verkuil
For both patches:
Tested-by: Ezequiel Garcia
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);
> }
[PATCH 2/2] vicodec: set state->info before calling the encode/decode funcs
From: Hans Verkuil
state->info was NULL since I completely forgot to set state->info.
Oops.
Reported-by: Ezequiel Garcia
Signed-off-by: Hans Verkuil
---
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
