GNOME Bugzilla – Bug 754470
v4l2videodec: send decoder stop command instead of queueing empty buffers
Last modified: 2015-09-03 13:24:29 UTC
Created attachment 310510 [details] [review] v4l2_calls: add gst_v4l2_encoder_cmd and gst_v4l2_decoder_cmd When v4l2videodec receives the EOS event on its sink, it should issue the V4L2_DEC_CMD_STOP decoder command to signal end of stream to the v4l2 mem2mem device. If the driver can handle the stop command, it should not be necessary to queue empty buffers at the end of the stream.
Created attachment 310512 [details] [review] v4l2videodec: send V4L2_DEC_CMD_STOP on EOS
Created attachment 310514 [details] [review] v4l2videodec: only queue empty buffers if the decoder stop command fails
Review of attachment 310510 [details] [review]: I'd like not to add more one time use function in v4l2_calls.h/c (in fact I'd like to drop that file in the long term. As this is specific to endoder/decoder, please keep that helper private to these element. Also, remove Signed-off-by in the commit message. In GStreamer project these mark are only used to indicated multiple contributors, we think this is redundant with the Author field.
Review of attachment 310512 [details] [review]: ::: sys/v4l2/gstv4l2videodec.c @@ +729,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); This should go in the _finish() function (and drain() which is not implemented)
Review of attachment 310514 [details] [review]: ::: sys/v4l2/gstv4l2videodec.c @@ +747,3 @@ + */ + self->queue_empty_buffers = FALSE; + } Again, I don't see any reason to split the EOS handling in here. @@ +835,3 @@ + + g_cond_init (&self->processing_cond); + self->queue_empty_buffers = TRUE; Don't we already have a condition for waiting for the thread to stop ?
Thanks for the patches, it needs a bit of work still, though I completely agree with what it is doing. I'll be waiting for and updated version.
Also, post your new version in the original bug (the one you submitted to last time). *** This bug has been marked as a duplicate of bug 733864 ***
(In reply to Nicolas Dufresne (stormer) from comment #5) > Review of attachment 310514 [details] [review] [review]: > > ::: sys/v4l2/gstv4l2videodec.c > @@ +747,3 @@ > + */ > + self->queue_empty_buffers = FALSE; > + } > > Again, I don't see any reason to split the EOS handling in here. Yes, now that you pointed it out it is quite obvious. > @@ +835,3 @@ > + > + g_cond_init (&self->processing_cond); > + self->queue_empty_buffers = TRUE; > > Don't we already have a condition for waiting for the thread to stop ? I guess I could use GST_OBJECT_LOCK (decoder->srcpad->task); GST_TASK_WAIT(decoder->srcpad->task); GST_OBJECT_UNLOCK (decoder->srcpad->task); in _dec_finish instead of adding this condition.
(In reply to Philipp Zabel from comment #8) > I guess I could use > GST_OBJECT_LOCK (decoder->srcpad->task); > GST_TASK_WAIT(decoder->srcpad->task); > GST_OBJECT_UNLOCK (decoder->srcpad->task); > in _dec_finish instead of adding this condition. Or wasn't gst_pad_stop_task() the way to go here ? (or at least what is used elsewhere ? (don't forget to update the patchset on the original bug please).
(In reply to Nicolas Dufresne (stormer) from comment #9) > (In reply to Philipp Zabel from comment #8) > > I guess I could use > > GST_OBJECT_LOCK (decoder->srcpad->task); > > GST_TASK_WAIT(decoder->srcpad->task); > > GST_OBJECT_UNLOCK (decoder->srcpad->task); > > in _dec_finish instead of adding this condition. > > Or wasn't gst_pad_stop_task() the way to go here ? (or at least what is used > elsewhere ? (don't forget to update the patchset on the original bug please). gst_pad_stop_task immediately stops the task and doesn't wait for the remaining buffers to be dequeued. I have added new patches to bug 733864.