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 764707 - segment: Modifiy inside segment condition
segment: Modifiy inside segment condition
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 1.11.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-07 03:03 UTC by Seungha Yang
Modified: 2017-01-31 15:09 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
segment: Modifiy inside segment condition (1.12 KB, patch)
2016-04-07 03:04 UTC, Seungha Yang
none Details | Review
segment: Modifiy inside segment condition (4.98 KB, patch)
2016-04-15 12:02 UTC, Seungha Yang
none Details | Review
segment: Modifiy inside segment condition (3.94 KB, patch)
2016-04-15 14:35 UTC, Seungha Yang
none Details | Review
segment: Modifiy inside segment condition (3.71 KB, patch)
2016-04-15 14:41 UTC, Seungha Yang
committed Details | Review

Description Seungha Yang 2016-04-07 03:03:00 UTC
segment: Modifiy inside segment condition

There is a special case that segment_start == segment_stop == start.
It's inside segment, but current old code did not consider it.
Comment 1 Seungha Yang 2016-04-07 03:04:05 UTC
Created attachment 325516 [details] [review]
segment: Modifiy inside segment condition
Comment 2 Seungha Yang 2016-04-07 03:05:51 UTC
This approach similar to commit 
76a55f 2009-08-11 gstsegment: Actually start==stop==segment_start is inside the segment
Comment 3 Seungha Yang 2016-04-07 03:26:02 UTC
There is a mp4 file which have different track duration between video and audio.
- Video track duration: 48 sec
- Audio track duration: 29 sec

For the file, seek to over 29 sec is failure always.
Detailed description is as follows.

- 1. Seek to over 29 sec
qtdemux figure out keyframe offset, segment, and etc for video and audio
Then, segments for video and audio are different

<For Video>
0:00:03.951523490 22988 0x7f4a201181e0 DEBUG              GST_EVENT gstpad.c:5531:gst_pad_send_event_unchecked:<multiqueue0:sink_0> have event type segment event: 0x7f4a1803e040, time 99:99:99.999999999, seq-num 1024, GstEventSegment, segment=(GstSegment)"GstSegment, flags=(GstSegmentFlags)GST_SEGMENT_FLAG_RESET, rate=(double)1, applied-rate=(double)1, format=(GstFormat)GST_FORMAT_TIME, base=(guint64)0, offset=(guint64)0, start=(guint64)30215692499, stop=(guint64)48048000000, time=(guint64)30215692499, position=(guint64)30215692499, duration=(guint64)18446744073709551615;";

<For Audio>
0:00:03.951859403 22988 0x7f4a201181e0 DEBUG              GST_EVENT gstpad.c:5531:gst_pad_send_event_unchecked:<multiqueue0:sink_1> have event type segment event: 0x1cfc640, time 99:99:99.999999999, seq-num 1024, GstEventSegment, segment=(GstSegment)"GstSegment, flags=(GstSegmentFlags)GST_SEGMENT_FLAG_RESET, rate=(double)1, applied-rate=(double)1, format=(GstFormat)GST_FORMAT_TIME, base=(guint64)0, offset=(guint64)0, start=(guint64)29696000000, stop=(guint64)29696000000, time=(guint64)30215692499, position=(guint64)29696000000, duration=(guint64)18446744073709551615;";

Note that segment.start == segment.stop for audio.

