framecpp_channeldemux.cc: send stream-start event before pushing first buffer

This merge request modifies the framecpp_channeldemux element to send a stream-start event to source pads before pushing the first buffer, which addresses 'Got data flow before stream-start event' warnings in framecppchanneldemux in gstreamer-1.18.x. See #66 (closed).

Closes #66 (closed).

requested review from @kipp.cannon

this is merely silencing a warning, not fixing non-functional code, and there is a chance this patch breaks working programs by introducing stream ID collisions that didn't exist before, so this should not be pushed without addressing the FIXME comment in the patch. the documentation https://gstreamer.freedesktop.org/documentation/gstreamer/gstevent.html#gst_event_new_stream_start says to use gst_pad_create_stream_id() to automatically generate a new ID. https://gstreamer.freedesktop.org/documentation/gstreamer/gstpad.html#gst_pad_create_stream_id and I assume the stream ID parameter can be the channel name.

what's the reason for not putting this in the src_pad_do_pending_events() function? caps need to be handled separately for the reason explained in the comments, but keeping this out of the src_pad_do_pending_events() function means heart beat events and buffers can be sent before the stream ID event, which looks like it's the same bug.

I had initially had the stream-start event being sent in src_pad_do_pending_events(), but ran into the issue that a "new caps" event was being sent before this (https://git.ligo.org/lscsoft/gstlal/-/blob/master/gstlal-ugly/gst/framecpp/framecpp_channeldemux.cc#L578-594). This causes a different warning, something like "got new caps event before stream-start event". I don't have the exact message off-hand but can dig it up if you'd like. Anyways, I had put the "stream-start" before updating the source pad's caps to avoid this.

hm. ok, well, I think some thought needs to be put to the relationships and order of things, then. the complication is the heart beat logic, which is used to send empty zero-length buffers carrying the current timestamp downstream to keep the assembly line flowing in the event that there is no data available. this is how we prevent the whole pipeline from locking up. there's no guarantee that online data is arriving when the pipeline is put into the playing state, so the demuxer might have to send heart beat buffers before it has sent any real data at all. but there are some events that must precede data buffers, empty or not, like the new segment event and the tags event (the new segment event because gstreamer requires it, the tags because some of our own elements require it). that's why the collection of event pushing code lives in a separate function src_pad_do_pending_events() that gets called from both the push-data-buffer code path and the push-heart-beat-buffer code path. if you don't put your stream ID event in that function, it will not be sent ahead of the first data buffer or other metadata events when the pipeline is started in a data outage situation. the caps don't live there because we can't know the caps unless we have actually decoded real data from a frame file: the caps cannot be set or even checked in the heart beat code path.

what I'm going to do is push a patch that moves the caps pushing code into the pending_events() function but with logic to disable it if caps are not known, and the heart beat code path will make use of that. then after that you can rework your patch to insert the stream ID logic into the correct place in the sequence of metadata events. first, I guess.

OK, I've pushed it. It compiles, but is otherwise untested.

Okay, great, I'll take a look.

Alright, I have moved the stream-start logic to the first event in src_pad_do_pending_events(). I'm getting the pad name via GST_PAD_NAME(pad) to set the stream ID. Can also confirm that your changes work as intended.

added 9 commits

• bdc58444...c4ae91ba - 7 commits from branch master
• 9882de38 - framecpp_channeldemux.cc: send stream-start event before pushing first buffer
• d09c4501 - framecpp_channeldemux.cc: generate stream ID from FrVect name rather than randomly generated ID

Compare with previous version

BTW, don't you need to do this all over the place? The gate element and the framecpp muxer come to mind. itacac, the older trigger generator. Anything not subclassed from basetransform needs its own custom event handling logic, and any of those that has multiple inputs or outputs probably won't process stream ID events properly.

Not that I'm aware of. I don't have a good sense of when it's required to handle this manually but my understanding is only elements that create new streams need to explicitly send stream-start events if it's not already handled by the corresponding base class. Some of the source elements (gds_lvshmsrc, etc) may need this. I haven't seen this warning pop up from the framecpp muxer or the gate element. itacac leverages GstAggregator which should take care of stream-start appropriately.

added 2 commits

• 64852648 - 1 commit from branch master
• 7945fd89 - framecpp_channeldemux.cc: send stream-start event before pushing first buffer

Compare with previous version

added 11 commits

• 7945fd89...41e7a2ed - 10 commits from branch master
• db53adaa - framecpp_channeldemux.cc: send stream-start event before pushing first buffer

Compare with previous version

merged