| Message ID | 20220404163533.707508-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] media: coda: set output buffer bytesused to appease v4l2-compliance | expand |
Le lundi 04 avril 2022 à 18:35 +0200, Philipp Zabel a écrit : > The V4L2 specification states: > > "If the application sets this to 0 for an output stream, then bytesused > will be set to the size of the buffer (see the length field of this > struct) by the driver." > > Since we set allow_zero_bytesused, we have to handle this ourselves. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > drivers/media/platform/chips-media/coda-bit.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c > index c484c008ab02..705a179ea8f0 100644 > --- a/drivers/media/platform/chips-media/coda-bit.c > +++ b/drivers/media/platform/chips-media/coda-bit.c > @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) > /* Dump empty buffers */ > if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { > src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > + vb2_set_plane_payload(&src_buf->vb2_buf, 0, > + vb2_plane_size(&src_buf->vb2_buf, > + 0)); > v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE); > continue; > }
On 04/04/2022 18:35, Philipp Zabel wrote: > The V4L2 specification states: > > "If the application sets this to 0 for an output stream, then bytesused > will be set to the size of the buffer (see the length field of this > struct) by the driver." > > Since we set allow_zero_bytesused, we have to handle this ourselves. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > drivers/media/platform/chips-media/coda-bit.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c > index c484c008ab02..705a179ea8f0 100644 > --- a/drivers/media/platform/chips-media/coda-bit.c > +++ b/drivers/media/platform/chips-media/coda-bit.c > @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) > /* Dump empty buffers */ > if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { > src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > + vb2_set_plane_payload(&src_buf->vb2_buf, 0, > + vb2_plane_size(&src_buf->vb2_buf, > + 0)); Would it be possible to stop using allow_zero_bytesused altogether? Are there still applications that rely on zero-sized output buffers to stop the decoder? I'm not actually sure that I want this in the driver, perhaps v4l2-compliance can be modified to turn a fail into a warn if the driver is the coda driver. Patching the driver is hiding the fact that the coda driver does something non-standard for legacy reasons. It doesn't make sense either to change bytesused to the buffer size since there really is nothing in the buffer. v4l2-compliance already has checks for two drivers, search for is_vivid and is_uvcvideo. I'm skipping this patch for now. Regards, Hans > v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE); > continue; > }
Hi Hans, On Do, 2022-04-21 at 11:44 +0200, Hans Verkuil wrote: > On 04/04/2022 18:35, Philipp Zabel wrote: > > The V4L2 specification states: > > > > "If the application sets this to 0 for an output stream, then bytesused > > will be set to the size of the buffer (see the length field of this > > struct) by the driver." > > > > Since we set allow_zero_bytesused, we have to handle this ourselves. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/media/platform/chips-media/coda-bit.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c > > index c484c008ab02..705a179ea8f0 100644 > > --- a/drivers/media/platform/chips-media/coda-bit.c > > +++ b/drivers/media/platform/chips-media/coda-bit.c > > @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) > > /* Dump empty buffers */ > > if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { > > src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); > > + vb2_set_plane_payload(&src_buf->vb2_buf, 0, > > + vb2_plane_size(&src_buf->vb2_buf, > > + 0)); > > Would it be possible to stop using allow_zero_bytesused altogether? > > Are there still applications that rely on zero-sized output buffers to stop the > decoder? This was used by GStreamer 1.8. The code is still left in current versions, but is never executed unless the decoder stop command fails: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454 Whether there are still any applications using GStreamer 1.8 for V4L2 video decoding on devices that get kernel updates, I don't know. > I'm not actually sure that I want this in the driver, perhaps v4l2-compliance > can be modified to turn a fail into a warn if the driver is the coda driver. Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc? > Patching the driver is hiding the fact that the coda driver does something > non-standard for legacy reasons. It doesn't make sense either to change > bytesused to the buffer size since there really is nothing in the buffer. > > v4l2-compliance already has checks for two drivers, search for is_vivid and > is_uvcvideo. Ok. > I'm skipping this patch for now. regards Philipp
On 21/04/2022 12:24, Philipp Zabel wrote: > Hi Hans, > > On Do, 2022-04-21 at 11:44 +0200, Hans Verkuil wrote: >> On 04/04/2022 18:35, Philipp Zabel wrote: >>> The V4L2 specification states: >>> >>> "If the application sets this to 0 for an output stream, then bytesused >>> will be set to the size of the buffer (see the length field of this >>> struct) by the driver." >>> >>> Since we set allow_zero_bytesused, we have to handle this ourselves. >>> >>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> >>> --- >>> drivers/media/platform/chips-media/coda-bit.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c >>> index c484c008ab02..705a179ea8f0 100644 >>> --- a/drivers/media/platform/chips-media/coda-bit.c >>> +++ b/drivers/media/platform/chips-media/coda-bit.c >>> @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) >>> /* Dump empty buffers */ >>> if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { >>> src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); >>> + vb2_set_plane_payload(&src_buf->vb2_buf, 0, >>> + vb2_plane_size(&src_buf->vb2_buf, >>> + 0)); >> >> Would it be possible to stop using allow_zero_bytesused altogether? >> >> Are there still applications that rely on zero-sized output buffers to stop the >> decoder? > > This was used by GStreamer 1.8. The code is still left in current > versions, but is never executed unless the decoder stop command fails: > > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454 > > Whether there are still any applications using GStreamer 1.8 for V4L2 > video decoding on devices that get kernel updates, I don't know. > >> I'm not actually sure that I want this in the driver, perhaps v4l2-compliance >> can be modified to turn a fail into a warn if the driver is the coda driver. > > Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc? Yes for venus and s5p, but why would imx-jpeg use this? It makes no sense for a jpeg codec. I think it should just be removed for imx-jpeg. IMHO, once a decoder supports the STOP command, it should no longer set allow_zero_bytesused to true. But that decision is up to you for the coda driver. Regards, Hans > >> Patching the driver is hiding the fact that the coda driver does something >> non-standard for legacy reasons. It doesn't make sense either to change >> bytesused to the buffer size since there really is nothing in the buffer. >> >> v4l2-compliance already has checks for two drivers, search for is_vivid and >> is_uvcvideo. > > Ok. > >> I'm skipping this patch for now. > > regards > Philipp
On Di, 2022-04-26 at 07:52 +0200, Hans Verkuil wrote: [...] > > > Are there still applications that rely on zero-sized output buffers to stop the > > > decoder? > > > > This was used by GStreamer 1.8. The code is still left in current > > versions, but is never executed unless the decoder stop command fails: > > > > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2videodec.c#L446-454 > > > > Whether there are still any applications using GStreamer 1.8 for V4L2 > > video decoding on devices that get kernel updates, I don't know. > > > > > I'm not actually sure that I want this in the driver, perhaps v4l2-compliance > > > can be modified to turn a fail into a warn if the driver is the coda driver. > > > > Same for nxp/imx-jpeg, qcom/venus and samsung/s5p-mfc? > > Yes for venus and s5p, but why would imx-jpeg use this? > > It makes no sense for a jpeg codec. I think it should just be removed for imx-jpeg. > > IMHO, once a decoder supports the STOP command, it should no longer set > allow_zero_bytesused to true. But that decision is up to you for the coda > driver. Turns out there is no associated v4l2-compliance failure at all. I'd just drop this patch for now and keep the allow_zero_bytesused flag as-is. regards Philipp
diff --git a/drivers/media/platform/chips-media/coda-bit.c b/drivers/media/platform/chips-media/coda-bit.c index c484c008ab02..705a179ea8f0 100644 --- a/drivers/media/platform/chips-media/coda-bit.c +++ b/drivers/media/platform/chips-media/coda-bit.c @@ -381,6 +381,9 @@ void coda_fill_bitstream(struct coda_ctx *ctx, struct list_head *buffer_list) /* Dump empty buffers */ if (!vb2_get_plane_payload(&src_buf->vb2_buf, 0)) { src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx); + vb2_set_plane_payload(&src_buf->vb2_buf, 0, + vb2_plane_size(&src_buf->vb2_buf, + 0)); v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE); continue; }
The V4L2 specification states: "If the application sets this to 0 for an output stream, then bytesused will be set to the size of the buffer (see the length field of this struct) by the driver." Since we set allow_zero_bytesused, we have to handle this ourselves. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- drivers/media/platform/chips-media/coda-bit.c | 3 +++ 1 file changed, 3 insertions(+)