- 2. After flush start - stop
<video sink element get segment -> commit state then wait playing>
0:00:03.965053464 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:3166:gst_base_sink_default_event:<xvimagesink0> configured segment time segment start=0:00:30.215692499, offset=0:00:00.000000000, stop=0:00:48.048000000, rate=1.000000, applied_rate=1.000000, flags=0x01, time=0:00:30.215692499, base=0:00:00.000000000, position 0:00:30.215692499, duration 99:99:99.999999999
0:00:04.006421592 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:3408:gst_base_sink_chain_unlocked:<xvimagesink0> got times start: 0:00:30.215692499, end: 0:00:30.238541666
0:00:04.006479980 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:1957:gst_base_sink_get_sync_times:<xvimagesink0> got times start: 0:00:30.215692499, stop: 0:00:30.238541666, do_sync 1
0:00:04.006498002 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:2256:gst_base_sink_do_preroll:<xvimagesink0> prerolling object 0x7f4a1c58b5d0
0:00:04.006529952 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:945:gst_base_sink_set_last_buffer_unlocked:<xvimagesink0> setting last buffer to 0x7f4a1c58b5d0
0:00:04.006540546 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:2279:gst_base_sink_do_preroll:<xvimagesink0> preroll buffer 0:00:30.215692499
0:00:04.007878708 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:1502:gst_base_sink_commit_state:<xvimagesink0> commiting state to PAUSED
0:00:04.007920258 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:1527:gst_base_sink_commit_state:<xvimagesink0> posting PAUSED state change message
0:00:04.007955615 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:1533:gst_base_sink_commit_state:<xvimagesink0> posting async-done message
0:00:04.007988127 22988 0x7f4a1c51c140 DEBUG               basesink gstbasesink.c:2209:gst_base_sink_wait_preroll:<xvimagesink0> waiting in preroll for flush or PLAYING

<audio sink element get segment -> get gap event (generated by streamsynchronizer)>
This is the problem happen point. audio sink element drop the gap event because start (of gap) is identical to segment.stop
So, audio sink element did not commit state
0:00:03.952646266 22988 0x7f4a1c579190 DEBUG               basesink gstbasesink.c:3166:gst_base_sink_default_event:<alsasink1> configured segment time segment start=0:00:29.696000000, offset=0:00:00.000000000, stop=0:00:29.696000000, rate=1.000000, applied_rate=1.000000, flags=0x01, time=0:00:30.215692499, base=0:00:00.000000000, position 0:00:29.696000000, duration 99:99:99.999999999
0:00:03.952902581 22988 0x7f4a1c579190 DEBUG              GST_EVENT gstpad.c:5531:gst_pad_send_event_unchecked:<alsasink1:sink> have event type gap event: 0x1bb2b20, time 99:99:99.999999999, seq-num 1046, GstEventGap, timestamp=(guint64)29696000000, duration=(guint64)18446744073709551615;
0:00:03.952914669 22988 0x7f4a1c579190 DEBUG               basesink gstbasesink.c:3231:gst_base_sink_event:<alsasink1> received event 0x1bb2b20 gap event: 0x1bb2b20, time 99:99:99.999999999, seq-num 1046, GstEventGap, timestamp=(guint64)29696000000, duration=(guint64)18446744073709551615;
0:00:03.952941688 22988 0x7f4a1c579190 DEBUG               basesink gstbasesink.c:1922:gst_base_sink_get_sync_times:<alsasink1> Got Gap time 0:00:29.696000000 duration 99:99:99.999999999
0:00:03.952957053 22988 0x7f4a1c579190 DEBUG               basesink gstbasesink.c:1957:gst_base_sink_get_sync_times:<alsasink1> got times start: 0:00:29.696000000, stop: 99:99:99.999999999, do_sync 1
0:00:03.952970977 22988 0x7f4a1c579190 LOG                 basesink gstbasesink.c:2044:gst_base_sink_get_sync_times:<alsasink1> buffer skipped, not in segment
0:00:03.952976721 22988 0x7f4a1c579190 DEBUG               basesink gstbasesink.c:2590:gst_base_sink_do_sync:<alsasink1> non syncable object 0x1bb2b20
Comment 4 Sebastian Dröge (slomo) 2016-04-07 06:39:09 UTC
Review of attachment 325516 [details] [review]:

::: gst/gstsegment.c
@@ +887,3 @@
    * we're outside of the segment */
+  if (G_UNLIKELY (segment->stop != -1 && start != -1 && (start > segment->stop
+              || (segment->start != segment->stop && start == segment->stop))))

