Re: [RFC PATCH 13/13] staging: vc04_services: Add a V4L2 M2M codec driver

From: Maarten <maarten@rmail.be>
To: Nicolas Dufresne <nicolas@ndufresne.ca>,
	Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>,
	linux-media@vger.kernel.org
Cc: Kieran Bingham <kbingham@kernel.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Umang Jain <umang.jain@ideasonboard.com>,
	Dave Stevenson <dave.stevenson@raspberrypi.com>,
	John Cox <jc@kynesim.co.uk>, Dom Cobley <popcornmix@gmail.com>,
	David Plowman <david.plowman@raspberrypi.com>,
	Maxim Devaev <mdevaev@gmail.com>
Subject: Re: [RFC PATCH 13/13] staging: vc04_services: Add a V4L2 M2M codec driver
Date: Mon, 01 Apr 2024 11:45:56 +0200	[thread overview]
Message-ID: <38f934b9422d82893337cd7b07c63db6@rmail.be> (raw)
In-Reply-To: <fe0f9269c9f21441b2d3166e68837cdd@rmail.be>

Maarten schreef op 2024-03-24 13:35:
> Nicolas Dufresne schreef op 2024-03-06 22:14:
> 
> <snip>
> 
>>> > > +	/* vb2 timestamps in nsecs, mmal in usecs */
>>> > > +	vb2->vb2_buf.timestamp = mmal_buf->pts * 1000;
>>> >
>>> > M2M drivers are not allowed to "interpret" the timesamp field. They can
>>> > only
>>> > copy it over. This scaling of the timestamp seems problematic and may
>>> > possibly
>>> > impair some userspace ability to match in and out buffers. Is there
>>> > some other
>>> > user data that could be use to maintain the lower bits ?
>>> >
>>> > I realized that it should not have worked for GStreamer, since it uses
>>> > a frame
>>> > counter as timestamp (and no real timestamp), but accidentally, it only
>>> > uses
>>> > tv_sec member (with a implicit cast from signed to unsigned, but lets
>>> > not grab
>>> > other projects issues in, unlike stateless, there is a single
>>> > representation of
>>> > it, so its less of an issue).
>>> >
>>> > https://gitlab.freedesktop.org/gstreamer/gstreamer/-/blob/main/subprojects/gst-plugins-good/sys/v4l2/gstv4l2bufferpool.c?ref_type=heads#L1179
>>> >
>>> > There is no requirement in the spec that tells userspace using forward
>>> > counters
>>> > to only use the seconds, so I suppose this is far from ideal.
>>> 
>>> You mean that vb2_buf.timestamp is not necessarily a time-based
>>> timestamp? I'll have to see how mmal is using the pts.
>> 
>> Correct, its opaque data from user, the spec only defines the transfer 
>> rules for
>> that data in the case that a frame is duplicated or merged.
> 
> So, I asked downstream to (if possible) handle mmal timestamps
> opaquely, which would simplify this code a lot and fix your GStreamer
> issues: https://github.com/raspberrypi/firmware/issues/1883 . We'll
> see if it's possible or not; if not I'll have to look for a way to
> present a time to mmal while keeping the opaque data intact.

Good news, it looks like the firmware is not actually using the 
timestamp, so, we can drop that part of it :-)

Maarten