GNOME Bugzilla – Bug 733864
v4l2videodec: Implement EOS handling through V4L2_DEC_CMD_STOP
Last modified: 2016-06-07 21:19:19 UTC
This commend was not supported on MFC driver at the time this driver was written. The current MFC implement has it now, but have type ENC instead of DEC in the switch. This is needed to get the CODA driver to work. This will require implementing event support, hence opening the door for resolution change even too.
Note the since 3.18, it's no longer possible to use the old method since when we set the payload (bytesused) to 0, the videobuf2 framework now set bytesused to length.
Here a first draft patchset to add V4L2_ENC_CMD_STOP/V4L2_DEC_CMD_STOP support to the v4l2 gstreamer elements (as suggested on [1]), against gstreamer-1.4.4 with patches for v4l2encoder applied (see [2]): 0001-v4l2_calls-add-gst_v4l2_encoder_cmd-and-gst_v4l2_dec.patch 0002-v4l2videoenc-send-V4L2_ENC_CMD_STOP-instead-of-empty.patch 0003-v4l2videodec-send-V4L2_DEC_CMD_STOP-instead-of-empty.patch 0004-gstv4l2bufferpool-gstv4l2videodec-add-last-buffer-eo.patch Patch 1 to 3 are straight forward, patch 4 very preliminary as it depends on adding the V4L2_BUF_FLAG_LAST flag to the linux kernel, see 0001-coda-mark-last-buffer.patch for adding it to the coda driver for testing. The patch re-enables the queueing of empty buffers to trigger the code driver (without this the code in coda_dqbuf() is never reached). With this patches applied a decoding pipeline with gstreamer-1.4.4 on i.mx6 stops with correct EOS handling. Regards, Peter [1] http://www.spinics.net/lists/linux-media/msg84390.html [2] https://bugzilla.gnome.org/show_bug.cgi?id=728438
Created attachment 293100 [details] [review] v4l2_calls: add gst_v4l2_encoder_cmd and gst_v4l2_decoder_cmd
Created attachment 293101 [details] [review] v4l2videoenc: send V4L2_ENC_CMD_STOP instead of empty buffers
Created attachment 293102 [details] [review] v4l2videodec: send V4L2_DEC_CMD_STOP instead of empty buffers
Created attachment 293103 [details] [review] gstv4l2bufferpool/gstv4l2videodec: add last buffer/eos detection
Created attachment 293104 [details] [review] coda: mark last buffer
Review of attachment 293100 [details] [review]: ::: sys/v4l2/v4l2_calls.c @@ +1150,3 @@ + (_("Failed to send encoder command %u with flags %u for '%s'."), + cmd, flags, v4l2object->videodev), GST_ERROR_SYSTEM); + return FALSE; ENOTTY should not be fatal. We just need to know and fallback. To be honest, I would not add a helper for these
Review of attachment 293101 [details] [review]: This isn't in master yet, please propose this into proper bug report.
Review of attachment 293102 [details] [review]: This isn't right. First we don't want to replace, backward compatibility is as important in userspace library that it is in kernel. Then the patch will make you loose few buffers at the end of the playback. There is a good reason why there is all this synchronization between threads. The problem is rather simple: a) Send CMD_STOP a') If CMD_STOP returned ENOTTY, fallback to the empty payload buffer loop b) wait for the processing thread to have left c) return ::: sys/v4l2/gstv4l2videodec.c @@ +299,3 @@ GST_DEBUG_OBJECT (self, "Finishing decoding"); +#if 0 Eh ? @@ +718,3 @@ + case GST_EVENT_EOS: + GST_DEBUG_OBJECT (self, "got EOS event, send V4L2_DEC_CMD_STOP"); + gst_v4l2_decoder_cmd(self->v4l2output, V4L2_DEC_CMD_STOP, 0); Ok, you didn't not understood what the virtual method _finish() is doing. Finish method is called by the base class when EOS event is received. The implementation should drain sycnhronously the decoder and then return. At the end of the drain the processing thread should be stopped.
Review of attachment 293103 [details] [review]: ::: sys/v4l2/gstv4l2bufferpool.c @@ +1049,3 @@ +#ifndef V4L2_BUF_FLAG_LAST +#define V4L2_BUF_FLAG_LAST 0x00100000 +#endif Hmm, I think it's a bit soon for this one, has there been a patch submitted ? @@ +1078,3 @@ + if (group->buffer.flags & V4L2_BUF_FLAG_LAST) { + goto eos_buffer; The EOS buffer may have a payload. The current EOS buffer detection is currently done in _process() method. We currently detect empty payload as being EOS. The FLAG_LAST may or may not have a payload, hence require special handling. ::: sys/v4l2/gstv4l2videodec.c @@ +299,3 @@ GST_DEBUG_OBJECT (self, "Finishing decoding"); +#if 1 The should not be there.
Review of attachment 293104 [details] [review]: To the linux-media ML please.
(In reply to comment #1) > Note the since 3.18, it's no longer possible to use the old method since when > we set the payload (bytesused) to 0, the videobuf2 framework now set bytesused > to length. Small update on this one. This was not a choice but a regression. Patches as been submitted: Framework flag https://patchwork.kernel.org/patch/5500311/ MFC fix https://patchwork.kernel.org/patch/5500321/ CODA fix https://patchwork.kernel.org/patch/5518741/ It is still recommended to move forward with CMD_STOP. I'd like to clarify that I want this to be backward compatibly at least for 1 or 2 more version of GStreamer. For more context, both of these driver signal currently signal the EOS through an event after sending the last buffer. Though this method is slightly controversial, since the EOS is meant to signal the the OUTPUT queue of a decoder (M2M or not) has been processed. Sending EOS only after the last CAPTURE buffer has been dequeued is slightly off the spec here. This is to be addressed first on linux-media ML.
*** Bug 754470 has been marked as a duplicate of this bug. ***
Created attachment 310575 [details] [review] v4l2videodec: add gst_v4l2_decoder_cmd helper
Created attachment 310576 [details] [review] v4l2videodec: use decoder stop command instead of queueing empty buffers
Please mark as "obsolete" any patches that are not useful here anymore (it helps me doing reviews).
Unfortunately I can't edit details on patches that I didn't submit myself. I think all but the last two attachments are obsolete.
Comment on attachment 293103 [details] [review] gstv4l2bufferpool/gstv4l2videodec: add last buffer/eos detection We have EPIPE handling now, which is imho better then using FLAG_LAST.
Comment on attachment 293104 [details] [review] coda: mark last buffer This one was a kernel patch ;-P So done, I've think your two last are the one remaining.
Review of attachment 310576 [details] [review]: ::: sys/v4l2/gstv4l2videodec.c @@ +344,3 @@ + GST_OBJECT_LOCK (decoder->srcpad->task); + GST_TASK_WAIT (decoder->srcpad->task); + GST_OBJECT_UNLOCK (decoder->srcpad->task); gst_pad_task_stop() here instead. Actually, just remove the _unlock() call down there. @@ +349,3 @@ + /* otherwise keep queuing empty buffers until the processing thread has + * stopped, _pool_process() will return FLUSHING when that happened */ + while (ret == GST_FLOW_OK) { I actually found that MFC behaviour have changed over time. In the past we would have to push an empty buffer for each buffer to come out, but now it actually run the cmd stop routine. So only one empty buffer would do. Anyway, that lead to strange behaviour, specially with the _unlock() all that flush everything, we sometimes loose buffers now. Maybe we should just drop it in the end ...
Do you mean I should use gst_task_join(decoder->srcpad->task) to stop and wait for completion? Unfortunately that will set the task state to GST_TASK_STOPPED, which will stop the task func immediately or after the next work item (in this case one frame). I want to wait until all remaining frames are dequeued and gst_v4l2_allocator_dqbuf stops the pad task by returning GST_FLOW_EOS, after receiving -EPIPE from the VIDIOC_DQBUF ioctl.
(In reply to Philipp Zabel from comment #22) > Do you mean I should use gst_task_join(decoder->srcpad->task) to stop and > wait for completion? > Unfortunately that will set the task state to GST_TASK_STOPPED, which will > stop the task func immediately or after the next work item (in this case one > frame). I want to wait until all remaining frames are dequeued and > gst_v4l2_allocator_dqbuf stops the pad task by returning GST_FLOW_EOS, after > receiving -EPIPE from the VIDIOC_DQBUF ioctl. I see, you are right. Though, you can't just wait like this, as the task might be signalled without reaching the state you are expected. I see in the gsttask.c that they loop on a running boolean, which seems public. That might be safer no ?
Review of attachment 310576 [details] [review]: ::: sys/v4l2/gstv4l2videodec.c @@ +344,3 @@ + GST_OBJECT_LOCK (decoder->srcpad->task); + GST_TASK_WAIT (decoder->srcpad->task); + GST_OBJECT_UNLOCK (decoder->srcpad->task); So, here's exactly what need fixing. First, you use the wrong lock, then you need to spin over the condition you want to change. /* GstPad call gst_task_set_lock() using the stream lock */ GST_PAD_STREAM_LOCK (decoder->srcpad); while (decoder->srcpad->task->running) GST_TASK_WAIT (decoder->srcpad->task); GST_PAD_STREAM_UNLOCK (decoder->srcpad);
I don't think I used the wrong lock. The GST_TASK_WAIT expands to g_cond_wait(GST_TASK_GET_COND (task, GST_OBJECT_GET_LOCK (task)), so g_cond_wait first tries to unlock the object lock, which is what protects task->running and task->state. Also I think spinning on (task->running) won't terminate because gst_v4l2_video_dec_loop() only calls gst_pad_pause_task(). gst_pad_task_stop() is called from gst_v4l2_video_dec_finish() right after this code. How about this instead: GstTask *task = decoder->srcpad->task; /* If the decoder stop command succeeded, just wait until processing is * finished */ GST_OBJECT_LOCK (task); while (GST_TASK_STATE (task) == GST_TASK_STARTED) GST_TASK_WAIT (task); GST_OBJECT_UNLOCK (task); Or should the beach: label in gst_v4l2_video_dec_loop() be changed to call gst_pad_task_stop() if ret == GST_FLOW_EOS?
(In reply to Philipp Zabel from comment #25) > I don't think I used the wrong lock. The GST_TASK_WAIT expands to > g_cond_wait(GST_TASK_GET_COND (task, GST_OBJECT_GET_LOCK (task)), so > g_cond_wait first tries to unlock the object lock, which is what protects > task->running and task->state. You are right, I miss-read that. The other lock is a recursive lock, so it could not have been used for a GCond, sorry. > > Also I think spinning on (task->running) won't terminate because > gst_v4l2_video_dec_loop() only calls gst_pad_pause_task(). > gst_pad_task_stop() is called from gst_v4l2_video_dec_finish() right after > this code. > > How about this instead: > > GstTask *task = decoder->srcpad->task; > > /* If the decoder stop command succeeded, just wait until processing is > * finished */ > GST_OBJECT_LOCK (task); > while (GST_TASK_STATE (task) == GST_TASK_STARTED) > GST_TASK_WAIT (task); > GST_OBJECT_UNLOCK (task); > > Or should the beach: label in gst_v4l2_video_dec_loop() be changed to call > gst_pad_task_stop() if ret == GST_FLOW_EOS? I see, both ways sounds reasonable. With the second, we'd need to leave the thread for both EOS and ERROR, but not flushing. In general though, it's probably more efficient to keep that thread running as long as it's possibly needed again (recoverable). That's probably why going to pause is what makes sense. So I'd go for first option. I think the task handling in general is very hairy in v4l2videodec and that accessing the state like you suggest could help cleaning things up. I'll be looking forward this method if you aren't faster then me ;-P
Created attachment 328372 [details] [review] v4l2videodec: use decoder stop command instead of queueing empty buffers Only if the decoder stop command fails, keep queueing empty buffers to signal end of stream as before. This version waits until the decoder task state changes to paused after the EOS event.
Attachment 310575 [details] pushed as ddf7d9d - v4l2videodec: add gst_v4l2_decoder_cmd helper Attachment 328372 [details] pushed as 96d8235 - v4l2videodec: use decoder stop command instead of queueing empty buffers - none
This enables more decoders to be used, I think it's totally worth having on stable branch.
Branch: 1.8 Commit: 7625d3349bc609e47c3de637158497c17a8c5c8a Commit: 719b700b80386ce2512b6d6a7da07a2818d37b56