That sounds wrong. The FLUSH_START event should stop any tasks and empty any queues, then the FLUSH_STOP event should re-start them in a clean state.

It is not clear what exactly is the issue that you have found. If there is a bug, please let us know what it is that you are observing and how you would expect it to work instead.

What I observed is that.
When using the qtmultimedia to make a player, at the beginning of the video, it would start flushing. Then the task would be paused because the queue is flushing. Then stop at the end of sink event. After that, handle_frame() would start the src task again.
But before FLUSH_STOP is called, the QT sink would still refuse the new frame, and return FLUSHING in the gst_video_decoder_finish_frame() making the srcpad task paused again, but no one stop it again. So the srcpad task would stay in the paused state, never be started again.

Hmm, I think I understand what you mean. The real problem is that handle_frame() will only restart the task if it's STOPPED and will ignore it if it's PAUSED, so if gst_v4l2_video_dec_loop() gets a FLOW_FLUSHING from downstream and goes to 'beach', pausing the task, the task never gets a chance to restart.

Adding Thibault in CC as he reworked that part recently. I still don't understand the sequence that lead to this, can you provide a test, or some pseudo code ?

Review of attachment 353991 [details] [review]:

Waiting for the task to stop isn't really correct in flush_start, that I completly agree. So removing that make sense, but what will replace this ? Then, I have the impression that we just do thing the wrong way, why don't we just pause() the task instead, would't that simplify everything to only play with two task states (PLAYING, PAUSED) while the pipeline is in READY+ state ?

(In reply to George Kiagiadakis from comment #1)
> That sounds wrong. The FLUSH_START event should stop any tasks and empty any
> queues, then the FLUSH_STOP event should re-start them in a clean state.

In general, I think FLUSH_START simply "triggers" blocking operation to stop. While in FLUSH_STOP we do the cleanup and restart (all protected by the STREAM_LOCK).

Clearly the decoder isn't correct, but I think this patch is also incomplete.

(In reply to Randy Li (ayaka) from comment #0)
> Created attachment 353991 [details] [review] [review]
> patch for sink event
> 
> I found the QT would like to flush in beginning of the video,
> then the task of the srcpad would be paused twice with that.
> 
> The task of the srcpad would only start at the STOPPED state,
> so I think I have to stop the task at the end of the sink
> flushing.

Would you mind explaining though why stopping the task twice is an issue ?

The playing would be stopped there. When the CAPTURE is full of buffers, the OUTPUT may be blocked.
The problem is not the src pad is stopped twice, but once it is in the pause state, it won't be start again.

Are you able to retest this with master and this change ?


@@ -522,6 +552,7 @@ gst_v4l2_video_dec_handle_frame (GstVideoDecoder * decoder,
   GstFlowReturn ret = GST_FLOW_OK;
   gboolean processed = FALSE;
   GstBuffer *tmp;
+  GstTaskState task_state;
 
   GST_DEBUG_OBJECT (self, "Handling frame %d", frame->system_frame_number);
 
@@ -649,8 +680,8 @@ gst_v4l2_video_dec_handle_frame (GstVideoDecoder * decoder,
       goto activate_failed;
   }
 
-  if (gst_pad_get_task_state (GST_VIDEO_DECODER_SRC_PAD (self)) ==
-      GST_TASK_STOPPED) {
+  task_state = gst_pad_get_task_state (GST_VIDEO_DECODER_SRC_PAD (self));
+  if (task_state == GST_TASK_STOPPED || task_state == GST_TASK_PAUSED) {

(In reply to Nicolas Dufresne (stormer) from comment #9)
> Are you able to retest this with master and this change ?
> 
> 
> @@ -522,6 +552,7 @@ gst_v4l2_video_dec_handle_frame (GstVideoDecoder *
> decoder,
>    GstFlowReturn ret = GST_FLOW_OK;
>    gboolean processed = FALSE;
>    GstBuffer *tmp;
> +  GstTaskState task_state;
>  
>    GST_DEBUG_OBJECT (self, "Handling frame %d", frame->system_frame_number);
>  
> @@ -649,8 +680,8 @@ gst_v4l2_video_dec_handle_frame (GstVideoDecoder *
> decoder,
>        goto activate_failed;
>    }
>  
> -  if (gst_pad_get_task_state (GST_VIDEO_DECODER_SRC_PAD (self)) ==
> -      GST_TASK_STOPPED) {
> +  task_state = gst_pad_get_task_state (GST_VIDEO_DECODER_SRC_PAD (self));
> +  if (task_state == GST_TASK_STOPPED || task_state == GST_TASK_PAUSED) {

It solved the problem, but I need more time to verified this.

Ok, but this sounds promising. This fix is part on another that I'm not done yet (drain and renegotiation). But I agree the flushing sequence is far from ideal (waiting on thread in flush-start).

This is fix is merged now, let's assume this is fixed, and file new bugs as they are found.