GNOME Bugzilla – Bug 793412
msdk: session close failure if there are multiple sessions in a pipeline
Last modified: 2018-03-08 19:42:25 UTC
Reproduce step: gst-launch-1.0 filesrc location=video.mp4 ! qtdemux ! h264parse ! msdkh264dec ! msdkh264enc ! msdkh264dec ! fakesink When this pipeline finishes playing(or press Ctrl-C), one of SessionClose fails. Make sure that child sessions are closed before closing a parent session.
Created attachment 368816 [details] [review] msdk: manage child sessions on parent GstMsdkContext Sometimes parent context is released before its children get released. In this case MFXClose of parent session fails. To make sure that child sessions are closed before closing a parent session, Parent context needs to manage child sessions and close them first when it's released. ----------------------------------- Note that this patch is based on patches in bug #793413
Review of attachment 368816 [details] [review]: ::: sys/msdk/gstmsdkcontext.c @@ +216,3 @@ +{ + mfxSession _session = session; + MFXDisjoinSession (_session); Ideally, it should make sure there is no active task. According to spec, if this API returns MFX_WRN_IN_EXECUTION: "Active tasks are in execution or in queue. Wait for the completion of the tasks and then call this function again." @@ +292,3 @@ priv->job_type = parent_priv->job_type; + parent_priv->child_session_list = + g_list_append (parent_priv->child_session_list, priv->session); It is recommended to use g_list_prepend whenever possible. https://developer.gnome.org/glib/stable/glib-Doubly-Linked-Lists.html#g-list-append
(In reply to sreerenj from comment #2) > Review of attachment 368816 [details] [review] [review]: > > ::: sys/msdk/gstmsdkcontext.c > @@ +216,3 @@ > +{ > + mfxSession _session = session; > + MFXDisjoinSession (_session); > > Ideally, it should make sure there is no active task. > According to spec, if this API returns MFX_WRN_IN_EXECUTION: > "Active tasks are in execution or in queue. Wait for > the completion of the tasks and then call this > function again." Would that lead to a deadlock? As the list is iterated sequentially, perhaps the dependency chain might not follow the sequence and we would wait for a task forever. > > @@ +292,3 @@ > priv->job_type = parent_priv->job_type; > + parent_priv->child_session_list = > + g_list_append (parent_priv->child_session_list, priv->session); > > It is recommended to use g_list_prepend whenever possible. > https://developer.gnome.org/glib/stable/glib-Doubly-Linked-Lists.html#g-list- > append
(In reply to sreerenj from comment #2) > Review of attachment 368816 [details] [review] [review]: > > ::: sys/msdk/gstmsdkcontext.c > @@ +216,3 @@ > +{ > + mfxSession _session = session; > + MFXDisjoinSession (_session); > > Ideally, it should make sure there is no active task. > According to spec, if this API returns MFX_WRN_IN_EXECUTION: > "Active tasks are in execution or in queue. Wait for > the completion of the tasks and then call this > function again." > I've read it on the document, but feels that it could be risky as Victor said. Do you really want to handle the MFX_WRN_IN_EXECUTION? If so, I have to think again from the scratch for this issue, because current patch is not good for doing that.
I think at least it should throw some warning so that we can see whether the pipeline is hitting this issue. How about filing a separate bug so that we can track it after 1.14 release?
(In reply to sreerenj from comment #5) > I think at least it should throw some warning so that we can see whether the > pipeline is hitting this issue. How about filing a separate bug so that we > can track it after 1.14 release? Good.
Created attachment 369403 [details] [review] msdk: manage child sessions on parent GstMsdkContext Sometimes parent context is released before its children get released. In this case MFXClose of parent session fails. To make sure that child sessions are closed before closing a parent session, Parent context needs to manage child sessions and close them first when it's released. ------------ rebased.
Review of attachment 369403 [details] [review]: Pushed as commit c9faf0d612d7e73fbca16f1c227b181d851c5ae9