| Message ID | 20180910150040.39265-2-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] vicodec: check for valid format in v4l2_fwht_en/decode | expand |
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); > }
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); > > } > >
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
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); >> } >
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); }