After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 699793 - videomixer: resets its current segment when receiving a flush stop
videomixer: resets its current segment when receiving a flush stop
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.1.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-05-07 00:20 UTC by Mathieu Duponchelle
Modified: 2013-05-20 19:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Don't reinit the segment on flush stop. (1.23 KB, patch)
2013-05-07 00:22 UTC, Mathieu Duponchelle
needs-work Details | Review
This should be the good fix. (961 bytes, patch)
2013-05-07 17:11 UTC, Mathieu Duponchelle
needs-work Details | Review
New proposed patch to fix the issue. (1.03 KB, patch)
2013-05-08 16:23 UTC, Mathieu Duponchelle
rejected Details | Review
Proposed patch to fix the issue (1.31 KB, patch)
2013-05-20 18:58 UTC, Mathieu Duponchelle
committed Details | Review
Additionally, send a segment event when sinkpads are EOS (963 bytes, patch)
2013-05-20 18:59 UTC, Mathieu Duponchelle
committed Details | Review

Description Mathieu Duponchelle 2013-05-07 00:20:25 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.
Comment 1 Mathieu Duponchelle 2013-05-07 00:22:04 UTC
Created attachment 243448 [details] [review]
Don't reinit the segment on flush stop.
Comment 2 Sebastian Dröge (slomo) 2013-05-07 07:10:35 UTC
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 3 Sebastian Dröge (slomo) 2013-05-07 07:41:32 UTC
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.
Comment 4 Mathieu Duponchelle 2013-05-07 17:11:52 UTC
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 5 Sebastian Dröge (slomo) 2013-05-08 11:48:17 UTC
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.
Comment 6 Mathieu Duponchelle 2013-05-08 16:23:10 UTC
Created attachment 243599 [details] [review]
New proposed patch to fix the issue.
Comment 7 Sebastian Dröge (slomo) 2013-05-09 14:19:29 UTC
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
Comment 8 Sebastian Dröge (slomo) 2013-05-09 14:26:17 UTC
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.
Comment 9 Thibault Saunier 2013-05-19 14:26:47 UTC
(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? :)
Comment 10 Thibault Saunier 2013-05-19 16:44:15 UTC
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 :)
Comment 11 Sebastian Dröge (slomo) 2013-05-20 12:23:01 UTC
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?)
Comment 12 Thibault Saunier 2013-05-20 14:33:59 UTC
(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.
Comment 13 Nicolas Dufresne (ndufresne) 2013-05-20 15:52:55 UTC
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.
Comment 14 Mathieu Duponchelle 2013-05-20 18:58:28 UTC
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.
Comment 15 Mathieu Duponchelle 2013-05-20 18:59:18 UTC
Created attachment 244854 [details] [review]
Additionally, send a segment event when sinkpads are EOS
Comment 16 Sebastian Dröge (slomo) 2013-05-20 19:05:49 UTC
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
Comment 17 Sebastian Dröge (slomo) 2013-05-20 19:07:25 UTC
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