GNOME Bugzilla – Bug 719791
v4l2sink: enable zero-copy path between mfcdec and v4l2sink io-mode=mmap
Last modified: 2014-05-08 19:54:28 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.
Created attachment 263410 [details] [review] v4l2bufferpool: mark mapped memory as GST_MEMORY_FLAG_PHYSICALLY_CONTIGUOUS
Created attachment 263411 [details] [review] v4l2sink: make sure the v4l2 buffer pool is inactive before to call set_config
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
Created attachment 263413 [details] [review] v4l2sink: check returned value of gst_buffer_pool_set_active
Created attachment 263414 [details] [review] v4l2: add support for GstVideoAlignment and cropping
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.
Review of attachment 263411 [details] [review]: Looks good
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 ?
Review of attachment 263413 [details] [review]: Looks more robust this way.
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.
mfcdec has been removed.