GNOME Bugzilla – Bug 728379
appsink: add push_sample() convenience function for easy appsrc -> appsink use
Last modified: 2015-07-13 23:38:18 UTC
In 0.10 you can do this: rtspsrc ... ! rtph264depay ! h264parse ! video/x-h264,stream-format=avc,alignment=au ! appsink and then: appsrc ! queue ! matroskamux ! ... and it works fine, in 1.0 to make this works in the appsink callback you have to get the caps from the sample and explictly set them on appsrc, if you set something generic like video/x-h264,stream-format=avc,alignment=au on appsrc it does not work in conclusion, in 0.10 the caps of the buffers sent to appsrc are automatically recognized, in 1.0 even putting an h264parse after appsrc or setting some generic caps is not enough, you have to wait for the first buffer on the appsink callback, get the full caps such as: video/x-h264, stream-format=(string)avc, alignment=(string)au, codec_data=(buffer)0142001fffe100146742001fe29019026fcb80b7010101a41e24454001000468ce3c80, width=(int)800, height=(int)600, framerate=(fraction)0/1, parsed=(boolean)true, pixel-aspect-ratio=(fraction)1/1" and set them on appsrc before start to send buffers, 0.10 was much more convenient, would be useful to restore the old way to negotiate/propagate buffers caps. Probably in 0.10 this works since each buffer has caps setted, in 1.0 the caps are on the samples and not on the buffers and in appsink callbacks we get samples, so why we push buffers and not samples to appsrc? Is this intentional for some technical reasons or an incomplete implementation?
It is not possible to restore this behaviour, and it is also not really desirable IMHO. If you push data into an appsrc, you should know what format that data is. And you do from your caps filter anyway, so I'm not sure why you can't set it from the start. h264parse can't detect/recognise AVC stream-format from the data, you must set caps when you push AVC. That's just how it is. Use byte-stream if you want something that doesn't require out-of-band signalling. It is actually the other way round than you describe: in 0.10 you could only know the format of the stream once you got a first buffer. In 1.0 you can know the format even before any data is sent (a CAPS event will be sent before any buffers are sent). appsink doesn't expose API for that, but you can hook into it differently if you want to (e.g. by connecting to the notify::caps event on the sink pad, or installing a pad probe). It is not clear to me how 1.0 is any more or less convenient than the 0.10 was. You get both buffer+caps from the appsink, so just set the caps on appsrc before you push the first data. What is inconvenient about that?
thanks for your answer, what is incovenient is that in 0.10 I have not to set the caps at all, while in 1.0 I have to and the caps must be exact, for example I know that I have h264,avc,au but I don't know codec data. I taked a look to appsrc code, what do you think about this modifications: 1) add a new method gst_app_src_push_sample, that accept a sample, exctract buffer and caps from the received and call the already existing push_buffer_full and set_caps methods 2) caps are setted only if the existing caps are null or different from the buffer's one 3) eventually add a new property, something similar to resend-streamheader in multifdsink, that allow to reset the caps if they change in new samples there should be no compatibility issue since existing applications can continue to use push_buffer this change will make much more simpler and convenient the use of appsink/appsrc that will become much more similar to a single pipeline (with some minor exceptions) if you think this could be accepted upstream I'll try to do a proper patch
just to be more clear about the different behaviour: - in 0.10 you can start both the pipeline at the same time and they just work - in 1.0 you have to wait for the caps on the first pipeline (streaming thread), set them on the second pipeline (main thread) and then send buffers on the second pipeline, is doable but require much work and different code organization than the same in 0.10, in my case I need to keep compatibility with both 0.10 and 1.0 in my apps for some time and I would like to maintain the same code organization for both 0.10 and 1.0
Created attachment 274580 [details] [review] proof of concept patch maybe we can make optional the part that set the caps from the sample with a new boolean property and then add docs and eventually a test/example, let me known if you want that I finish this work for upstream inclusion, for my use case this patch is enough
I think you're exagerating the inconvenience of setting the caps separately a little, but I don't see why something like in the patch would not be acceptable. Could you make a patch without the common submodule change please?
I'll sent a new patch with some other small changes and some more docs later or this weekend, thanks! Do you want a property to control the automatic caps setting on push_sample or is ok without?
Created attachment 274644 [details] [review] updated patch please review and check docs, english is not my native language :-)
Created attachment 274645 [details] [review] example that show push-sample usage the example use gst_app_src_push_sample, you can remove line 35 comment out line 36 to feed data using the signal push-sample
ping
Comment on attachment 274644 [details] [review] updated patch Patch no longer applies, could you rebase it please?
Created attachment 285322 [details] [review] rebased patch I tested the sample provided here: https://bugzilla.gnome.org/show_bug.cgi?id=729760 and seems ok with this patch applied
Created attachment 285326 [details] [review] example that show push-sample usage fixed a leak in the example
Review of attachment 285322 [details] [review]: Looks good to me but we usually don't add copyright notices and people to the authors list in the element factory for smaller changes. Otherwise those would become very big soonish, which wouldn't help anybody.
Review of attachment 285322 [details] [review]: Oh and the new function has to be added to the docs and to win32/common/libgstapp.def
Ok, I'll resend an updated patch later today, the docs should be included, can you please review my english? :-)
Created attachment 285368 [details] [review] updated patch
Comment on attachment 285368 [details] [review] updated patch The docs seem ok, but please add that only the caps and the buffer of the GstSample are used... and e.g. not the GstSegment.
Created attachment 285459 [details] [review] updated patch added """ Only the caps and the buffer of the provided sample are used and not for example the segment in the sample. """ is enough?
commit 617f72b526087a21679a9d71af88259f65c2a1f6 Author: Nicola Murino <nicola.murino@gmail.com> Date: Fri Sep 5 11:14:51 2014 +0200 appsrc: Add push_sample() convenience function for easy appsink -> appsrc use https://bugzilla.gnome.org/show_bug.cgi?id=728379
Comment on attachment 285326 [details] [review] example that show push-sample usage commit 646352e9595dc7b1a42b60b6a7a6b29567ce2440 Author: Nicola Murino <nicola.murino@gmail.com> Date: Thu Sep 4 11:56:50 2014 +0200 appsrc: Add example that shows gst_app_src_push_sample() usage
commit ab58a9af2f4c2570708c5620408409de02e391c2 Author: Tim-Philipp Müller <tim@centricular.com> Date: Mon Sep 15 21:51:15 2014 +0100 appsrc: fix recent ABI breakage caused by GstAppSrc structure size increase Also fixes 'make check'. https://bugzilla.gnome.org/show_bug.cgi?id=728379
Hi, I wonder if this patch need to be updated now that gst_sample support bufferlists, probably we need to check for both buffer and buffer list to correctly extract all buffers from a sample, am I right?
Probably, yes.