Re: [PATCH 1/7] media: coda: set output buffer bytesused to appease v4l2-compliance

[Hans Verkuil <hverkuil@xs4all.nl>]

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