Can you provide a unit test for this too?
Comment 5 Seungha Yang 2016-04-15 12:02:05 UTC
Created attachment 326089 [details] [review]
segment: Modifiy inside segment condition
Comment 6 Seungha Yang 2016-04-15 12:12:08 UTC
(In reply to Sebastian Dröge (slomo) from comment #4)
> Review of attachment 325516 [details] [review] [review]:
> 
> ::: gst/gstsegment.c
> @@ +887,3 @@
>     * we're outside of the segment */
> +  if (G_UNLIKELY (segment->stop != -1 && start != -1 && (start >
> segment->stop
> +              || (segment->start != segment->stop && start ==
> segment->stop))))
> 
> Can you provide a unit test for this too?

Update test code.

"segment.start == segment.stop" is very rare case, but it actually happen.
Both following cases are inside of segment in my opinion.
Case1: segment_start == segment_stop == start
Case2: segment_start == segment_stop == stop
Comment 7 Sebastian Dröge (slomo) 2016-04-15 13:21:45 UTC
Review of attachment 326089 [details] [review]:

::: tests/check/gst/gstsegment.c
@@ +358,3 @@
+  /* touching boundary point. it's inside because stop at segment start */
+  res = gst_segment_clip (&segment, GST_FORMAT_BYTES,
+      100, 200, &cstart, &cstop);

stop is exclusive, so this should return FALSE

@@ +388,3 @@
+
+  /* start on boundary point */
+  res = gst_segment_clip (&segment, GST_FORMAT_BYTES, 200, -1, &cstart, &cstop);

Maybe also some with stop!=-1 here?
Comment 8 Seungha Yang 2016-04-15 14:06:14 UTC
(In reply to Sebastian Dröge (slomo) from comment #7)
> Review of attachment 326089 [details] [review] [review]:
> 
> ::: tests/check/gst/gstsegment.c
> @@ +358,3 @@
> +  /* touching boundary point. it's inside because stop at segment start */
> +  res = gst_segment_clip (&segment, GST_FORMAT_BYTES,
> +      100, 200, &cstart, &cstop);
> 
> stop is exclusive, so this should return FALSE

One question. Although stop == segment.stop, it's FALSE because stop is exclusive. right?


> 
> @@ +388,3 @@
> +
> +  /* start on boundary point */
> +  res = gst_segment_clip (&segment, GST_FORMAT_BYTES, 200, -1, &cstart,
> &cstop);
> 
> Maybe also some with stop!=-1 here?
Comment 9 Seungha Yang 2016-04-15 14:35:36 UTC
Created attachment 326106 [details] [review]
segment: Modifiy inside segment condition
Comment 10 Seungha Yang 2016-04-15 14:37:52 UTC
(In reply to Sebastian Dröge (slomo) from comment #7)
> Review of attachment 326089 [details] [review] [review]:
> 
> ::: tests/check/gst/gstsegment.c
> @@ +358,3 @@
> +  /* touching boundary point. it's inside because stop at segment start */
> +  res = gst_segment_clip (&segment, GST_FORMAT_BYTES,
> +      100, 200, &cstart, &cstop);
> 
> stop is exclusive, so this should return FALSE

Code is changed that return FALSE

> 
> @@ +388,3 @@
> +
> +  /* start on boundary point */
> +  res = gst_segment_clip (&segment, GST_FORMAT_BYTES, 200, -1, &cstart,
> &cstop);
> 
> Maybe also some with stop!=-1 here?

Add test case with stop != -1
+  /* touching boundary point. it's inside because start at segment start */
+  res = gst_segment_clip (&segment, GST_FORMAT_BYTES,
+      200, 300, &cstart, &cstop);
+  fail_unless (res == TRUE);
+  fail_unless (cstart == 200);
+  fail_unless (cstop == 200);
Comment 11 Seungha Yang 2016-04-15 14:41:42 UTC
Created attachment 326108 [details] [review]
segment: Modifiy inside segment condition

Removing unnecessary white space... sorry
Comment 12 Tim-Philipp Müller 2016-11-11 18:43:20 UTC
Patch was updated.
Comment 13 Edward Hervey 2017-01-31 15:09:32 UTC
commit 15f2898e872d3b7ce74ea5fbd35fac9f1f47b4a5
Author: Seungha Yang <sh.yang@lge.com>
Date:   Fri Apr 15 20:54:42 2016 +0900

    segment: Modifiy inside segment condition
    
    There is a special case that segment_start == segment_stop == start.
    It's inside of segment
    
    https://bugzilla.gnome.org/show_bug.cgi?id=764707