GNOME Bugzilla – Bug 793221
Add support for ONVIF backchannel
Last modified: 2018-02-16 09:09:33 UTC
The feature is implemented here: https://cgit.freedesktop.org/~slomo/gst-rtsp-server/log/?h=onvif-backchannel It comes with a couple of new subclasses of various GstRTSPServer classes for ONVIF specific things. We might also want to add further ONVIF specific features there at a later time, see e.g. https://bugzilla.gnome.org/show_bug.cgi?id=731769 and https://bugzilla.gnome.org/show_bug.cgi?id=741745 Biggest question is if the existing API now is extensive enough, and if the hack to add further arguments to virtual methods is acceptable. Included is also an example application, which can be used with the following rtspsrc changes and example application here: https://cgit.freedesktop.org/~slomo/gst-plugins-good/log/?h=onvif-backchannel
Shouldn't there be padding in the new class and instance structures for the onvif subclasses? We might want to add more vfuncs in future, for example, no? The vfunc extension hack is a bit unfortunate, but still seems better than adding 50 new vfuncs just for that extra context parameter. I wonder what g-i/bindings make of the #ifdef #else there. The only alternative I can think of is to push the context into thread-local storage before ->vfunc and pop it off after. Does the new vfunc define also need to be set in the meson build?
(In reply to Tim-Philipp Müller from comment #1) > Shouldn't there be padding in the new class and instance structures for the > onvif subclasses? We might want to add more vfuncs in future, for example, > no? Good point, I forgot to add that! > The vfunc extension hack is a bit unfortunate, but still seems better than > adding 50 new vfuncs just for that extra context parameter. I wonder what > g-i/bindings make of the #ifdef #else there. They probably take the case without the #define, I hope they run through cpp first before parsing the code :) That's how the generated .gir file looks like in any case. This would also be the good thing to do, otherwise it would break API of the bindings, but of course also means that the new variants of the vfuncs are not available from bindings. But subclassing any of this from bindings is unlikely to work anyway, the gst-rtsp-server API is not very bindings friendly in that regard. > The only alternative I can > think of is to push the context into thread-local storage before ->vfunc and > pop it off after. How did I miss this? I considered this but thought it would be bad because the number of TLS variables you can have is relatively limited... but gst-rtsp-server already has this anyway! gst_rtsp_context_push_current(), gst_rtsp_context_get_current(), etc. I'll update the patch to use that API instead of all the vfunc uglyness. > Does the new vfunc define also need to be set in the meson build? Yes
Updated to use the context from TLS instead of the vfunc hack, and added padding to the public structs.
That's much nicer :) About the rtspsrc patch: - API-wise it seems acceptable to me, though a bit unfortunate that the user has to find out the stream_id like that. Are multiple audio backchannels allowed, or are we just future proofing it? - Don't we need to keep the fakesrc around? Or at least a fakesrc ! funnel? I don't think we can count on the user always pushing audio samples, in fact they might not be pushing backchannel samples at all most of the time, so we'd still want to send those initial packets for NAT piercing, no?
(In reply to Tim-Philipp Müller from comment #4) > That's much nicer :) > > About the rtspsrc patch: > > - API-wise it seems acceptable to me, though a bit unfortunate that the > user has to find out the stream_id like that. Are multiple audio > backchannels allowed, or are we just future proofing it? Future-proofing, yes. > - Don't we need to keep the fakesrc around? Or at least a fakesrc ! funnel? > I don't think we can count on the user always pushing audio samples, in fact > they might not be pushing backchannel samples at all most of the time, so > we'd still want to send those initial packets for NAT piercing, no? What would those packets be good for though, on the backchannel ports I mean? Maybe I misunderstand what you mean
> > - Don't we need to keep the fakesrc around? Or at least a fakesrc ! funnel? > > What would those packets be good for though, on the backchannel ports I > mean? Maybe I misunderstand what you mean No, I think I was just confused, sorry. I thought the fakesrc was being removed in general and replaced by the src for the backchannel, but it's just the variable name changing and it can hold either the appsrc or the fakesrc (gst_rtspsrc_stream_configure_udp_sinks), so it's all good I think.
Going to merge both parts now then!