GNOME Bugzilla – Bug 726329
vp8enc: Add support for caps renegotiation
Last modified: 2015-07-06 17:29:55 UTC
Created attachment 271874 [details] [review] vp8enc: Change GMutex for GRecMutex Attached patch adds support for caps renegotiation at sink pad. This way screen size changes can be handled by the encoder. Additionally I attach a patch that changes GMutex for GRecMutex to avoid release the lock for calling other functions, avoiding possible race conditions.
Created attachment 271875 [details] [review] vp8enc: Allow caps renegotiation
Created attachment 271877 [details] [review] vp8enc: Change GMutex for GRecMutex
Created attachment 271878 [details] [review] vp8enc: Allow caps renegotiation
I'm really not fan of replacing fast mutex with recursive mutex, unless strictly required. Would it be possible to elaborate ?
It was my first idea, but then I realized that the lock should be released to call gst_video_encoder_finish_frame this could end in race conditions during the caps renegotiation. If you think that is better not to change to recursive mutex I can rework it, but I think that the risk of race conditions will be there.
(In reply to comment #5) > It was my first idea, but then I realized that the lock should be released to > call gst_video_encoder_finish_frame this could end in race conditions during > the caps renegotiation. If you think that is better not to change to recursive > mutex I can rework it, but I think that the risk of race conditions will be > there. Well no, you hare holding the pads stream lock already in the function (it's a chain handler). That lock is only there to protect against a property change. hence a fast mutex is preferred. I would remove that lock and use the object if it as me doing the patch.
Sorry, object *lock*. But for now I would propose just to remove the mutex change. Though if you want to give it a try, GST_OBJECT_LOCK()/GST_OBJECT_UNLOCK() is what I've referring to.
Comment on attachment 271878 [details] [review] vp8enc: Allow caps renegotiation As said, just going back to normal mutex of OBJECT_LOCK is needed.
Ok, I'll change the patch to avoid the RecMutex. I haven't noticed that the stream lock is held in both functions.
Created attachment 272123 [details] [review] vp8enc: Allow caps renegotiation
Review of attachment 272123 [details] [review]: ::: ext/vpx/gstvp8enc.c @@ +1516,3 @@ + g_mutex_lock (&encoder->encoder_lock); + vpx_codec_destroy (&encoder->encoder); + encoder->inited = FALSE; Normally I rename _finish() into _drain() (removing the lock), and implement _finish() that simply lock and call _drain(). Then I can use _drain() whenever needed. Let me know if you still have time, otherwise I can do that.
I'll do it tomorrow. Thanks for the review.
Created attachment 272655 [details] [review] vp8enc: Allow caps renegotiation
Created attachment 272656 [details] [review] vp8enc: Allow caps renegotiation
Hi, have you got time to review the patch, any changes or fixes that should I add?
Hi, i just tried this patch because i needed to renegotiate caps with vp8enc. It works perfectly for me (gstreamer 1.2.4, android version). Thank you.
Hi guys, Any options of getting this accepted? Thanks
Review of attachment 272656 [details] [review]: Looks good to me.
Review of attachment 272656 [details] [review]: ::: ext/vpx/gstvp8enc.c @@ +1511,3 @@ GST_DEBUG_OBJECT (video_encoder, "set_format"); + g_mutex_lock (&encoder->encoder_lock); Why is the encoder lock needed here for all the duration of the drain() call? Doesn't seem necessary... and @@ +1901,2 @@ gst_vp8_enc_process (encoder); + g_mutex_lock (&encoder->encoder_lock); ... this wouldn't be neecessary then. Try to keep the lock outside the finish/drain functions and only hold it when necessary. Note that finish() and set_format() are both called with the stream lock held.
Created attachment 282510 [details] [review] vp8enc: Allow caps renegotiation To test this patch patch attached to bug734266 is needed
Any changes I should perform?
Thanks, I've merged it and fixed some things around it. See the other commits commit 23a3377b1efcd634b6ca531ba29445905df0ceda Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Sep 30 11:35:12 2014 +0300 vp8enc/vp9enc: Protect the encoder with a mutex in all situations commit df053c997cef3156403a52dd00ceea576f680344 Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Sep 30 11:31:43 2014 +0300 vp9enc: Allow caps renegotiation https://bugzilla.gnome.org/show_bug.cgi?id=726329 commit ced5d657e39bdbb55ac8566c0d7cc47b23c4780b Author: Sebastian Dröge <sebastian@centricular.com> Date: Tue Sep 30 11:28:39 2014 +0300 vp8enc: finish() and drain() should return a GstFlowReturn commit a2e2012ae3cb6991382af639defeef9e898b9076 Author: Jose Antonio Santos Cadenas <santoscadenas@gmail.com> Date: Fri Mar 14 12:59:02 2014 +0100 vp8enc: Allow caps renegotiation https://bugzilla.gnome.org/show_bug.cgi?id=726329
Hi, this patch introduced an regression on two pass encoding. Some DVDs will cause caps renegotiation just at the end of stream and as result corrupted multipass cache. The issue looks like fallowing: 1. start of stream 2. frames ......... 5. caps reinit. 6. frames 6. end of stream. Since reinit will send NULL frame image to the encoder, it will create EOS header for the multipass cache file. Next frame will reinit multipass file and actual end of stream will create one more header with EOS and wrong count of frames.
Bug 747728 related to this regression