v4l2src regression in git master

[Nicolas Dufresne]

Hi Everyone,

Sebastien mention to me last week that the state of v4l2src was not good 
in git master. I did some investigation while on a plane yesterday. What 
I notice have changed is the following (note that I'll file bugs about 
each of these soon, I just wanted to group all these issues together as 
the link might not be evident) :

Decodebin systematically cause a reconfigure after first frame. This 
cause GstBaseSrc to re-run allocation query. This is not really a bug, 
but in fact the effect of having another bug fixed. During the second 
pool configuration, the pool is already active. This would fail in the 
past (but we were ignoring return value, making it work by accident most 
of the time). In recent pool, we are a bit more tolerant, in the sense 
that succeed the configuration if the pool is active and the 
configuration have not changed. We do this using equality on the 
structure, which is too strict since adjustment are made by the pool. 
The pool also do one more step in trying to succeed setting the new 
configuration, which is to stop the pool. Note that at this point, the 
allocation query has already run, hence the pipeline should have been 
drained. Though, because the check is too strict, this lead to an 
unfortunate reallocation of the buffers. On V4L2 side, this also mean we 
restart streaming, which cause additional delay between the first and 
second frame.

The equality check is obviously too strict. I have two proposition to 
solve this issue. The first solution is that when the pool is active, 
and configuration does not match, I would simply return FALSE. The 
caller will then get new configuration and validate it. If it's still 
compatible (which is most likely), it will simply call again 
set_config() with the configuration which is equal and succeed. In that 
scenario, the pool will never try to stop itself in set_config(), which 
removes the unwanted side effect and the set_config() documentation is 
perfectly respected. This would also mean that renegotiation of 
allocation does not happen unless the element support it. Surprisingly, 
this is the old behaviour, since the new configuration was never taken 
into account unless a new pool was selected.

The second solution would be to relax this check, checking for 
compatible configuration. Though this check remains a grey zone. The 
generic method that we added for that was not meant to be a perfect 
compatibility, but just a convenience function that cover the most 
common case. I'm starting to think it's not an ideal approach, but it is 
something that could work too. This would allow somehow implicit 
reconfiguration when the pool didn't changed.

Now, this isn't the only thing that have changed. For reason I still 
don't understand, the GstAdapter used in the jpegdec internal parser 
started to randomly copy GstBuffers, which lead to calling 
v4l2mem_share(). I found that the implementation in GstV4l2Allocator is 
currently broken and lead to the memory to being unmmap() too early 
hence lead to crash. This does not happen with raw format as they are 
expected to be framed. This I'll fix in both master and 1.4, I have 
prepared a patch already. There might be something we could add to the 
caps to get this parsing step optimized, as we know v4l2 buffer are 
framed (maybe jpegdec has no such optimization, I didn't check).

I also notice new issues with camerabin but my flight was not long 
enough, so this isn't investigated yet. We patched it for 1.4 to stop 
trying to reconfigure v4l2src (which is still not supported). But it 
seems that behaviour just came back. It is possibly related to the 
reconfigure event going through now. I think the right solution here 
would be to implement re-negotiation. I can't guaranty when this will 
happen, neither if it will happen for 1.6. So we should probably patch 
camerabin again if fixing the previous issues does not also fix this one.

If you are noticing other regression in this element, please let me 
know. We have enabled many kernel features that we found are badly 
proxied by libv4l2. We also don't have (yet) any automated test for this 
element. For that, kernel 3.19 now ship with vivid test drivers. I am 
looking forward using this within gst-validate scenario in a near future.

cheers,
Nicolas

p.s. I'll reply to this when bugs URI are available (some bugs already 
exist)