GNOME Bugzilla – Bug 413418
Make GstAppSrc/GstAppSink better
Last modified: 2010-07-03 12:17:17 UTC
Here are a few remarks of things I noticed when reading the source code : (1) the header lacks G_BEGIN_DECLS/G_END_DECLS (2) are the inclusion of config.h, unistd.h, fcntl.h and string.h needed? (3) the app_src_details are incomplete ("FIXME") (4) are the enum, gst_app_src_get_properties and gst_app_src_set_properties needed if there are no properties ? (5) the ->mutex, ->queue and ->cond are destroyed in dispose : shouldn't it be done in finalize ? And by the way, shouldn't something be done with the ->caps? (6) in gst_app_src_unlock, ->unlock is accessed and the condition variable signalled without holding the mutex : shouldn't it hold the mutex ? (7) in gst_app_src_unlock, wouldn't it be safer to broadcast the condition variable (although the current state of the code makes me think signal is enough) (8) in gst_app_src_create, shouldn't ->caps be tested for NULL before use ? (9) if that class is to be used by a program not based on GStreamer, shouldn't it have a method void gst_app_src_push_data (GstAppSrc *appsrc, gpointer data, guint length); which builds a GstBuffer then calls gst_app_src_push_buffer with it ?
Perhaps I should make it clearer that I'm mostly waiting for feedback about each point to provide patches : I'm not just waiting for someone else to do the work!
(10) the external api functions don't check GST_IS_APP_SRC... I'm working on a patch : bug days make developpers easier to catch :-)
Created attachment 83722 [details] [review] Should fix most issues This should fix all issues I reported except (3). The only testing it received is : gcc -Wall -c `pkg-config --cflags gstreamer-0.10` gstappsrc.c I don't really how to test it properly -- in fact I don't even know if it ever worked!
Hmmm... my new method gst_app_src_push_data should take a "const gpointer" as second argument.
Created attachment 83734 [details] [review] Patch to fix most issues Makes the second argument of gst_app_src_push_data a gconstpointer.
Created attachment 83747 [details] [review] Patch to fix most issues Eh, I noticed I left a g_cond_signal (which isn't a real issue), but more importantly that the _create method didn't check the unlocking!
> (2) are the inclusion of config.h, unistd.h, fcntl.h and string.h needed? config.h, always > (4) are the enum, gst_app_src_get_properties and gst_app_src_set_properties > needed if there are no properties ? Doesn't matter. I always leave them. > (6) in gst_app_src_unlock, ->unlock is accessed and the condition variable > signalled without holding the mutex : shouldn't it hold the mutex ? no. > (7) in gst_app_src_unlock, wouldn't it be safer to broadcast the condition > variable (although the current state of the code makes me think signal is > enough) no. > (8) in gst_app_src_create, shouldn't ->caps be tested for NULL before use ? no. > (9) if that class is to be used by a program not based on GStreamer, shouldn't > it have a method > void gst_app_src_push_data (GstAppSrc *appsrc, > gpointer data, > guint length); > which builds a GstBuffer then calls gst_app_src_push_buffer with it ? > This would need a callback and a closure. I don't think it's a good idea at this time.
All the above stuff is fixed, but we can continue using this bug to discuss stuff.
Created attachment 83916 [details] Testing program I thought it would help immensely to have a small test program, so I wrote this. The idea is that one should run this giving the name of a mono 8kHz wav file, and it should play. The organization is the following : - on the one hand there is a pipeline which starts with a filesrc, and ends in a fakesink whose handoffs we watch ; - on the other hand there is a pipeline which starts with an appsrc and ends with an autoaudiosink -- we push data we got from handoffs in the appsrc. It currently doesn't work, and I don't know if it's my test which is broken or appsrc, but it's a start.
From a --gst-debug-no-color --gst-debug=3 log, I see : 0:00:00.420883000 4254 0x804b220 WARN baseaudiosink gstbaseaudiosink.c:477:gst_base_audio_sink_preroll:<autoaudiosink0-actual-sink-alsa> error: sink not negotiated. 0:00:00.421131000 4254 0x804b220 INFO GST_ERROR_SYSTEM gstelement.c:1547:gst_element_message_full:<autoaudiosink0-actual-sink-alsa> posting message: The stream is in the wrong format. Although I have : 0:00:00.347852000 4254 0x804e890 INFO GST_PADS gstpad.c:1864:gst_pad_link: linked capsfilter1:src and autoaudiosink0:sink, successful and of course : 0:00:00.347062000 4254 0x804e890 INFO GST_PADS gstpad.c:1864:gst_pad_link: linked testsource:src and capsfilter1:sink, successful Which show that src->sink where connected correctly :-/
Created attachment 84165 [details] Updated version of my testing program Ok, I modified my code to use : audio/x-raw-int,channels=1,rate=8000,signed=(boolean)true,width=16,depth=16 as caps (those are the caps wavparse puts out -- taken from the log), which finally gets me sound, but it's pretty bad, probably because of that one-time : (test:4516): GStreamer-CRITICAL **: gst_segment_clip: assertion `segment->format == format' failed
Created attachment 84166 [details] Working testing program (audio) I can now confirm GstAppSrc works well with audio. It would probably be nicer to have a more formal series of tests, and a more thorough coverage of the various api features.
If one replaces the filesrc in my test with an audiotestsrc is_live=(boolean)true, then one finds out that the little test gets a little bigger : 30296 jpuydt 15 0 206m 145m 3680 S 72.9 28.9 0:09.73 test where the 72% of cpu usage is quite constant, but the memory usage rises steadily... pushing the box to its limits real fast. So either my test is broken or there's something else to fix in GstAppSrc :-)
I haven't had much time to hack on this. Hopefully this weekend. There's also an appsink now.
That's ok. I'll try to have a look at appsink. I can tell the GstAppBuffers are freed... I don't know if they all are, but I used a function like : void free_gst_app_buffer (gpointer buffer) { g_print ("Freeing a GstAppBuffer\n"); g_free (buffer); } and saw the message printed many times. So the leak doesn't look like it's here :-/
I didn't find appsink... uncommitted yet ?
It's the weekend. I worked on it. Yay! I added an example in examples/app.
Ok, I compiled and tested with the example ; I added the following to it : g_print ("Pulled buffer for pointer %p\n", GST_BUFFER_DATA (buf)); to see things better. I don't know how it works for you, but here : it generally works, but sometimes gets stuck (ie : we don't hit the return 0). Since it doesn't tax the box either, my theory is that the gst_app_sink_end_of_stream stucks ; as the problem doesn't always occur, I would suspect a multithreading issue. Looking at the code of GstAppSink, I see that in gst_app_sink_event, you don't signal appsink->cond. Once you'll have done that, make your example check for buf != NULL before use. Finally, my example (see attachment) still taxes the box. It's getting there :-)
I am not feeling very happy with having this plugin in -bad like this. We have never before shipped anything apart from core and base exposing API in this manner (having header files and stuff in /usr/lib/). I do realize that -bad is our sandbox, yet I cant help but worry about putting out a release at some point exposing this. If people agree that this either needs to move to -base first or be disabled before any releases of -bad I am fine, but in either case I think we should push hard to make its lifespan in -bad CVS as short as possible.
I agree.
I agree, and that's why I'm pushing to make it better -- so that it can go out of -bad :-)
What could I do to find why my test taxes the box ?
Created attachment 85607 [details] Test taxing box This test pushes audiotestsrc to alsasink indirectly using GstAppSrc ; it makes my box slow down, fills the ram... until I kill it.
The log obtained with : ./test --gst-debug-no-color --gst-debug=4 2>/tmp/log is about 27M in size (1M compressed), and that was for a very short test, so perhaps it's better for me not to attach it there....
I can probably paste this chunk of it : 0:00:05.154529000 4352 0x80eb038 DEBUG basesrc gstbasesrc.c:1371:gst_base_src_get_range:<audiotestsrc0> buffer ok 0:00:05.154547000 4352 0x80eb038 DEBUG capsfilter gstcapsfilter.c:244:gst_capsfilter_prepare_buf:<capsfilter0> Input buffer already has caps 0:00:05.154565000 4352 0x80eb038 DEBUG basesink gstbasesink.c:1952:gst_base_sink_chain_unlocked:<testsink> got times start: 0:20:11.520000000, end: 0:20:11.648000000 0:00:05.154582000 4352 0x80eb038 DEBUG basesink gstbasesink.c:934:gst_base_sink_get_sync_times:<testsink> got times start: 0:20:11.520000000, stop: 0:20:11.648000000, do_sync 1 0:00:05.154600000 4352 0x80eb038 DEBUG basesink gstbasesink.c:1187:gst_base_sink_do_sync:<testsink> waiting for clock to reach 0:20:11.520000000 0:00:05.154617000 4352 0x80eb038 DEBUG basesink gstbasesink.c:1047:gst_base_sink_wait_clock:<testsink> sync disabled 0:00:05.154632000 4352 0x80eb038 DEBUG basesink gstbasesink.c:1193:gst_base_sink_do_sync:<testsink> clock returned 4 0:00:05.154648000 4352 0x80eb038 DEBUG basesink gstbasesink.c:1532:gst_base_sink_render_object:<testsink> rendering buffer 0x80f1728 0:00:05.154701000 4352 0x80eb038 DEBUG basesink gstbasesink.c:1588:gst_base_sink_render_object:<testsink> object unref after render 0x80f1728 0:00:05.154723000 4352 0x80eb038 DEBUG basesrc gstbasesrc.c:1353:gst_base_src_get_range:<audiotestsrc0> calling create offset 18446744073709551615 length 4096, time 0 0:00:05.154742000 4352 0x80eb038 DEBUG GST_PADS gstpad.c:2613:gst_pad_buffer_alloc_unchecked:<capsfilter0:sink> calling bufferallocfunc &gst_base_transform_buffer_alloc (@0xb7f496c0) of for size 2048 offset 9693184 0:00:05.154759000 4352 0x80eb038 DEBUG basetransform gstbasetransform.c:1059:gst_base_transform_buffer_alloc:<capsfilter0> allocating a buffer of size 2048 ... 0:00:05.154774000 4352 0x80eb038 DEBUG basetransform gstbasetransform.c:1063:gst_base_transform_buffer_alloc:<capsfilter0> ... and offset 9693184 0:00:05.154789000 4352 0x80eb038 DEBUG basetransform gstbasetransform.c:1086:gst_base_transform_buffer_alloc:<capsfilter0> requesting buffer with same caps, size 2048 0:00:05.154804000 4352 0x80eb038 DEBUG GST_PADS gstpad.c:2613:gst_pad_buffer_alloc_unchecked:<testsink:sink> calling bufferallocfunc &gst_base_sink_pad_buffer_alloc (@0xb7f3aef0) of for size 2048 offset 9693184 0:00:05.154821000 4352 0x80eb038 DEBUG GST_PADS gstpad.c:2655:gst_pad_buffer_alloc_unchecked:<testsink:sink> fallback buffer alloc 0:00:05.154910000 4352 0x80eb038 DEBUG basesrc gstbasesrc.c:1245:gst_base_src_do_sync:<audiotestsrc0> get_times returned invalid start 0:00:05.154925000 4352 0x80eb038 DEBUG basesrc gstbasesrc.c:1371:gst_base_src_get_range:<audiotestsrc0> buffer ok
Hi, is this bug still relevant, or should I use fakesrc instead? :)
It's still relevant : I don't know how to debug it further than I already have, and there has been no answer here... which unfortunately means that ekiga won't get gstreamer support anytime soon.
(In reply to comment #27) > It's still relevant : I don't know how to debug it further than I already have, > and there has been no answer here... which unfortunately means that ekiga won't > get gstreamer support anytime soon. > Have you tried the mailing list?
I didn't try the mailing-list, but this bug report wasn't open before I discussed about it on irc, and I already tried to push it further from there too. I must admit I didn't push it again recently since I lacked the time, but you'll notice I didn't leave the issue impossible to work on : I gave a minimal example showing the problem, which means any gstreamer developper can easily pick up where I left things.
Could you tell me how how you compiled the test program? The command below doesn't work (link to gstapp missing): gcc -I$HOME/root/include/gstreamer-0.10/gst/app `pkg-config --libs --cflags gstreamer-plugins-base-0.10 gstreamer-base-0.10` test.c
The -I$HOME/root/include/gstreamer-0.10/gst/app you added only makes the compiler find the header files ; for the linker you should certainly add something like "-L$HOME/root/lib -lgstapp" at the end of your command line.
For the record: -L$HOME/root/lib -lgstapp-0.10 I confirm that the test program seems to leak and slows down my computer. No time to look at it right now.
I had a look at the appsrc: There is a GQueue in it which fills up without limits because buffers arrive faster (audiotestsrc) than they are processed (alsasink). There is a GstQueue plugin in GStreamer. I don't know how, but I feel that this element should be used instead of the built-in GQueue in GstAppSrc.
I'm thinking of this: http://api.kde.org/4.0-api/kdelibs-apidocs/phonon/html/classPhonon_1_1AbstractMediaStream.html Basically you can push as much as you want in it but it's not a very good idea. Better is to react to the need-data signal and then push stuff in it, this can be used to implement pull and push based sources. There should also be a seek signal emited when the appsrc is configured to be seekable. Alternatively a max queue length could be set but then your push will block when the queue is full, don't know if that is cool, it depends on the use case. You could also set the source to sync, making it a (pseudo) live source. Or configure it as live and feed it live data. I'll try some of this and post some patches.
Adding myself, because swfdec needs this for proper decoding of audio/video streams. Question: What needs to be done to get this in the next -good release?
Uh... adding myself because for some reason I wasn't notify about the recent comments!!! I came here to mention that it would be nice if that were taken care of before the upcoming releases : after all my example code shows there's something fishy somewhere... and it isn't clear (at least to me) it's a bug limited to GstApp* : perhaps it's just GstApp* triggering an issue elsewhere.
Hoping there is still activity on this since I've been looking for something like this for an own project. I think my application would need the proposed sync option, processing input from a camera.
Just thought about it today, in fact. We probably need to set up a separate project for gstapp, since gst-plugins-bad can't usefully contain libraries.
Well, it should go much nearer the core at some point, but it can't as long as it's not up to the task (and my sample code shows that either it isn't, or there's a bug somewhere else... and I'm unable to help further on that point, as I don't have the knowledge : this is the reason why that bug has been stuck this long even though I really deeply care for it). Let it get in a good state in gst-plugins-bad, then move it somewhere else.
I know its not exactly nice, but we could use action signal instead of API. gst_app_src_push_buffer (GST_APP_SRC (app->src), buf); would then become: g_signal_emit_by_name (app->src, "push-buffer", buf);
The point of this element was to *have* an API.
I made it so that it has a real API but everything is also exposed as properties and action signals, just because we can.
And what about the taxing test? I'm glad there's a nice api, but if it's unusable anyway...
Added some more features and the -bad/examples/app directory is filling up with examples for all use cases I could think of. The case for Comment #23 is that indeed audiotestsrc is running faster (sync against the system clock) than appsrc ! alsasink consumes (sync with audio clock). One option is to set the block property on appsrc so that its queues don't fill up, this will make audiotestsrc slow down/drop buffers. Another option is to slave the alsasink pipeline clock to the producer pipeline clock or the other way around. I could write an example app of that if needed. I think we still need an example app for push and pull based insertion of video frames/audio samples into the pipeline.
*** Bug 331058 has been marked as a duplicate of this bug. ***
From the Bug #491050 marked as a dup: GstAppSink should be able to operate in pull-mode and provide an API something like GstBuffer * gst_app_sink_pull_range(guint64 offset, guint size); There would probably be an additional property "can-activate-pull" (like fakesink has) needed? We need to be able to operate the sink in pull mode, with adaptor buffering when upstream is push-based or completely pull based like basesrc.
*** Bug 491050 has been marked as a duplicate of this bug. ***
How does one set the block property in a pipeline description ? "appsrc block=true" ? You can have a look at bug #339897 to see some code which wraps gstreamer for use with ptlib (the base lib for ekiga's media framework) : it shows how another (much more primitive) media system works, and give idea what api would be needed... it's been long since I dug in it, but I think it's pull-based indeed. I'll try to give gstreamer-in-ekiga another try when I'll find the time : I'd be pretty happy to limit my (and ekiga's) exposure to ptlib's media framework to a simple adaptator code!
yes, properties are <name>=[<type>]<value>, optionally specifying the <type> as something like '(string)' if it cannot be derived from <value>, like: name=test name=(string)0 What's missing here still?
It depends on bug #554168, and it's in gst-plugins-bad although structurally it should belong to core or base (see comment #19)
So, you want to pull a specific number of bytes from the sink? I'm currently working on pull based scheduling but it will only work end to end in specific cases. Simulating this api in appsink is not too hard, though.
Not exactly a specific number of bytes, but rather at most that number of bytes per buffer. The goal is to fit in some network packets, if I understood well (I'm not into the networking part that much).
You use appsink to capture raw data from a gstreamer pipeline, right? So, If I understand correctly, you want to capture exactly N audio samples for payloading?
(In reply to comment #53) > You use appsink to capture raw data from a gstreamer pipeline, right? So, If I > understand correctly, you want to capture exactly N audio samples for > payloading? Yes, or whatever the logical unit of the last element's output is. The point is, the app should be able to request only the necessary amount of data in situations where the processing operation is expensive.
The other framework I'm trying to interface gstreamer with (and which I can't throw [yet]) pulls at most some number of bytes of data when it wants to. The method declaration is the following : bool GST::AudioInputManager::get_frame_data (char* data, unsigned size, unsigned& read); (where size is in fact constant, and is given to me before opening...)
Ok, in this case you could configure the audiosource to produce buffers of the given size with the latency-time property. The audiosource will then try to set this as the period time of the audio device. With each pull on appsink you should get a buffer of the negotiated duration.
Well "*the* audiosource" doesn't make sense : the code is basically making a pipeline from : command = g_strdup_printf ("%s ! appsink max_buffers=2 drop=true" " caps=audio/x-raw-int" ",rate=%d" ",channels=%d" ",width=%d" " name=ekiga_sink", devices_by_name[std::pair<std::string,std::string>(current_state.device.source, current_state.device.name)].c_str (), samplerate, channels, bits_per_sample); so there's not a single audiosource. I still added things to the device part of the pipeline : when I detect a device, I name one of the elements to get access to the volume, like "audiotestsrc name=ekiga_volume" or "alsasrc device=%s ! volume name=ekiga_volume". The problem is that if I want something named ekiga_volume and something named ekiga_blocksize, I'll have a problem when it's the same element I'd like to act one...
This bug hasn't seen any activity in 20 months. I suggest we close it as OBSOLETE. There are too many old comments, and it's hard to make sense of what's relevant and what's not. I suggest new bugs be opened for specific well-defined enhancement requests. This is more likely to yield results IMHO. There's also bug #554168 which I'm guessing refers to the issue mentioned in the last few comments (it's not really entirely clear, since it doesn't provide a great deal of context / information).