GNOME Bugzilla – Bug 796539
vaapiencoder: (baseclass): wrap in a while loop g_cond_wait() as documented
Last modified: 2018-11-03 15:54:55 UTC
Created attachment 372602 [details] Screenshot of gstreamer's log I developed streaming application with gstreamer-1.10.4 and used vaapi to accelerate decoding/encoding. It streams video encoded to ts stream with h264 format. But streaming stops after random period of time when it is started. Constantly I get error gst_vaapi_encoder_put_frame: failed to allocate coded buffer and gst_vaapiencode_handle_frame: failed to encode frame xxx (status -2). After some debugging I found the reason for this. It seems that problem is in gst_vaapi_encoder_create_coded_buffer function in gstvaapiencoder.c. In this line g_cond_wait (&encoder->codedbuf_free, &encoder->mutex); should be placed in loop as spurious wakeup can occur. This is explained in glib documentation: https://developer.gnome.org/glib/stable/glib-Threads.html#g-cond-wait . I putted g_cond_wait with codedbuf_proxy check in a loop and forced it to retry and wait for the buffer to be available. Then I recompilled gstreamer-vaapi. Now it works perfectly without stopping. I can submit a patch on request.
(In reply to Ahmed K from comment #0) > I can submit a patch on request. Yes, please.
Created attachment 372603 [details] [review] Patch for gstreamer-vaapi-1.10.4
Review of attachment 372603 [details] [review]: Ok, this isn't the right fix, but clearly a cond_wait without a condition loop is buggus. g_cond_wait() can unblock for no reason, you must wrap it up in a look that checks a condition. Is that still like this in 1.14+ ?
(In reply to Nicolas Dufresne (ndufresne) from comment #3) > Review of attachment 372603 [details] [review] [review]: > > Ok, this isn't the right fix, but clearly a cond_wait without a condition > loop is buggus. g_cond_wait() can unblock for no reason, you must wrap it up > in a look that checks a condition. Is that still like this in 1.14+ ? That code hasn't changed since. Thus, your patch looks promising. Would be great if you do it for master and following the contributing rules: https://gstreamer.freedesktop.org/documentation/contribute/index.html#patch-format
Okay, I will upload the right patch. Thanks!
Created attachment 372663 [details] [review] Patch for gstreamer-vaapi master branch
Review of attachment 372663 [details] [review]: thanks a lot for both patches. Is it possible to merge only one in both branches? to avoid, as possible, the diversion. One more thing, your patches don't follow the gstreamer code stylte: https://gstreamer.freedesktop.org/documentation/frequently-asked-questions/developing.html#what-is-the-coding-style-for-gstreamer-code
Comment on attachment 372603 [details] [review] Patch for gstreamer-vaapi-1.10.4 I assume this patch is obsolete, right?
Review of attachment 372663 [details] [review]: ::: gst-libs/gst/vaapi/gstvaapiencoder.c @@ +438,3 @@ + g_cond_wait (&encoder->codedbuf_free, &encoder->mutex); + codedbuf_proxy = gst_vaapi_coded_buffer_proxy_new_from_pool (pool); + if (codedbuf_proxy) I don't think codedbuf_proxy can be the flag to check if the cond signaled falsely. We need an extra flag in _coded_buffer_proxy_released_notify() to check in gst_vaapi_encoder_create_coded_buffer() Though, that is not clean. We would need a new API in GstVaapiVideoPool to do an async pop in its queue.
(In reply to Víctor Manuel Jáquez Leal from comment #8) > Comment on attachment 372603 [details] [review] [review] > Patch for gstreamer-vaapi-1.10.4 > > I assume this patch is obsolete, right? Yes, the old patch is obsolete now.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer-vaapi/issues/99.