GNOME Bugzilla – Bug 699793
videomixer: resets its current segment when receiving a flush stop
Last modified: 2013-05-20 19:07:28 UTC
The problem is that the current segment values are set on the seek events, and we get multiple flush stops after that seek event. The other solution would be to find out who sends these flush stops and why they arrive after the seek, but as adder doesn't reinit its segment values, I thought this could be an equally good patch.
Created attachment 243448 [details] [review] Don't reinit the segment on flush stop.
I'm not sure this is correct, flushing should reset the segment information and after a flush there must be a segment event. As videomixer forwards the seek events upstream it should get a segment event with that exact segment information from upstream after the flush-stop events.
Comment on attachment 243448 [details] [review] Don't reinit the segment on flush stop. Of course this also means that adder's code would be wrong. Did you check why you don't get the correct segment after the last flush-stop? Note that there could also be flush-stop because of other reasons than a seek and then you definitely need to reset the segment.
Created attachment 243510 [details] [review] This should be the good fix. This should be the good fix in my opinion, leaving the segment get reset on flush stop and taking into account new segments.
Comment on attachment 243510 [details] [review] This should be the good fix. Please take a look at the existing git logs for the format of the commit messages. We don't indent and do lists with + bullets in the beginning. Also for bugs we usually just put the bug URL in there. Otherwise, whenever the segment is updated, videomixer should also push a segment event downstream with that segment on the next opportunity.
Created attachment 243599 [details] [review] New proposed patch to fix the issue.
commit 84ae670ab40b258a10e1e21471e6dc9d786bf086 Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Mon May 6 23:43:03 2013 +0200 videomixer2: Take into account new segments Also forward the event downstream on the next opportunity. https://bugzilla.gnome.org/show_bug.cgi?id=699793
Actually, no. That's not how this is supposed to work. videomixer will always output a [0,-1] segment and will output frames according to the running times of the separate streams.
(In reply to comment #8) > Actually, no. That's not how this is supposed to work. > > videomixer will always output a [0,-1] segment and will output frames according > to the running times of the separate streams. How are we supposed to handle that in Gnl then, for the moment we calculate segment.base the following way: rstart = gst_segment_to_running_time (segment, GST_FORMAT_TIME, segment->start); rstop = gst_segment_to_running_time (segment, GST_FORMAT_TIME, segment->stop); copy.base = comp->priv->next_base_time; comp->priv->next_base_time += rstop - rstart; Who is wrong, gnonlin or videomixer? :)
Pasting a small conversation about the subject (no conclusion reached): <thiblahute> According to slomo ( https://bugzilla.gnome.org/show_bug.cgi?id=699793), videomixer is supposed to always output segment of the form of [0, -1], but it will break gnl way of calculating segment.base as what we do this next_segment.base = gst_segment_to_running_time (segment.stop) - gst_segment_to_running_time (segment.start). Is it wrong in GNL or should videomixer output a meaningful value for segment.stop? <__tim> thiblahute, it seems the only sane way to me to be able to acommodate any number of inputs with the possibility of inputs being added/removed/updated at any time <__tim> thiblahute, though maybe you want a mode where it 'rebases' on top of a specified master stream's segment or something <__tim> don't know if what gnl does could or should be done differently <thiblahute> __tim, I see, I am not sure adding such a mode would help a lot and instead bring complexity to videomixer users. I am thinking gnl might be able to calculate the base time differently. <thiblahute> __tim, Hrm, thinking about it, in gnl it would be good enough if we could make sure videomixer sets the stop time to the smallest stop time of the various segment received on it sinkpads. Would that be a solution for you? <__tim> I don't know. I haven't really thought properly about it, I'm just thinking out loud really :) <thiblahute> __tim, I see, If you get to some conclusion, I would be happy to hear about it :)
I think the stop time of the segment of videomixer should be the stop time of any seek event that was received. If there was no seek event ever, the stop will be -1. videomixer however could send a new segment event with stop updated (to the current running time) when all pads went EOS. Does that make sense? (After a seek videomixer should have a segment going from seek_start to seek_stop, not 0 to -1 anymore. Does it do that?)
(In reply to comment #11) > I think the stop time of the segment of videomixer should be the stop time of > any seek event that was received. If there was no seek event ever, the stop > will be -1. videomixer however could send a new segment event with stop updated > (to the current running time) when all pads went EOS. > > Does that make sense? > > > (After a seek videomixer should have a segment going from seek_start to > seek_stop, not 0 to -1 anymore. Does it do that?) That makes a lot of sense to me yes, thanks.
Then not resetting the saved segment uppon flush-stop also makes sense, as the segments will no longer come from upstream. That might solve some issues in GNL unit test, ensuring we don't receive any unexpected segments.
Created attachment 244853 [details] [review] Proposed patch to fix the issue This patch does what was first proposed, which means not reseeting the output segment on FLUSH_STOP.
Created attachment 244854 [details] [review] Additionally, send a segment event when sinkpads are EOS
Comment on attachment 244853 [details] [review] Proposed patch to fix the issue commit 521c9a7b5d8385355571616936e0ea251b0799df Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Mon May 20 19:51:07 2013 +0200 videomixer: Don't reset the output segment on flush stop Only init it when getting from READY to PAUSED, and change it on seek events. https://bugzilla.gnome.org/show_bug.cgi?id=699793
commit 2d3910fc7901b5f29e16c0fdd4e9067a6d7f66fe Author: Mathieu Duponchelle <mathieu.duponchelle@epitech.eu> Date: Mon May 20 19:59:13 2013 +0200 videomixer: When all sinkpads are eos, update output segment stop and forward it https://bugzilla.gnome.org/show_bug.cgi?id=699793