GNOME Bugzilla – Bug 779466
v4l2: improve v4l2dec auto-negotiation
Last modified: 2017-11-29 15:16:26 UTC
Auto-negotiation for decoding videos with the V4L2 decoder results in a suboptimal pipeline configuration. The following patches allow to set the decoder framerate based on the framerate of the encoded video and to auto-select GST_V4L2_IO_DMABUF for capture buffers if the driver supports it.
Created attachment 347046 [details] [review] v4l2videodec: only set latency if the frame duration is valid
Created attachment 347047 [details] [review] v4l2object: set streamparm for outputs that support it
Created attachment 347048 [details] [review] v4l2object: auto-detect dmabuf export for V4L2_IO_AUTO on capture side
(In reply to Michael Tretter from comment #2) > Created attachment 347047 [details] [review] [review] > v4l2object: set streamparm for outputs that support it What's the use for framerate in driver here ? Is this to implement some deadline ?
Review of attachment 347046 [details] [review]: It's the SRC that should provide the rate, not the sink. Is duration on the v4l2-output correct?
(In reply to Nicolas Dufresne (stormer) from comment #5) > Review of attachment 347046 [details] [review] [review]: > > It's the SRC that should provide the rate, not the sink. Is duration on the > v4l2-output correct? I do not use a v4l2-output as sink, but the kmssink, which reports a framerate range and the caps negotiation results in a framerate of 0/1. It's a driver bug to accept this framerate, but this leads to an invalid duration on the decoder src and there should be a warning instead of using the invalid duration for calculating a latency. As the output does not request a specific framerate, I want to use the framerate that is reported by the encoded video. The other patch allows to set the framerate on the output and then read it on capture and use it for calculating the latency.
What I think, is that we should, when available, pick the rate from the incoming caps (from upstream), and ignore anything from driver or downstream.
Review of attachment 347048 [details] [review]: This check was alread implemented in the GstV4l2Allocator code, I was wondering if it was usable, otherwise we should clean it up, and remove.
(In reply to Nicolas Dufresne (stormer) from comment #8) > Review of attachment 347048 [details] [review] [review]: > > This check was alread implemented in the GstV4l2Allocator code, I was > wondering if it was usable, otherwise we should clean it up, and remove. I cannot find a check in the GstV4l2Allocator code. The allocator tries to export the dmabuf only if the mode of the GstV4l2Object was set to GST_V4L2_IO_DMABUF before and handles the export failures. You can change the mode via properties, but if you do not specify the mode (because you are using playbin), it will always use GST_V4L2_IO_MMAP.
(In reply to Nicolas Dufresne (stormer) from comment #7) > What I think, is that we should, when available, pick the rate from the > incoming caps (from upstream), and ignore anything from driver or downstream. I'm not sure about using the caps directly. We could negotiate the framerate with the caps, but we are already asking capture devices about their framerate, so we could do the same with the mem2mem devices.
See: GST_V4L2_ALLOCATOR_CAN_REQUEST() GST_V4L2_ALLOCATOR_CAN_ALLOCATE() Obviously you need to create the allocator a bit earlier, but you'll need it anyway.
(In reply to Michael Tretter from comment #10) > I'm not sure about using the caps directly. We could negotiate the framerate > with the caps, but we are already asking capture devices about their > framerate, so we could do the same with the mem2mem devices. This is specific to decoder, it's the incoming caps the count here. The latency is driven by speed of incoming frames.
(In reply to Nicolas Dufresne (stormer) from comment #11) > See: > GST_V4L2_ALLOCATOR_CAN_REQUEST() > GST_V4L2_ALLOCATOR_CAN_ALLOCATE() > > Obviously you need to create the allocator a bit earlier, but you'll need it > anyway. If I understand correctly, the allocator should already know, if the driver supports dmabuf, but currently the mode of the v4l2object (capture) is not updated. Therefore, it doesn't use GST_V4L2_IO_DMABUF, but always falls back to GST_V4L2_IO_MMAP. The better solution would be to update the req_mode after probing the allocator and switch to GST_V4L2_IO_DMABUF if supported. At least it should solve my issue to automatically select GST_V4L2_IO_DMABUF if the device supports it and I do not specify it as property.
Review of attachment 347046 [details] [review]: ::: sys/v4l2/gstv4l2videodec.c @@ +754,3 @@ + if (GST_CLOCK_TIME_IS_VALID (self->v4l2capture->duration)) { + GST_DEBUG_OBJECT (self, "Setting latency: %u * %llu", %llu for unsigned 64bit is 32bit CPU specific here. I'll correct it locally, I just wanted you to know. GLib and GStreamer provides defines to help with that. "%" GST_TIME_FORMAT, GST_TIME_ARGS (time) Will print human readable from of time, splitting days, hours, minute, seconds, fraction. "%" G_GUINT64_FORMAT, time Will automatically pick from %lu/%llu depending on the platform. There is many other _FORMAT definition you can use to write portable code.
Attachment 347046 [details] pushed as 1d43f6d - v4l2videodec: only set latency if the frame duration is valid Attachment 347047 [details] pushed as ce5c0b8 - v4l2object: set streamparm for outputs that support it
Looking again, the allocator checks for dmabuf importation, but not for expbuf, sorry. I'm thinking about just using your patch as-is in fact, I was nitpicking something that didn't exist. I'll merge into my local tree to see how it behaves. I already know it breaks if you combined dmabuf and tee with various sink. The sink will store a cached dmabuf wrapper as a memory quark. As the quark name is the same regardless of the tee branch, we end up with one branch getting their object destroyed when the other try to cache.
Created attachment 360843 [details] [review] v4l2object: auto-detect dmabuf export for V4L2_IO_AUTO on capture side I've simply rebased on master. I'm currently running this patch locally.
Thanks a lot, I've been running with this enabled and it does not seem to break anything so far. There is a small problem if we do UVC to KMS, but I believe this is driver issues, make this visible will likely bring developer attention. Attachment 360843 [details] pushed as e9c00c0 - v4l2object: auto-detect dmabuf export for V4L2_IO_AUTO on capture side
I use a v4l driver that does not support DMABUF, and right now this patch triggers the use of DMABUF. The errno is ENOTTY, but the code checks for -ENOTTY, is that a typo ? if (errno == -ENOTTY) ret = FALSE; See https://bugzilla.gnome.org/show_bug.cgi?id=790940
It is a typo, errno is positive, while return value is negative. I'll fix.
commit a31581064013e729ae03f33c086cadb8919e17ab (HEAD -> master, origin/master, origin/HEAD) Author: Florent Thiéry <florent.thiery@ubicast.eu> Date: Wed Nov 29 11:29:31 2017 +0100 v4l2object: Fix dmabuf support detection This resulted in improper selection of dmabuf on unsupported drivers. The checked ioctl errno was not correct. https://bugzilla.gnome.org/show_bug.cgi?id=790940