After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 754470 - v4l2videodec: send decoder stop command instead of queueing empty buffers
v4l2videodec: send decoder stop command instead of queueing empty buffers
Status: RESOLVED DUPLICATE of bug 733864
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.5.90
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-02 15:03 UTC by Philipp Zabel
Modified: 2015-09-03 13:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
v4l2_calls: add gst_v4l2_encoder_cmd and gst_v4l2_decoder_cmd (3.21 KB, patch)
2015-09-02 15:03 UTC, Philipp Zabel
needs-work Details | Review
v4l2videodec: send V4L2_DEC_CMD_STOP on EOS (944 bytes, patch)
2015-09-02 15:04 UTC, Philipp Zabel
needs-work Details | Review
v4l2videodec: only queue empty buffers if the decoder stop command fails (3.48 KB, patch)
2015-09-02 15:04 UTC, Philipp Zabel
needs-work Details | Review

Description Philipp Zabel 2015-09-02 15:03:53 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.
Comment 1 Philipp Zabel 2015-09-02 15:04:23 UTC
Created attachment 310512 [details] [review]
v4l2videodec: send V4L2_DEC_CMD_STOP on EOS
Comment 2 Philipp Zabel 2015-09-02 15:04:51 UTC
Created attachment 310514 [details] [review]
v4l2videodec: only queue empty buffers if the decoder stop command fails
Comment 3 Nicolas Dufresne (ndufresne) 2015-09-02 17:29:32 UTC
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.
Comment 4 Nicolas Dufresne (ndufresne) 2015-09-02 17:30:32 UTC
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)
Comment 5 Nicolas Dufresne (ndufresne) 2015-09-02 17:33:35 UTC
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 ?
Comment 6 Nicolas Dufresne (ndufresne) 2015-09-02 17:34:56 UTC
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.
Comment 7 Nicolas Dufresne (ndufresne) 2015-09-02 17:36:25 UTC
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 ***
Comment 8 Philipp Zabel 2015-09-03 08:21:09 UTC
(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.
Comment 9 Nicolas Dufresne (ndufresne) 2015-09-03 13:11:36 UTC
(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).
Comment 10 Philipp Zabel 2015-09-03 13:24:29 UTC
(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.