GNOME Bugzilla – Bug 774030
postproc: return valid error when gst_pad_push fails
Last modified: 2016-11-08 07:31:31 UTC
Currently, when gst_pad_push fails it always return GST_FLOW_ERROR. I found this could lead to deadlock during seek, which happens very unusually. I don't know why it returns same error value, but IMO, it should be modified. @Victor, with the media you downloaded from me last week, you can reproduce deadloc. Just play it and seek repidly several times.
Created attachment 339220 [details] [review] postproc: return GST_FLOW_FLUSHING during flushing Returning GST_FLOW_ERROR always when gst_pad_push fails might lead to deadlock during seek sometimes. Return exact error value when flush happens.
Review of attachment 339220 [details] [review]: ::: gst/vaapi/gstvaapipostproc.c @@ +812,2 @@ GST_ERROR_OBJECT (postproc, "failed to push output buffer to video sink"); + return GST_FLOW_ERROR; Why not "return ret"? @@ +813,3 @@ + return GST_FLOW_ERROR; + } else + return GST_FLOW_FLUSHING; {} @@ +891,2 @@ GST_ERROR_OBJECT (postproc, "failed to push output buffer to video sink"); + return GST_FLOW_EOS; Why not "return ret"? @@ +892,3 @@ + return GST_FLOW_EOS; + } else + return GST_FLOW_FLUSHING; {}
(In reply to Sebastian Dröge (slomo) from comment #2) > Review of attachment 339220 [details] [review] [review]: > > ::: gst/vaapi/gstvaapipostproc.c > @@ +812,2 @@ > GST_ERROR_OBJECT (postproc, "failed to push output buffer to video > sink"); > + return GST_FLOW_ERROR; > > Why not "return ret"? > > @@ +813,3 @@ > + return GST_FLOW_ERROR; > + } else > + return GST_FLOW_FLUSHING; > > {} > > @@ +891,2 @@ > GST_ERROR_OBJECT (postproc, "failed to push output buffer to video > sink"); > + return GST_FLOW_EOS; > > Why not "return ret"? > > @@ +892,3 @@ > + return GST_FLOW_EOS; > + } else > + return GST_FLOW_FLUSHING; > > {} Maybe I was afraid a sort of regression? :) But I totally agree that just do "return ret;"
Well, you would've uncovered more bugs in the existing code there then. It is the right thing to do in this context ;)
What Sebastian said and please log out the return of gst_flow_get_name() for debugging purposes :)
Created attachment 339296 [details] [review] postproc: return error value coming out from gst_pad_push instead of just GST_FLOW_ERROR Returning GST_FLOW_ERROR always when gst_pad_push fails might lead to deadlock during seek sometimes.
This should be pushed for 1.10 and perhaps 1.8