GNOME Bugzilla – Bug 752214
ipc: new elements for multi-process pipelines
Last modified: 2017-08-01 11:48:20 UTC
Created attachment 307208 [details] [review] inter: new elements for multi-process pipelines These elements allow splitting a pipeline across several processes, with communication done by intersink and intersrc elements. One pipeline is the master pipeline, and is the one that controls state changes. Other pipelines are slave, and change states based on orders from the master pipeline. Slave pipelines must use the interslavepipeline pipeline subclass. Comes with tests and example programs.
Created attachment 307209 [details] [review] add a virtual method for pushing buffers
Created attachment 307210 [details] [review] basesrc: forward message events
Created attachment 307211 [details] [review] basesink: expose _commit_state This one might be a bit controversial :)
Created attachment 307212 [details] [review] new elements Rebased to master as now now
Should #743510 be closed now or it's not a duplicate?
They seem to be aimed at different things. Arun's is much lighter too. It's not clear if it can talk between processes.
It seems that I might have broken the case where preroll needs more than one buffer...
Created attachment 307223 [details] [review] new elements inter3 example works again (issue with more than one async sink in slave processes)
Created attachment 307332 [details] [review] new elements Now builds when gst-validate is not present, and some minor fixes.
Created attachment 307356 [details] [review] new elements Fixes rare issue with libcheck bailing with "Bad message type argc [...]"
Created attachment 307413 [details] [review] new elements A few leak fixes
Created attachment 307553 [details] [review] basesink: expose _commit_state Add the pubic symbol to win32/common/libgstbase.def as per make check
Created attachment 307569 [details] [review] new elements Add a videoconvert in the inter3 sample pipeline (fixes preroll when autovideosink picks ximagesink), and update copyrights to the preferred naming.
Created attachment 307915 [details] [review] new elements Optimizations (malloc/free, adapter use) A documentation of the fd protocol Misc tweaks/improvements Ignore allocation query Pass protection meta (the only meta we support, as meta aren't generically saveable/loadable as events and messages are, but new metas should be easier to add using the new code)
Created attachment 307993 [details] [review] new elements Performance optimizations, and make ack time and read buffer size properties
Created attachment 308529 [details] [review] new elements Fix for occasional hang when seeking
Will anyone here want to review first, or should I push them ? Given they're new elements, there's pretty much no breakage risk.
Please wait until after 1.6.
I'd like to review this first, so please hold off with merging once master is open again (but feel free to remind me frequently). I noticed the test sample files are quite large (1MB together, note how the others are all a couple of kB at most) and you're adding a soft dependency on gst-validate, perhaps these tests should go into gst-validate instead then, or elsewhere anyway?
(In reply to Tim-Philipp Müller from comment #19) > I'd like to review this first, so please hold off with merging once master > is open again (but feel free to remind me frequently). OK. > I noticed the test sample files are quite large (1MB together, note how the > others are all a couple of kB at most) and you're adding a soft dependency They can probably be thinned down without too much difficulty. > on gst-validate, perhaps these tests should go into gst-validate instead > then, or elsewhere anyway? I'd rather they stay as unit tests. If gst-validate is unwanted, the changes to remove it are small anyway (gst-validate is optional there).
Created attachment 312680 [details] [review] new elements With slimmed down test data files, and the gst-validate bits removed.
ping(In reply to Tim-Philipp Müller from comment #19) > I'd like to review this first, so please hold off with merging once master > is open again (but feel free to remind me frequently). ping
Created attachment 319855 [details] [review] fix reader thread leak
Created attachment 319856 [details] [review] performance and leak fixes Couple patches with fixes. No behavioral change. They apply on top of the others.
I've continued fixing things on top of this so it seem best to post those here instead of separate bugs as this is still not merged.
Created attachment 325152 [details] [review] fix racy behvior when flushing
Created attachment 325153 [details] [review] fix leak false positive in tests
Created attachment 325154 [details] [review] move thread shutdown from finalize to dispose
Created attachment 325155 [details] [review] fix messages being sent as inter elements are shutting down
Created attachment 325156 [details] [review] Add HLS support to the inter3 example
Created attachment 325157 [details] [review] clearer error message
Created attachment 325158 [details] [review] do not timeout on serualized objects
(In reply to Tim-Philipp Müller from comment #19) (but feel free to remind me frequently). *reminder* :)
I understand Nicolas has reviewed this now, so I guess there's going to be updated patches?
There was a race found in integration when changing states, and there are substantial changes in that area. I'll post the updates once it's confirmed to fix the problem. Also more minor fixes I found while doing so. I don't think there was an actual review done yet.
There are a load of patches, so I'm going to point at a branch instead: https://git.collabora.com/cgit/user/vincent/gst-plugins-bad/log/?h=inter-tests The early history is not rebased/merged on that branch, you can ignore that. What counts are the newer patches after the last ones posted here.
The branch is now updated with the latest patches.
For the record, I'm unlikely to do more substantial work on this unless bugs are reported (and most recent patches are extra tests).
h(In reply to Tim-Philipp Müller from comment #19) > I'd like to review this first, so please hold off with merging once master > is open again (but feel free to remind me frequently). pingity ping.
Created attachment 354921 [details] [review] inter: introduce new elements for multi-process pipelines
Created attachment 354922 [details] [review] tests/check: add automatic unit test suite for intersink & intersrc
Created attachment 354923 [details] [review] tests/examples: add manual tests/examples for intersink & intersrc
Here is a new iteration of these patches. There are a lot of cleanups and fixes since the 2015 version. The automatic unit test depends on bug 784551.
Can we please find better names that express the ipc nature of these elements? I think intersrc/sink is too generic and it's confusing given the existing intervideosrc/sink which work in the same process. Maybe this should even go in a separate plugin? Could you please also flesh out the documentation to explain what the purpose of these elements is, and the big picture how-to-use-them, and some hints about how it's implemented. More than just two example pipelines.
(In reply to Tim-Philipp Müller from comment #44) > Can we please find better names that express the ipc nature of these > elements? I think intersrc/sink is too generic and it's confusing given the > existing intervideosrc/sink which work in the same process. Agreed. I have now chosen ipcpipeline (short for inter-process pipeline) as an alternative. Let me know what you think. > Maybe this should even go in a separate plugin? Agreed. > Could you please also flesh out the documentation to explain what the > purpose of these elements is, and the big picture how-to-use-them, and some > hints about how it's implemented. More than just two example pipelines. Done.
Created attachment 355419 [details] [review] ipcpipeline: introduce new plugin for inter-process pipelines
Created attachment 355420 [details] [review] tests/check: add automatic unit test suite for the ipcpipeline elements
Created attachment 355421 [details] [review] tests/examples: add manual tests/examples for the ipcpipeline elements
> I have now chosen ipcpipeline (short for inter-process pipeline) as > an alternative. Let me know what you think. Seems fine to me, thanks. > > Could you please also flesh out the documentation [..] > > Done. Thanks. To me it still looks like a lot of code with not a lot of comments or much implementation documentation apart from the protocol docs. I just hope it's all very obvious and self-explanatory :) I had mentioned before that I wanted to review the implementation in detail, but realistically I don't see myself having time for that any time soon, so that shouldn't keep you from merging it.
Created attachment 356288 [details] [review] ipcpipeline: introduce new plugin for inter-process pipelines Updated patch with some memory leak fixes
Created attachment 356289 [details] [review] tests/check: add automatic unit test suite for the ipcpipeline elements Refactored this test suite to be more readable and also fixed a lot of race conditions that were noticeable when run in valgrind.
Olivier gave me a review over chat and he brought up the question of why these functions are not in core (they are in ipcpipelinecomm.c in my patch): gst_value_serialize_event (const GValue * value) gst_value_deserialize_event (GValue * dest, const gchar * s) These functions are necessary to serialize GstNavigationEvent messages, which are messages that contain an event in one of the structure fields. Because we use gst_structure_to_string to serialize messages, the event is lost unless the core knows how to serialize a GstEvent into a string. I have not attempted to put them in the core because the serialization is not 100% accurate. It internally uses gst_structure_to_string() as well, which is not capable of serializing every possible field of the event (it will lose objects, for a start). Do you think it makes sense to put them in core anyway?
It might make sense, but we usually only put things into core once there are multiple potential users for it. Can always be done later IMHO.
Alright, I'm going to merge this today, unless there is an objection. Olivier has already ack'ed it.
commit d3920aa2a902405a9d455d270445037e163135ab Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Tue Aug 1 13:26:38 2017 +0300 meson: enable building the ipcpipeline plugin commit e97877dc7e107f0ae514325a86c0cbac77653e9b Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Wed Jul 5 16:56:24 2017 +0300 tests/examples: add manual tests/examples for the ipcpipeline elements ipcpipeline1 is a very simple test that shows a short videotestsrc fragment. ipc-play is a clone of gst-play that splits the pipeline in two processes, running the source & demuxer on the master process and the decoders & sinks on the slave. commit 35a01f41ce312b53a2ad82ce8a65b3561c3abf81 Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Wed Jul 5 16:53:48 2017 +0300 tests/check: add automatic unit test suite for the ipcpipeline elements All tests run within a common framework for splitting processes and making them interract properly with the gst check system. commit 3089d142b015d9c8fcf0c9b25ce0c6c705f4be7d Author: George Kiagiadakis <george.kiagiadakis@collabora.com> Date: Wed Jul 5 16:50:22 2017 +0300 ipcpipeline: introduce new plugin for inter-process pipelines These elements allow splitting a pipeline across several processes, with communication done by the ipcpipelinesink and ipcpipelinesrc elements. The main use case is to split a playback pipeline into a process that runs networking, parser & demuxer and another process that runs the decoder & sink, for security reasons. https://bugzilla.gnome.org/show_bug.cgi?id=752214