Created attachment 106603 [details] [review]
gst.c patch

Created attachment 106761 [details] [review]
gst.c patch

The patch looks ok to me.

Can we have a second validation before we commit ?

Looks ok to me. I haven't applied it or tested it, though.

Did you do that? If so, looks ok to commit after the current freeze.

seems ok and seems to work for me.

oh, it did not compile, I had to fix it to const gchar *update_env;

If this is in fact useful and we want to support it, then we should IMHO also add proper _is_enabled() _set_enabled() functions rather than just rely on an environment variable.

It seems to me this is more something you'd want to hard-code in embedded applications and not so much something you'd want to be able to sometimes enable and sometimes disable for debugging purposes like the segtrap and registry fork stuff.

(didn't mean to change milestone and patch status)

I wonder if this is the way to do it. Shouldn't there be a configure switch to turn off registry checking by default and only scan if the env-var is set. Nothing would change for normal installations. Where this is used, one can run GST_REGISTRY_UPDATE=1 gst-inspect >/dev/null 2>&1 after installations or during boot-time.

Also the new env-var should be documented in the man-pages.

The default is yes for GST_REGISTRY_UPDATE, so I think it will not impact current implementation, and adding _is_enabled() _set_enabled() functions is good idea for convenience.

I will try to get another fix verson for Tim's proposal.

Created attachment 107725 [details] [review]
new patch with "-is_enable and _set_enable"

I would still make this a build option (via configure). I am not seeing a usecase for _is_enabled() _set_enabled() functions. If I do _set_enabled(TRUE) nothing will happen, as the registry is only checked at startup.

If people want it, then when committing the patch please also add the new env-var to the docs and update docs/gst/gstreamer-section.txt with the new API.

If this should go in, please don't make the variable be called GST_REGISTRY_UPDATE with default yes/1.  It should be called GST_DISABLE_REGISTRY_UPDATE with default being unset (meaning off) instead.  This makes more sense and is consistent with the other GST_DISABLE_*/--gst-disable-* options.

I have updated my patch, add "--gst-disable-registry-update" in init option, env-var to running.xml and API to gstreamer-sections.txt

For environment name, I find there are no such environment name as "GST_DISABLE_*", so I think GST_REGISTRY_UPDATE is reasonable just like GST_REGISTRY_FORK. 

For the usecase of "_set_enabled()", I am not sure whether it is correct that user can call it before gst_init(), or we may remove these functions ?

Changed files:
gst.c
doc/gst/running.xml
doc/gst/gstreamer-section.txt

Created attachment 107896 [details] [review]
latest gst.c patch

Created attachment 107897 [details] [review]
gstreamer-sections.txt patch

Created attachment 107898 [details] [review]
running.xml patch

jason, could you do some before/after meassurements? I modified logging slightly and looked at the numbers. On x86 the reading of the cache takes 1/10 of a second. Verification that the cache is still good, takes 1/100 of a second. I don't think its worth to optimize that. I have committed a small optimization on cache validation and will try another one still. But I recommend looking at optimizing reading the cache. Bug #359653 has some hints in comment #26 and comment #35.

$ GST_DEBUG="GST_INIT:3" gst-inspect --gst-disable-registry-fork >/dev/null
0:00:00.005030537 gst.c:753:ensure_current_registry_nonforking: ...
0:00:00.094529159 gst.c:659:scan_and_update_registry: Validating registry cache
0:00:00.102749110 gst.c:723:scan_and_update_registry: Registry cache has not changed

Stefan, I think it is good for PC, but on embedded, both cpu and storage speed are slow, and the scan_and_update time is increasing line with the plugin number. 
By my test on embedded, if our plugin number is about 20, it will cost 200 ms, and app will do it every time launch. I think it is not necessary on embedded, as it may not change the plugins frequently.

By the way, if optimizing reading the cache can improve the performance, it is  really a good idea.

So current situation on this bug is that registry update is enabled by default except if launched with --gst-disable-registry-update or the GST_DISABLE_REGISTRY_UPDATE env var set.

I think that the function set to enable disable is not really useful in that case.

Can we commit ?

Just for the record on Nokia N810:
0:00:00.101044000  gst.c:767:ensure_current_registry_nonforking: 
0:00:00.288879000  gst.c:737:scan_and_update_registry: Registry cache has not
changed

So its about 2/100 sec. which is still quite fast.
The opne optimization I tried did not work out. I am basically fine with
committing it. I still wonder whats the usecase for the API, Tim?

< ensonic> __tim: what use case where you thinking of when suggesting to add _is_enabled() _set_enabled() to the registry-no-checking patch
< __tim> ensonic: use case "call this before gst_init to disable registry update checking"
< __tim> Having a magic environment variable for this doesn't seem quite right to me 
< ensonic> __tim: imho the usecase is to have it disabled by default and just run GST_CHECK_REGISTRY_CACHE=1 gst-inspect >/dev/null at startup and after installs
< __tim> true, and some have setters and getters as well, e.g. gst_registry_fork_set_enabled() and gst_segtrap_is_enabled()
< ensonic> __tim: instead of an env-var it could lso be an option
< __tim> but I don't really care enough to argue about this at length to be honest ;)
< ensonic> __tim: I prefer and option over two extra API entries that would be hardly called

so lets use an option, unless someone has a usecase for apps, the API can be added later if needed.

Stefan, if use gstreamer as ringtone engine, it will have critial performance issue, as ringtone engine needs start playback rapidly (less than 300ms), if we spend 200ms in gst_init(), we will not match the goal.

And if this patch doesn't impact the current behavior, and it can also improve the startup performance, why not add it?

For new API(set/get), I also think it is not useful, I will try to remove it.

Created attachment 108450 [details] [review]
gst.c patch with removing API

jason the patch is missing the Gobject entry for the option:

see gstc:gst_init_get_option_group()

    {"gst-disable-registry-update", 0, G_OPTION_FLAG_NO_ARG, G_OPTION_ARG_CALLBACK,
          (gpointer) parse_goption_arg,
          N_("Disable updating the registry"),
        NULL},

you can verift by running e.g. gst-launch-0.10 --help-gst
and it should show:
--gst-disable-registry-update     Disable updating the registry

Besides the GOption stuff this works fine and I am +1 to commit it. jason, thanks for the works on it.

Stefan, thanks for your suggestion, I will update my patch and add the missed.

Created attachment 109796 [details] [review]
gst.c patch updated with stefan's review

This looks good now. Before committing, I would just reorder the goption, so that its before "gst-disable-registry-fork" (as it is in the other lsts).

Anyone also fine with it?

I was just about to commit it, but while looking at it once again I would like to confirm one detail with the ther developers.

ensure_current_registry() is now  doing
#ifdef USE_BINARY_REGISTRY
  gst_registry_binary_read_cache (default_registry, registry_file);
#else
  gst_registry_xml_read_cache (default_registry, registry_file);
#endif

while this code was in
ensure_current_registry_forking/nonforking()

My question: should the forking cover registry cache loading *and* plugin loading or is plugin loading enough.

2008-04-24  Stefan Kost  <ensonic@users.sf.net>

	patch by: Jason Zhao <e3423c@motorola.com>

	* docs/gst/running.xml:
	* gst/gst.c:
	  Enable/disable scan_and_update_registry() based on commandline switch
	  or environment variable. Fixes #520468.

Stefan, your question may be our further optimizatiin goal. 

jason, I misread the code. The registry reading was actualy before the fork. So you change to move it to ensure_current_registry() is also a nice cleanup to have this just in one place.

Yes, but if registry is big, adding registry reading into fork process is also a good idea. How about it?

Jason, look at Bug #359653. It outlines some ideas how to make registry reading as such faster. I haven't had time to start implementing it though.

The fork() won't help to make things faster, as the parent waits for the child to finish checking the registry and in case of changes re-read it.

stefan, Is it possible to do registry reading at forked background process?

Well, it is done by default if your platform has fork() and you don't pass --gst-disable-registry-fork. But ensure_current_registry_forking() wait for the child-process to finish.

As far as I know, you can't initialize the libs totally async. I would be nice to have a set of threads doing gst_init(), gtk_init() and so on and then wiat for all of them to join. But imho that would not work.

Stefan, thanks for your warmhearted answer.

Try to put all of gst_init() in a background thread sounds painful to me - but possible, and without changing GStreamer itself.... just all the applications, heh.

An approach I've thought of before would be to make gst_init() spawn a thread for registry reading and instrumenting all the entry points to the gst_registry_* with a _priv_gst_registry_wait_loaded()... so that the registry loading occurs asynchronously and the program can continue without blocking until the first time it actually tries to look at plugin features or create an element.

It doesn't work if anything accesses the GstRegistry stuff directly though - everyone would need to go through the API