GNOME Bugzilla – Bug 615783
reworked timestamping in adder
Last modified: 2010-04-30 07:31:27 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.
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 :)
(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?
Yes, either that or implement them.
(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:
(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.
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.
Created attachment 159030 [details] [review] only accept seek-types none and set Happy now?
Yes, thanks :)
Created attachment 159108 [details] [review] only accept seek-types none and set And this one even works :)