GNOME Bugzilla – Bug 757892
rtph264pay: add config-interval option to send PPS/SPS before every key frame
Last modified: 2015-12-02 06:52:45 UTC
Created attachment 315192 [details] [review] Patch V1 It's not enough to have timeout or event based SPS/PPS information sent in RTP packet. There are some scenarios when key frames may appear faster than 1 second which is the minimum timeout for sending SPS/PPS. So we need to have the possibility to always send SPS/PPS with every key frame.
Review of attachment 315192 [details] [review]: ::: gst/rtp/gstrtph264pay.c @@ -125,3 @@ g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_CONFIG_INTERVAL, - g_param_spec_uint ("config-interval", That's an API change, it's not allowed. I suggest just using G_MAXUINT instead or something like that
(In reply to Olivier Crête from comment #1) > Review of attachment 315192 [details] [review] [review]: > > ::: gst/rtp/gstrtph264pay.c > @@ -125,3 @@ > g_object_class_install_property (G_OBJECT_CLASS (klass), > PROP_CONFIG_INTERVAL, > - g_param_spec_uint ("config-interval", > > That's an API change, it's not allowed. I suggest just using G_MAXUINT > instead or something like that Olivier, I agree, this is an API change. But I think this is the minimal possible change and a bit more logical. Ideally would be to use "config-interval" equal to 0, but this is already defined as "disable". So now we have to choose between -1 and G_MAXUINT. The property itself was defined as integer with valid range from 0 to 3600 before. So to make G_MAXUINT valid we must allow all other values from 3601 to G_MAXUINT. One the other hand changing property storage from "unsigned int" to "signed int" is element internal implementation details and does not affect API since value is still should be in range from 0 to 3600. In this case we can extend min_value to -1 while keeping old numbers with same meaning. Another way is to introduce a new property like "config-mode" with options "interval", "disable", "always" and default value "interval". But this could make things a bit more complicated, since now we have 2 ways to disable PPS/SPS send. Set "config-interval" to 0 or set "config-mode" to "disable". So what do you think?
(In reply to Anton Bondarenko from comment #2) > On the other hand changing property storage from "unsigned > int" to "signed int" is element internal implementation details and does not > affect API since value is still should be in range from 0 to 3600. In this > case we can extend min_value to -1 while keeping old numbers with same > meaning. It's not just internal storage but an API change and will break code, e.g. > GValue v = G_VALUE_INIT; > g_value_init(&v, G_TYPE_UINT); > g_value_set_uint(&v, 123); > g_object_set_property(pay, "config-interval", &v); This will give you assertions unless your GLib is compiled without all the type checks.
I think 3600 is completly arbitrary, and we should not care. I would just suggest to increase the range to 0..MAXUINT, and using MAXUINT as a marker. The benifit is that it's concistent with how we disable the keyframes in most encoders. You don't event have to implement a special case, it's MAXUINT in seconds.
(In reply to Nicolas Dufresne (stormer) from comment #4) > ... > You don't event have to implement a special case, it's > MAXUINT in seconds. Nicolas, But in this case we are going to use MAXUINT as "send ASAP with key frame" so we still need to handle this as a special case. Or I didn't understand something?
Sorry, I had a different issue in mind. Attaching a PPS/SPS with every keyframe should imho just be a seperate property. Combine this with config-interval = 0 and you'll get what you expect (let the encoder decide). In term of recovery point, if you don't run in intra-frame mode, I wonder if this config-interval makes any sense.
My intent is to make this (send SPS/PPS with every keyframe) the default by the way. In light of that it seemed nice to re-use the existing property instead of adding a new one.
Created attachment 315284 [details] [review] Always transmit SPS/PPS if config-interval is G_MAXUINT
Indeed it make sense, except maybe for 1 case, IDR-H264. I'm saying "maybe", but maybe in that case one would like to send header on interval and not for each frames. Maybe we need to describe what we want to be configurable before throwing patches ?
I'm very much against making this the default unless we implement aggregation (STAP packets). In the worse case, if you have an IDR-only stream with very small frames (that fit in one packet each), you'll triple the number of packets you send. We need to keep a "reasonable" default interval, at least one second.
(In reply to Olivier Crête from comment #10) > I'm very much against making this the default unless we implement > aggregation (STAP packets). In the worse case, if you have an IDR-only > stream with very small frames (that fit in one packet each), you'll triple > the number of packets you send. We need to keep a "reasonable" default > interval, at least one second. Olivier, Do you have any other comments related to Patch V2 with use G_MAXUINT? It does not enable sending always as default behavior. > Maybe we need to describe what we want to be configurable before throwing patches ? Nicolas, I would like to be able to configure delay for sending PPS/SPS, but with possibility to always sent them with IDR. Zero value serve my needs ideally. But it's not possible since current API treat 0 as 'disable' sending PPS/SPS. So I'm trying to find another way to enable such possibility.
I believe disable cannot be as agressive as describe. PPS/SPS need to be send initially, and on discount (if it doesn't, then we have useless mode here).
Let's move the discussion whether to and under what circumstances to enable this by default into a different/future bug. I just mentioned that this was my goal because this might/should inform the API decision. Nicolas, in many cases (RTSP, SDP, etc.) PPS/SPS are transmitted out of band and do not have to be resent ever. The existing mode 0 is for that. I think the functionality that this patch tries to add makes a lot of sense and is desirable to have, even if not used by default yet. I also think the API proposed (special value G_MAXUINT/-1) makes sense and it's the one I had in mind as well. I think we should push this in some form (but leave the current default as is). I'd like to see the property description changed a little perhaps, to say "send with every keyframe" or such instead of "send always", I think that's easier to understand.
Agreed, thanks for clarifying the case 0.
Created attachment 315658 [details] [review] Updated patch wiht special G_MAXUINT according to Tim comments New patch which substitute all other patches. Differs only in property description and comment message.
Created attachment 316236 [details] [review] rtph264pay: change config-interval property type from uint to int This way we can use -1 as special value, which is nicer than MAXUINT. This is backwards compatible even with the GValue API, as shown by a unit test. ----- I think changing the property type and using -1 as special value is nicer, and it appears to be fully backward-compatible API/ABI-wise.
Agreed, works since we had a maximum of 3600 in the past (which we maintain now. It's good learning that having a maximum lower then G_MAXINT, allow changing a uint into int.
Created attachment 316367 [details] [review] rtph264pay: change config-interval property type from uint to int I know I'm butting in on someone elses review. I just looked at your proposed patches and I suggest that you commit them in the opposite order. This way the code always compiles and the unittests succeed. And each commit is logically self-contained and hopefully more obviously correct.
Created attachment 316368 [details] [review] rtph264pay: add "send SPS/PPS with every key frame" mode Oh, and yes I did keep the original authorship as these patches are not mine.
Adding additional choices usually makes it harder to get stuff merged ;) I was going to squash it like in Anton's original patch, but I've now pushed the two patches as suggested by Sebastian, it's nicer to do it in two separate steps and in that order (to not remove something again just added by the previous patch). commit 453a618a9dc0ee6b35532f8b6c1d526300e17c54 Author: Anton Bondarenko <antonbo@axis.com> Date: Fri Nov 27 09:27:29 2015 +0100 rtph264pay: add "send SPS/PPS with every key frame" mode It's not enough to have timeout or event based SPS/PPS information sent in RTP packets. There are some scenarios when key frames may appear more frequently than once a second, in which case the minimum timeout for "config-interval" of 1 second for sending SPS/PPS is not sufficient. It might also be desirable in general to make sure the SPS/PPS is available with every keyframe (packet loss aside), so receivers can actually pick up decoding immediately from the first keyframe if SPS/PPS is not signaled out of band. This patch adds the possibility to send SPS/PPS with every key frame. This mode can be enabled by setting "config-interval" property to -1. In this case the payloader will add SPS and PPS before every key (IDR) frame. https://bugzilla.gnome.org/show_bug.cgi?id=757892 commit 3026d1094b0041c9d43279cdb6a441391c6e6170 Author: Tim-Philipp Müller <tim@centricular.com> Date: Fri Nov 27 09:03:51 2015 +0100 rtph264pay: change config-interval property type from uint to int This way we can use -1 as special value, which is nicer than MAXUINT. This is backwards compatible even with the GValue API, as shown by a unit test. https://bugzilla.gnome.org/show_bug.cgi?id=757892