GNOME Bugzilla – Bug 727916
bufferpool: Does not implement set_config() as documented
Last modified: 2014-05-08 17:18:25 UTC
Documentation said: "If the parameters in config can not be set exactly, this function returns FALSE and will try to update as much state as possible. The new state can then be retrieved and refined with gst_buffer_pool_get_config()." Though the implementation simply free and ignore the config if the subclass returns FALSE. From now own, v4l2 is actually trying to negotiate it's buffer pool, but without this feature it quite hard.
Created attachment 273931 [details] [review] Update the configure even if set_config() returned false
Comment on attachment 273931 [details] [review] Update the configure even if set_config() returned false So we accept the unaccepted config but are not marked as configured. So the caller has to get the partially set config to see what happened, then set a new one that can be accepted and only then the pool is configured? Seems ok to me
Exactly. I'll give myself a bit of time to think it through. One case I haven't answered yet is If the pool was already configured (or pre-configured), but the new config is rejected, shall be got back to configure = FALSE ?
I've started updating base class accordingly. Pseudo-code for configuring a pool (that is not active) is: if (!set_config()) read_config if (newmin < min) fail if (newsize < size) fail etc. accept_updated_config() The idea is to do the strict minimum in term of pool config validation, which is that we can at least have the queried minimum.
Created attachment 275699 [details] [review] [PATCH] bufferpool: Update the configure even if set_config() returned false According to the documentation, when set_config() return false, it should be possible to read the modified version of the config. This patch fixes the implementation so it is now according to the documentation. https://bugzilla.gnome.org/show_bug.cgi?id=727916 --- gst/gstbufferpool.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
Created attachment 275700 [details] [review] [PATCH] pool-nego: Retry setting configuration with modified config Buffer pool set_config() may return FALSE if requested configuration needed small changes. Reget the config and try setting it again (validating the changes first). This ensure we have a configured pool if possible. https://bugzilla.gnome.org/show_bug.cgi?id=727916 --- libs/gst/base/gstbasesrc.c | 33 ++++++++++++++++++++++++++++++++- libs/gst/base/gstbasetransform.c | 32 +++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 2 deletions(-)
Remains a patch for gst-plugins-base and more for elements doing their own. But I would like to have this reviewed first, as it's going to be very similar over and over.
This will depend on #728268 indirectly, since checking return values triggers more bugs in our configuration scheme. Basically, it seems that when get a reconfigure event, we try and reconfigure the pool, though the offered pool are active. Even though nothing have changed, we fail set_config() and that's what #728268 is about ;-P
Looking again, I notice that validating a config is more work then that. Since we need to check that no option has been dropped. If the only "generically" acceptable change is increasing min, or decreasing max within min boundary, the right approach would be to keep a copy of the config, and apply the min/max of the new config on the old one and set the modified old one. The dilemma I have, is that if the pool only makes "acceptable changes", returning TRUE seems fair. Though, for libav setting max to min is not an acceptable change. So I'm tempted to say that base classes can't really validate anything. So I'm starting to think that base class should presume modification by pool are fine, and it's up to the class, through decide_allocation virtual to validate these changes if they have specific rules.
Only subclasses can return FALSE from the set_config() call. If they do, they would need to do something like this: - take old, currently accepted config - update old config with acceptable parameters from new config - clear new config, copy (updated old) config fields into new config - return FALSE Not very easy but doable.
Comment on attachment 275700 [details] [review] [PATCH] pool-nego: Retry setting configuration with modified config A little verbose but maybe we can make a nice convenience check function for those caps,size,min,max
I've allowed myself to add a helper. commit 23e69d98d8a6dd84faa97926cab7c2e5cf274f0a Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Apr 15 14:17:00 2014 -0400 pool-nego: Retry setting configuration with modified config Buffer pool set_config() may return FALSE if requested configuration needed small changes. Reget the config and try setting it again (validating the changes first). This ensure we have a configured pool if possible. https://bugzilla.gnome.org/show_bug.cgi?id=727916 commit 194db480e061f3b43c06e1f162715939e7f29c58 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Thu May 8 12:47:43 2014 -0400 bufferpool: Add an helper to validate config When we call gst_buffer_pool_set_config() the pool may return FALSE and slightly change the parameters. This helper is useful to do the minial required validation before accepting the modified configuration. https://bugzilla.gnome.org/show_bug.cgi?id=727916
Comment on attachment 275700 [details] [review] [PATCH] pool-nego: Retry setting configuration with modified config With helper
commit 43acfbbb86c4c140dc1f90bed31e7c0d708b74fd Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Apr 8 19:27:55 2014 -0400 bufferpool: Update the configure even if set_config() returned false According to the documentation, when set_config() return false, it should be possible to read the modified version of the config. This patch fixes the implementation so it is now according to the documentation. https://bugzilla.gnome.org/show_bug.cgi?id=727916
Sorry for the noise, 1.3.2 is not out yet ;-P