GNOME Bugzilla – Bug 752962
v4l2videodec: Add resolution change support
Last modified: 2018-01-08 22:34:24 UTC
I am implementing v4l2videodec video decoder support resolution change stream. The changes is list below. Please help to review. Will upload patch later. We also developing video decoder based on V4L2 M2M. I verify below on our video decoder. 1. Should subscribe V4L2_EVENT_EOS and V4L2_EVENT_SOURCE_CHANGE event for V4L2 video decoder. 2. Capture port should get the video format after got V4L2_EVENT_SOURCE_CHANGE. So capture buffer pool should activated in gst_v4l2_video_dec_loop (). 3. dqevent () will blocked until got one V4L2 event. 4. Will call dqevent () when got one zero size video frame buffer. and check the event, the event will be EOS event or SOURCE_CHANGE event. 5. Will gst_v4l2_object_stop () capture object if received SOURCE_CHANGE event, and active it again after get right video format.
Two main points: 1. Activate capture object until got SOURCE_CHANGE event. 2. Check event if got zero sized video buffer. Stop capture object if got SOURCE_CHANGE event, and activate it again after got changed video resolution.
About 1, it's not going to be trivial in the current architecture, but I would be please to see how you handle it. About 2, I agree. It will though break my plan of passing the header, and wait to see if the decoder is happy before letting data flow. If the header is part of that caps, that would have let us skip the decoder if it does not support certain flavour of a certain encoding (think MFC with many MPEG2 video stream, or with H264 stream with unpaired field). About 3, this is driver job, should have nothing special to do in GStreamer. Unless I'm mistaken ? About 4, zero payload buffers is there for backward compatibility. Make sure you also support "after EPIPE". There is also the LAST flag, but I believe hte EPIPE case is sufficient, even if a bit late (next gst_v4l2_video_dec_loop () call). Note that I'll probably have to delay these change to the next development cycle, it's very late now in the 1.5/1.6 process to make such a change. The only way around is if you can prove you didn't break support for existing implementation. Known to be used are MFC (Exynos 4 and 5), this is known to work with hardkernel kernels. CODA is also known to work from upstream with very little patches.
Another note, don't forget about the stream change support, would make no sense to not add this too. Very often in GStreamer we know if advanced that the stream properties have changes (even for resolution change). So what we do in this case is drain the decoder, reset(), setup() and start again. Very few decoder do support a complete format change asynchronously. So if we know in advanced, better do it synchronously.
What's mean of stream change? Do you mean video compressed format changed? such as H264 changed to MPEG4? If so, it is out range of decoder, it should be decoderbin's job. As maybe need load another element to do video decoder. My currently fix is for decoder output change. The change can be raw video resolution or format (NV12 or I420).
For most format (compressed format) the width/height is included in the input caps.
(In reply to kevin from comment #4) > What's mean of stream change? Do you mean video compressed format changed? > such as H264 changed to MPEG4? If so, it is out range of decoder, it should > be decoderbin's job. As maybe need load another element to do video decoder. I mean the header changes (for the AVC case, and any format that have the header in the caps as codec_data).
(In reply to Nicolas Dufresne (stormer) from comment #6) > (In reply to kevin from comment #4) > > What's mean of stream change? Do you mean video compressed format changed? > > such as H264 changed to MPEG4? If so, it is out range of decoder, it should > > be decoderbin's job. As maybe need load another element to do video decoder. > > I mean the header changes (for the AVC case, and any format that have the > header in the caps as codec_data). For video decoder element input caps change, seems should be handled in gstvideodecoder base. Is it ok to file another ticket to implement? This ticket only handle video decoder output change without set_caps (). Such as H264 in byte-stream format, the header change detect by video decoder.
Which is the best indicator to indicate V4L2 user need check event. zero payload buffer, EPIPE or LAST flag? As dqevent () will be block call, will call dqevent () after got one indicator. I use zero payload buffer currently.
(In reply to kevin from comment #8) > For video decoder element input caps change, seems should be handled in > gstvideodecoder base. Is it ok to file another ticket to implement? > > This ticket only handle video decoder output change without set_caps (). > Such as H264 in byte-stream format, the header change detect by video > decoder. I don't want half a renegotiation supported. If a demuxer spread the width/height field in the encoded caps, you'll have a set_caps(), if it does not, you won't. If you only implement one of these two, then renegotiation will be half broken. (In reply to kevin from comment #8) > Which is the best indicator to indicate V4L2 user need check event. zero > payload buffer, EPIPE or LAST flag? As dqevent () will be block call, will > call dqevent () after got one indicator. I use zero payload buffer currently. First rule, never let you program block on an ioctl() (Currently G_FMT(CAPTURE) is the exception, and you might be about to remove it). Instead, you should poll for read after registering the event. The code might need a little refactoring. Having the poll buried in the buffer pool isn't ideal, I do have plan for 1.7/1.8 to rewrite the queue handling in order to reduce the amount of abstraction here.
(might need to make the FD non blocking if it's not already)
Created attachment 308432 [details] [review] Draft patch. Only for explain the mothod. Draft patch to explain the method. Still need move poll out of buffer pool to let dqevent () use poll. Still need unblock poll when flush and stop. The fixes will be in another patch.
Review of attachment 308432 [details] [review]: Only reviewed v4l2_call.c ::: sys/v4l2/v4l2_calls.c @@ +506,3 @@ +gboolean +gst_v4l2_subscribe_event (GstV4l2Object * v4l2object) I'd prefer if you create a helper to register 1 event, not a hidden selection. @@ +513,3 @@ + e = v4l2object->element; + + GST_DEBUG_OBJECT (e, "subscribe event"); Drop e, it fits on single line, and it's the only use. @@ +521,3 @@ + sub.type = V4L2_EVENT_SOURCE_CHANGE; + if (v4l2_ioctl (v4l2object->video_fd, VIDIOC_SUBSCRIBE_EVENT, &sub) < 0) + goto failed; This is not backward compatible. This will post on error on the bus for older driver. Remember this event it new in the kernel.
Thanks, how about other implement? Is it reasonable?
Review of attachment 308432 [details] [review]: The only problem with this approach is that you dqevent or dqbuffer, it's not really done in parallel. I'd like so see an implementation where event behave like events. ::: sys/v4l2/gstv4l2object.c @@ +2906,3 @@ + +// if ((res = gst_v4l2_object_poll (v4l2object)) != GST_FLOW_OK) + // goto poll_failed; We need cancellable polling, but I already mention this. It's also why this require more work (some refactoring it needed).
Yes, I will make one patch to move poll out of buffer pool. How to keep SOURCE_CHANGE backward compatible?
(In reply to kevin from comment #15) > Yes, I will make one patch to move poll out of buffer pool. If you can explain your design, that might require few iterations. > > How to keep SOURCE_CHANGE backward compatible? You should first detect that it's not supported. Normally registering an event that does not exist shall return -1 with errno ENOTTY (probably that we don't care, any error means not supported really). Then if it's not supported, I expect the code to behave (you can still more some of it to the loop) like before, which means after sending the header, simply calling G_FMT (acquire_format()) and expecting it to reply immediately, and fail if no answer.
poll should based on buffer pool as OUTPUT and CAPTURE need different poll when dequeue buffer. For dequeue event, do need poll? if so, should add one poll in v4l2object? How driver know unblock OUTPUT and CAPTURE poll? poll only depend on fd.
When you poll, you get flags as result, can be POLLIN (you can queue buffer, applicable to OUTPUT), POLLOUT (you can dequeue buffer, applicable to CAPTURE) and finally POLLPRI (you can dequeue event, for both). GstPoll API need to be enhanced so we know the the distinction. I would add a method (unix specific) where we actually pass the flags.
Created attachment 309390 [details] [review] Draft patch. Only for explain the mothod. Attached patch for explain the implement method. Add one poll in v4l2object to dequeue event. every v4l2 buffer pool still need one poll to dequeue buffer. Disable dequeue event if can't subscribe event. Is this implementation reasonable?
The approach is reasonable, unfortunately it's implementation seems too specific, and mostly hard to use. What I foresee for the 1.7 branch that will start soon, is a refactoring of the queue handling. I'll provide more details as I settle the design. Big lines are: a) Introducing a seperate thread, to handle immediate queuing and dequeuing of pending buffer along with dequeuing and propagation of events. This way, we can keep the driver active, and create a saner processing loop that better fit with GStreamer. That thread would prepare and queue work for loop function to pick and handle it while feeding back recycled buffer to the driver immediatly. b) The rest of the queuing and streaming state handling will be move to the allocator, this way we can instantiate multiple queue per allocator, allowing to renegotiate the allocation queues, and adding support for renegotiation in a scenario where filters move from passthrough to non-passthrough mode.
How many threads will be used for CAPTURE port? Below process will block: 1. get recycled video buffer from video decoder base pool. 2. dequeue video buffer from V4L2 driver. 3. dequeue event from V4L2 driver. Do those process need separate thread to avoid any dependence?
(In reply to kevin from comment #21) > How many threads will be used for CAPTURE port? Below process will block: > > 1. get recycled video buffer from video decoder base pool. > 2. dequeue video buffer from V4L2 driver. > 3. dequeue event from V4L2 driver. > > Do those process need separate thread to avoid any dependence? Of course not, since polling return you a flag telling you which next operation won't block. This allow you to effectively poll for everything. Then of course you have the usual streaming thread, which may block (this is normal in GStreamer to block the streaming thread). This new thread might be optional in fact, I need to work on the design (code is not a design fwiw) but I believed it could replace (and centralize the code) for the internal thread required in encoder/decoder and eventually converters. My main point, and why I'm not opting for your solution is that it makes no sense to call gst_buffer_pool_acquire() to discover that there is EVENT pending. It's a pain to handle, custom return value for acquire() should go away.
I agree with you, dequeue buffer or dequeue event can be decided by return of polling. So dequeue event can keep alone with dequeue buffer. Do need wait event before g_fmt () of CAPTURE? or driver should wait and ensure get right video format?
(In reply to kevin from comment #23) > Do need wait event before g_fmt () of CAPTURE? or driver should wait and > ensure get right video format? These ioctl() are meant to be synchronous, but deterministic (they should not really block longer then the time is takes to read/write the information). This is where pushing a head, and immediatly doing g_fmt() is not correct (and why the V4L2_EVENT_SOURCE_CHANGE is needed here).
Created attachment 309675 [details] [review] Draft patch. Only for explain the mothod. Dequeue event or dequeue buffer based on poll return.
Created attachment 309676 [details] [review] Add one poll API to check event.
Do the method right for process V4L2 driver event?
Review of attachment 309676 [details] [review]: Nah, I'd make a more generic method, where you actually pass the flags (unix specific) specially that PRI is already arch specific.
Some notes about the proposal. * The EOS handling is wrong As best on EOS event we can free early the output queue, but we still need to wait for all buffer to be queued from the capture queue. We could though run the drain procedure without emitting the stop command. As it is now, it will drop the capture queue, if I read correctly. * Wrong value in enum We subcribe to SOURCE_CHANGE and check for FRAME_SYNC. * We flush rather then draining When we handle a resoltion change, it is expected to drain the decoder, to not loose any of the buffers, right now the code flush. * A poll function that give you what unblocked This would reduce the amount of code change that happens all over the place. * Patch way too big Split you patch in smaller changes.
Created attachment 366522 [details] [review] v4l2videodec: Add dynamic resolution change support This implements a "big hammer" reallocation method. We effectively drain and stop both side of the decoder and restart. This though is the most generic method. This change should enable on most drivers adaptive streaming.
Attachment 366522 [details] pushed as 97985a3 - v4l2videodec: Add dynamic resolution change support