After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 726329 - vp8enc: Add support for caps renegotiation
vp8enc: Add support for caps renegotiation
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
1.x
Other All
: Normal enhancement
: 1.4.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-03-14 12:11 UTC by Jose Antonio Santos Cadenas
Modified: 2015-07-06 17:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
vp8enc: Change GMutex for GRecMutex (7.83 KB, patch)
2014-03-14 12:11 UTC, Jose Antonio Santos Cadenas
none Details | Review
vp8enc: Allow caps renegotiation (1.99 KB, patch)
2014-03-14 12:11 UTC, Jose Antonio Santos Cadenas
none Details | Review
vp8enc: Change GMutex for GRecMutex (7.97 KB, patch)
2014-03-14 12:19 UTC, Jose Antonio Santos Cadenas
rejected Details | Review
vp8enc: Allow caps renegotiation (2.38 KB, patch)
2014-03-14 12:20 UTC, Jose Antonio Santos Cadenas
needs-work Details | Review
vp8enc: Allow caps renegotiation (2.07 KB, patch)
2014-03-17 09:11 UTC, Jose Antonio Santos Cadenas
reviewed Details | Review
vp8enc: Allow caps renegotiation (4.13 KB, patch)
2014-03-22 21:38 UTC, Jose Antonio Santos Cadenas
none Details | Review
vp8enc: Allow caps renegotiation (4.13 KB, patch)
2014-03-22 21:39 UTC, Jose Antonio Santos Cadenas
needs-work Details | Review
vp8enc: Allow caps renegotiation (3.93 KB, patch)
2014-08-05 08:45 UTC, Jose Antonio Santos Cadenas
committed Details | Review

Description Jose Antonio Santos Cadenas 2014-03-14 12:11:19 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.
Comment 1 Jose Antonio Santos Cadenas 2014-03-14 12:11:47 UTC
Created attachment 271875 [details] [review]
vp8enc: Allow caps renegotiation
Comment 2 Jose Antonio Santos Cadenas 2014-03-14 12:19:34 UTC
Created attachment 271877 [details] [review]
vp8enc: Change GMutex for GRecMutex
Comment 3 Jose Antonio Santos Cadenas 2014-03-14 12:20:08 UTC
Created attachment 271878 [details] [review]
vp8enc: Allow caps renegotiation
Comment 4 Nicolas Dufresne (ndufresne) 2014-03-15 00:01:42 UTC
I'm really not fan of replacing fast mutex with recursive mutex, unless strictly required. Would it be possible to elaborate ?
Comment 5 Jose Antonio Santos Cadenas 2014-03-15 11:39:19 UTC
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.
Comment 6 Nicolas Dufresne (ndufresne) 2014-03-15 11:49:30 UTC
(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.
Comment 7 Nicolas Dufresne (ndufresne) 2014-03-15 11:52:56 UTC
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 8 Nicolas Dufresne (ndufresne) 2014-03-15 12:03:59 UTC
Comment on attachment 271878 [details] [review]
vp8enc: Allow caps renegotiation

As said, just going back to normal mutex of OBJECT_LOCK is needed.
Comment 9 Jose Antonio Santos Cadenas 2014-03-15 13:29:04 UTC
Ok, I'll change the patch to avoid the RecMutex. I haven't noticed that the stream lock is held in both functions.
Comment 10 Jose Antonio Santos Cadenas 2014-03-17 09:11:29 UTC
Created attachment 272123 [details] [review]
vp8enc: Allow caps renegotiation
Comment 11 Nicolas Dufresne (ndufresne) 2014-03-21 19:54:36 UTC
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.
Comment 12 Jose Antonio Santos Cadenas 2014-03-21 20:00:02 UTC
I'll do it tomorrow. Thanks for the review.
Comment 13 Jose Antonio Santos Cadenas 2014-03-22 21:38:18 UTC
Created attachment 272655 [details] [review]
vp8enc: Allow caps renegotiation
Comment 14 Jose Antonio Santos Cadenas 2014-03-22 21:39:58 UTC
Created attachment 272656 [details] [review]
vp8enc: Allow caps renegotiation
Comment 15 Jose Antonio Santos Cadenas 2014-04-01 07:29:17 UTC
Hi, have you got time to review the patch, any changes or fixes that should I add?
Comment 16 stic 2014-04-27 20:04:34 UTC
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.
Comment 17 Jose Antonio Santos Cadenas 2014-06-20 15:27:18 UTC
Hi guys,

Any options of getting this accepted?

Thanks
Comment 18 Nicolas Dufresne (ndufresne) 2014-07-10 17:00:19 UTC
Review of attachment 272656 [details] [review]:

Looks good to me.
Comment 19 Sebastian Dröge (slomo) 2014-07-18 12:20:03 UTC
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.
Comment 20 Jose Antonio Santos Cadenas 2014-08-05 08:45:46 UTC
Created attachment 282510 [details] [review]
vp8enc: Allow caps renegotiation

To test this patch patch attached to bug734266 is needed
Comment 21 Jose Antonio Santos Cadenas 2014-09-30 08:10:54 UTC
Any changes I should perform?
Comment 22 Sebastian Dröge (slomo) 2014-09-30 08:36:41 UTC
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
Comment 23 Oleksij Rempel 2015-07-06 17:25:40 UTC
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.
Comment 24 Oleksij Rempel 2015-07-06 17:29:55 UTC
Bug 747728 related to this regression