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 615783 - reworked timestamping in adder
reworked timestamping in adder
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 0.10.30
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-04-14 20:45 UTC by Stefan Sauer (gstreamer, gtkdoc dev)
Modified: 2010-04-30 07:31 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
reworked timestamping in adder (8.49 KB, patch)
2010-04-14 20:45 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review
only accept seek-types none and set (1.49 KB, patch)
2010-04-18 17:52 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
accepted-commit_after_freeze Details | Review
only accept seek-types none and set (1.50 KB, patch)
2010-04-19 20:05 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
committed Details | Review

Description Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-14 20:45:57 UTC
Created attachment 158756 [details] [review]
reworked timestamping in adder

Adder was using always incrementing timestamps. Seeking was done by setting the
position in the newsegment event. This was failing when doing segmented seeks
with rate<0.0, as offset (and thus timestamp) would go below 0.

Now we take both cur and end from the seek event. We construct newsegment events
depending including cur and end from the seek event. We set position to the
start of the segment. Timestamp is set to start or end of segment depending on
rate. Offset is recalculated.
Comment 1 Sebastian Dröge (slomo) 2010-04-16 17:56:39 UTC
Review of attachment 158756 [details] [review]:

Looks good in general but

::: gst/adder/gstadder.c
@@ +728,2 @@
       else
+        adder->segment_start = 0;

You should probably either refuse seeks with a different type or correctly handle them instead of just doing a [0,-1] seek :)
Comment 2 Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-16 18:38:06 UTC
(In reply to comment #1)
> Review of attachment 158756 [details] [review]:
> 
> Looks good in general but
> 
> ::: gst/adder/gstadder.c
> @@ +728,2 @@
>        else
> +        adder->segment_start = 0;
> 
> You should probably either refuse seeks with a different type or correctly
> handle them instead of just doing a [0,-1] seek :)

-1 is open end. We should probably reject GST_SEEK_TYPE_CUR and GST_SEEK_TYPE_END like everywhere else. Is it that what you mean?
Comment 3 Sebastian Dröge (slomo) 2010-04-16 18:53:07 UTC
Yes, either that or implement them.
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-17 18:37:19 UTC
(In reply to comment #3)
> Yes, either that or implement them.

Given that it was previously not handled for start anyway and the general state of support for GST_SEEK_TYPE_CUR/GST_SEEK_TYPE_END I would prefer to keep the patch as it is in this regard. If I would add support for it, it should be done as a separate patch (and feature request). It would also require to first fix sources to be able to test it even. I thing assuming an open end (which is the default anyway) is sane thing to do.


> find . -name "*.c" -exec grep -Hn "GST_SEEK_TYPE_CUR" {} \; | egrep -v "(check|enum)"
./gst-plugins-bad/gst/tta/gstttaparse.c:190:          case GST_SEEK_TYPE_CUR:
./gst-plugins-bad/ext/metadata/gstbasemetadata.c:1475:      if (start_type == GST_SEEK_TYPE_CUR)
./gst-plugins-bad/ext/metadata/gstbasemetadata.c:1500:      if (stop_type == GST_SEEK_TYPE_CUR)
./gst-plugins-base/gst-libs/gst/tag/gsttagdemux.c:834:          case GST_SEEK_TYPE_CUR:
./gst-plugins-base/gst-libs/gst/tag/gsttagdemux.c:854:          case GST_SEEK_TYPE_CUR:
./gstreamer/gst/gstsegment.c:304:    case GST_SEEK_TYPE_CUR:
./gstreamer/gst/gstsegment.c:338:    case GST_SEEK_TYPE_CUR:

> find . -name "*.c" -exec grep -Hn "GST_SEEK_TYPE_END" {} \; | egrep -v "(check|enum)"
./gst-plugins-bad/gst/tta/gstttaparse.c:193:          case GST_SEEK_TYPE_END:
./gst-plugins-bad/ext/metadata/gstbasemetadata.c:1477:      else if (start_type == GST_SEEK_TYPE_END) {
./gst-plugins-bad/ext/metadata/gstbasemetadata.c:1502:      else if (stop_type == GST_SEEK_TYPE_END) {
./gst-plugins-base/gst-libs/gst/cdda/gstcddabasesrc.c:774:      case GST_SEEK_TYPE_END:
./gst-plugins-base/gst-libs/gst/cdda/gstcddabasesrc.c:799:      case GST_SEEK_TYPE_END:
./gst-plugins-base/gst-libs/gst/tag/gsttagdemux.c:836:          case GST_SEEK_TYPE_END:
./gst-plugins-base/gst-libs/gst/tag/gsttagdemux.c:856:          case GST_SEEK_TYPE_END:
./gstreamer/gst/gstsegment.c:309:    case GST_SEEK_TYPE_END:
./gstreamer/gst/gstsegment.c:346:    case GST_SEEK_TYPE_END:
Comment 5 Sebastian Dröge (slomo) 2010-04-18 05:28:42 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Yes, either that or implement them.
> 
> Given that it was previously not handled for start anyway and the general state
> of support for GST_SEEK_TYPE_CUR/GST_SEEK_TYPE_END I would prefer to keep the
> patch as it is in this regard. If I would add support for it, it should be done
> as a separate patch (and feature request). It would also require to first fix
> sources to be able to test it even. I thing assuming an open end (which is the
> default anyway) is sane thing to do.

Well, then don't implement them... but your current patch pretends to support other seek types and interpretes them all as a open ended seek from 0, which is wrong.
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-18 08:02:44 UTC
My current patch changes the segment handling. Current code in git already doe this wrongly.

if (curtype == GST_SEEK_TYPE_SET)
  adder->segment_position = cur;
else
  adder->segment_position = 0;

I will make a second patch on top of this to return FALSE for CUR/END.
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-18 17:52:49 UTC
Created attachment 159030 [details] [review]
only accept seek-types none and set

Happy now?
Comment 8 Sebastian Dröge (slomo) 2010-04-19 07:03:06 UTC
Yes, thanks :)
Comment 9 Stefan Sauer (gstreamer, gtkdoc dev) 2010-04-19 20:05:49 UTC
Created attachment 159108 [details] [review]
only accept seek-types none and set

And this one even works :)