GNOME Bugzilla – Bug 711498
v4l2: remove link between v4l2object type and v4l2bufferpool when managing qbuf/dqbuf
Last modified: 2013-11-11 16:51:47 UTC
Created attachment 259025 [details] [review] disentangle the link between v4l2object type and v4l2bufferpool v4l2object type was used to know if the pool was for capture or not. The assumption was that v4l2 device can only have one queue but v4l2 video decoder/encoder device could have two queues. The idea is to use the direction of the pad instead of v4l2object type to be more flexible. Later a v4l2dec could take benefit of this to have 2 v4l2bufferpool, one for sink pad to queue access units and an other to dequeue output frame. I have only been able to test this patch with a webcam (v4l2src), tests with v4l2sink are maybe needed.
Review of attachment 259025 [details] [review]: This change seems correct, in the sense it should not break anything, though I don't see that well the link with encoder/decoders. I'm currently working on implement such a decoder (my current target being Exynos) and the solution I was to use it to have two v4l2object. Micheal Olbrich (from Pengutronix) seems to have chosen the same solution for the V4l2Filter element. The proposed added internal API so far is gst_v4l_clone() to create a v4l2object reusing the file descriptor. This ensure that each object handles one queue. Let me know what you think of this, and you're intention, we might be able to collaborate on this. http://git-public.pengutronix.de/git-public/gst-plugins-good.git/ [mem2mem]
I agree. There is a lot of stuff in v4l2object that is different for the source and sink side. In my experience using just one v4l2object would make things a lot more complex.
I have read the code of gstv4l2filter. I agree that use 2 v4l2object is a better option than mine. My goal is to have a gstv4l2dec element (based on GstVideodec class). Nicolas it seems your are doing the same than us. Do you agree to share your code for Exynos ? so we can converge.
Sure, I was only to start this work tomorrow, indeed using GstVideoDecoder as a base class. So far I only have an experimental branch with the multi-planar support, I expect the decoder part to be mostly trivial. In first step, I've been doing some house-cleaning in the v4l element, which I should have for review by next week. I still need to discuss that, but I'd also like to remove v4l2_calls.* and merge that into gstv4l2object, as this is where it should be. You can find me on freenode #gstreamer as stormer, let's have a chat there and figure-out how to not duplicate the effort. For this particular bug, I'm not very keen to merge it right now, as I don't see what we would gain, or if we really need that. Let me know why you need that, or if we can just reject it ?
(In reply to comment #3) > I have read the code of gstv4l2filter. > I agree that use 2 v4l2object is a better option than mine. > > My goal is to have a gstv4l2dec element (based on GstVideodec class). > Nicolas it seems your are doing the same than us. Do you agree to share your > code for Exynos ? so we can converge. Note that Michael is also having something like that, not based on GstVideoDecoder though. Reminds me that I should create a bug for an asynchronous basetransform.
(In reply to comment #5) > Note that Michael is also having something like that, not based on > GstVideoDecoder though. Reminds me that I should create a bug for an > asynchronous basetransform. The v4l2filter is good for colortransform, scaling and other video transformation, but I feel like having a decoder based on GstVideoDecoder class is best for decoders. Obviously asynchronous basetransorm for this is kind of a must.
(In reply to comment #6) > The v4l2filter is good for colortransform, scaling and other video > transformation, but I feel like having a decoder based on GstVideoDecoder class > is best for decoders. Obviously asynchronous basetransorm for this is kind of a > must. v4l2filter also works for decoding. But it's missing all the little things provided by GstVideoDecoder. My long-term goal is three elements based on GstVideoDecoder, GstVideoEncoder and the not yet existing asynchronous basetransform.
(In reply to comment #7) > v4l2filter also works for decoding. But it's missing all the little things > provided by GstVideoDecoder. My long-term goal is three elements based on > GstVideoDecoder, GstVideoEncoder and the not yet existing asynchronous > basetransform. Well you'll soon gain the decoder part for free. The problem with GstVideoFilter is that it cannot be classified, which is bad for auto-pluging. What I had in mind for the long term, is to create few base class and then have specific element for let's say a Converter / Scaler / Effect etc. Due to the nature of encoders, it also possible we'll need a base class and instance for different format, otherwise configuration will be a mess. I also suspect that encoder setting might be hard to consolidate, thus there might be an instance per actual HW, I don't know yet. All this will require proper probing and auto-detection, but I guess you already know that.
For that also see bug #687182. I'm not sure a base class for specific filters makes sense, other than for some where negotiation or something else is very complicated (videoscale negotiation!), in general an interface should suffice. Also unrelated to this bug, let's try to keep it on-topic otherwise nobody will be able to follow this bug anymore (and create new bugs for other issues) ;)
And let's get rid of this bug, it doesn't make much sense on its own. Not fixing anything and incompatible with the patchsets of Nicolas and Michael. Let's wait until someone proposes a complete solution and then we can consider which one is best if there are still multiple approaches.