After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 719791 - v4l2sink: enable zero-copy path between mfcdec and v4l2sink io-mode=mmap
v4l2sink: enable zero-copy path between mfcdec and v4l2sink io-mode=mmap
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-12-03 17:35 UTC by Julien Isorce
Modified: 2014-05-08 19:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
v4l2bufferpool: mark mapped memory as GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS (1.67 KB, patch)
2013-12-03 17:36 UTC, Julien Isorce
rejected Details | Review
v4l2sink: make sure the v4l2 buffer pool is inactive before to call set_config (1.38 KB, patch)
2013-12-03 17:37 UTC, Julien Isorce
accepted-commit_now Details | Review
v4l2sink: handle the case where allocation is called twice (3.41 KB, patch)
2013-12-03 17:38 UTC, Julien Isorce
reviewed Details | Review
v4l2sink: check returned value of gst_buffer_pool_set_active (888 bytes, patch)
2013-12-03 17:38 UTC, Julien Isorce
accepted-commit_now Details | Review
v4l2: add support for GstVideoAlignment and cropping (6.59 KB, patch)
2013-12-03 17:39 UTC, Julien Isorce
needs-work Details | Review

Description Julien Isorce 2013-12-03 17:35:36 UTC
Currently there is a copy. Video alignment needs also to be handle.

It also require to have the fix of mfcdec here: https://bugzilla.gnome.org/show_bug.cgi?id=719789

Patch follows.
Comment 1 Julien Isorce 2013-12-03 17:36:30 UTC
Created attachment 263410 [details] [review]
v4l2bufferpool: mark mapped memory as GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS
Comment 2 Julien Isorce 2013-12-03 17:37:20 UTC
Created attachment 263411 [details] [review]
v4l2sink: make sure the v4l2 buffer pool is inactive before to call set_config
Comment 3 Julien Isorce 2013-12-03 17:38:17 UTC
Created attachment 263412 [details] [review]
v4l2sink: handle the case where allocation is called twice

even if we fixes or not https://bugzilla.gnome.org/show_bug.cgi?id=719684
Comment 4 Julien Isorce 2013-12-03 17:38:53 UTC
Created attachment 263413 [details] [review]
v4l2sink: check returned value of gst_buffer_pool_set_active
Comment 5 Julien Isorce 2013-12-03 17:39:25 UTC
Created attachment 263414 [details] [review]
v4l2: add support for GstVideoAlignment and cropping
Comment 6 Nicolas Dufresne (ndufresne) 2013-12-03 19:10:16 UTC
Review of attachment 263410 [details] [review]:

Well, no, that we can't do. As suggested in the USERPTR bug, a white list would be acceptable. Currently, to check that it's the case we need to check the allocator being used in the kernel driver, something we cannot do programmatically.
Comment 7 Nicolas Dufresne (ndufresne) 2013-12-03 19:11:03 UTC
Review of attachment 263411 [details] [review]:

Looks good
Comment 8 Nicolas Dufresne (ndufresne) 2013-12-03 19:14:45 UTC
Review of attachment 263412 [details] [review]:

::: sys/v4l2/gstv4l2sink.c
@@ +634,1 @@
   /* we need at least 2 buffers to operate */

Might be better inside the if

@@ +634,2 @@
   /* we need at least 2 buffers to operate */
+  if (pool) {

Can it really be NULL ?
Comment 9 Nicolas Dufresne (ndufresne) 2013-12-03 19:15:58 UTC
Review of attachment 263413 [details] [review]:

Looks more robust this way.
Comment 10 Nicolas Dufresne (ndufresne) 2013-12-03 20:07:32 UTC
Review of attachment 263414 [details] [review]:

::: sys/v4l2/gstv4l2bufferpool.c
@@ +478,3 @@
+      /* do padding and alignment */
+      /* FIXME: should call it but it set wrong values
+       * gst_video_info_align (&obj->info, &pool->align); */

I don't think we need to call this, but it should not return wrong value (unless the alignment value are wrong).

Also we don't check if V4L2 respect the stride alignment. We can't decide it, but we should at least fail the pool activation if we are not aligned.

@@ +660,3 @@
   gst_poll_set_flushing (obj->poll, FALSE);
 
+  GST_V4L2_SET_ACTIVE (obj);

Why and is it related ?

::: sys/v4l2/gstv4l2object.c
@@ +2461,3 @@
+
+    v4l2object->update_crop = TRUE;
+  }

I think it would be a bit cleaner to set the width and height for what is written in the info, and then += what's missing ?

@@ +2702,2 @@
   /* now configure ther pools */
+  if (!v4l2object->pool && !gst_v4l2_object_setup_pool (v4l2object, caps))

Hmm, that seems wrong. If caps have changed you won't update the caps in the pool parameter.

::: sys/v4l2/gstv4l2sink.c
@@ +702,3 @@
+    obj->update_crop = FALSE;
+  }
+

You should not change what has been set in the property. Buffer pools cropping is mandatory and relative to the configured size, while the property cropping is optional and relative to the caps size. Both cropping need to be combined, right now the property cropping is overridden.

We also ignore extra cropping set in the GstVideoCropMeta on the buffer. I think the extra cropping there should be overridden by the value set in the properties.
Comment 11 Nicolas Dufresne (ndufresne) 2014-05-08 19:54:28 UTC
mfcdec has been removed.