GNOME Bugzilla – Bug 748738
gstevent: add new api for parse seek event
Last modified: 2018-11-03 12:27:39 UTC
generally, when we parse seek event using gst_event_parse_seek function, parameter is too many I think. For example, If we just want to get the rate value from event, we should use like this gst_event_parse_seek(event, NULL, &rate, NULL, NULL, NULL...). I agree this is minor problem but I think that If we use segment for parsing event, it would be better.
Created attachment 302701 [details] [review] gstevent: add new api for parse seek event I suggest new api for parse seek event.
Created attachment 302968 [details] [review] gstevent: add new api for parse seek event Remove the if condition
Created attachment 303392 [details] [review] gstevent: add new api for parse seek event If we parse the seek event, we can use like below. gst_event_parse_seek (event, &rate, &format, &flags, &cur_type, &cur, &stop_type, &stop); but I think that if we use segment for parsing event, this is a brief look. we can use like below. but I don't like this function name. gst_event_parse_seek_segment(event, &segment);
Could you review this patch?
Comment on attachment 303392 [details] [review] gstevent: add new api for parse seek event >diff --git a/gst/gstsegment.h b/gst/gstsegment.h >index f0f7af5..0ca84fd 100644 >--- a/gst/gstsegment.h >+++ b/gst/gstsegment.h >@@ -202,6 +202,8 @@ struct _GstSegment { > > guint64 position; > guint64 duration; >+ GstSeekType start_type; >+ GstSeekType stop_type; > > /* < private > */ > gpointer _gst_reserved[GST_PADDING]; This breaks ABI, you need to make a struct with the two new fields and make that struct a union with the _gst_reserved field in order to make sure the overall struct size of GstSegment stays the same. I'm sure you can find examples if you grep for 'union' in the header files.
(In reply to Tim-Philipp Müller from comment #5) > Comment on attachment 303392 [details] [review] [review] > gstevent: add new api for parse seek event > > >diff --git a/gst/gstsegment.h b/gst/gstsegment.h > >index f0f7af5..0ca84fd 100644 > >--- a/gst/gstsegment.h > >+++ b/gst/gstsegment.h > >@@ -202,6 +202,8 @@ struct _GstSegment { > > > > guint64 position; > > guint64 duration; > >+ GstSeekType start_type; > >+ GstSeekType stop_type; > > > > /* < private > */ > > gpointer _gst_reserved[GST_PADDING]; > > This breaks ABI, you need to make a struct with the two new fields and make > that struct a union with the _gst_reserved field in order to make sure the > overall struct size of GstSegment stays the same. I'm sure you can find > examples if you grep for 'union' in the header files. I appreciate your review. I make a struct with two new fileds using union. I have found 'union' in the gstpad header file. thanks for your comment:)
Created attachment 303928 [details] [review] gstevent: add new api for parse seek event Attached patch implements union fields for parse seek event.
Also, I tested using gstevent.check. It's working for me.
Could you please review this patch? I'm waiting for your review.
I hope that this patch will be merge to gstreamer version 1.6 since.
Review of attachment 303928 [details] [review]: There is quite a big type error, that makes me think it should not be considered for 1.6. Remains I've made a review as if it was. Additional, you forgot to update the -sections.txt file in docs, and forgot to update win32 definition file (warning in make check, error in make distcheck). ::: gst/gstevent.c @@ +1211,3 @@ + * + * Allocate a new seek event with the given @segment + * Returns: (transfer full): a new seek event. Missing Since 1.6 mark @@ +1243,3 @@ + GST_QUARK (RATE), G_TYPE_DOUBLE, segment->rate, + GST_QUARK (FORMAT), GST_TYPE_FORMAT, segment->format, + GST_QUARK (FLAGS), GST_TYPE_SEEK_FLAGS, segment->flags, Segment flags are not of GST_TYPE_SEEK_FLAGS, this need conversion. It might not be reversible. @@ +1258,3 @@ + * @segment: a pointer to a #GstSegment + * + * Parses a seek @event and stores the retults in the given by @segment. Missing Since 1.6 mark. ::: gst/gstsegment.h @@ +210,3 @@ + GstSeekType start_type; + GstSeekType stop_type; + } seek; Everywhere else we use ABI.abi. ::: tests/check/gst/gstevent.c @@ +219,3 @@ + fail_if (GST_EVENT_IS_SERIALIZED (event)); + + gst_event_parse_seek_segment (event, &parsed); parsed looks too much like a boolean. Could be name parsed_segment ?
I don't know, it's not clear to me that we should add + GstSeekType start_type; + GstSeekType stop_type; fields to the GstSegment structure, they don't have anything to do with the segment, do they? We're just (ab)using the GstSegment struct here for some convenience function, no? Not sure this function is really needed at all. Maybe gst_segment_do_seek_from_event() would be sufficient too?
(In reply to Nicolas Dufresne (stormer) from comment #11) > Review of attachment 303928 [details] [review] [review]: > > There is quite a big type error, that makes me think it should not be > considered for 1.6. Remains I've made a review as if it was. Additional, you > forgot to update the -sections.txt file in docs, and forgot to update win32 > definition file (warning in make check, error in make distcheck). > > ::: gst/gstevent.c > @@ +1211,3 @@ > + * > + * Allocate a new seek event with the given @segment > + * Returns: (transfer full): a new seek event. > > Missing Since 1.6 mark > > @@ +1243,3 @@ > + GST_QUARK (RATE), G_TYPE_DOUBLE, segment->rate, > + GST_QUARK (FORMAT), GST_TYPE_FORMAT, segment->format, > + GST_QUARK (FLAGS), GST_TYPE_SEEK_FLAGS, segment->flags, > > Segment flags are not of GST_TYPE_SEEK_FLAGS, this need conversion. It might > not be reversible. > > @@ +1258,3 @@ > + * @segment: a pointer to a #GstSegment > + * > + * Parses a seek @event and stores the retults in the given by @segment. > > Missing Since 1.6 mark. > > ::: gst/gstsegment.h > @@ +210,3 @@ > + GstSeekType start_type; > + GstSeekType stop_type; > + } seek; > > Everywhere else we use ABI.abi. > > ::: tests/check/gst/gstevent.c > @@ +219,3 @@ > + fail_if (GST_EVENT_IS_SERIALIZED (event)); > + > + gst_event_parse_seek_segment (event, &parsed); > > parsed looks too much like a boolean. Could be name parsed_segment ? I'll modify it. Thanks a lot stormer:)
(In reply to Tim-Philipp Müller from comment #12) > I don't know, it's not clear to me that we should add > > + GstSeekType start_type; > + GstSeekType stop_type; > > fields to the GstSegment structure, they don't have anything to do with the > segment, do they? We're just (ab)using the GstSegment struct here for some > convenience function, no? > > Not sure this function is really needed at all. > > Maybe gst_segment_do_seek_from_event() would be sufficient too? Tim sir. First of all, My english skill is not good. please understand it. I agree your opinion above comment. Actually, seektype don't have anything to do in GstSegment structure currently. But If we add seektype to GstSegment structure, we can "reuse" this structure simply without additional declaration of variable related seek event such as rate, format, start, stop, seektype and so on. As you know, size of segment is also same after changing structure because of make that struct a union with the _gst_reserved field. Also, we can change the gst_segment_do_seek fucntion simply like below. before -> gst_segment_do_seek(GstSegment * segment, gdouble rate, GstFormat foramt....) after -> gst_segment_do_seek(GstSegment * segment, GstSegment *seek_segment) We can reuse segment structure for segment_do_seek function also. IMHO, If we change the segment structure, it may help gstreamer user or developer to get simply implementation for seek event. Also I remember that sebastian agree this changing structure before uploading this patches. Please consider this patches for parse_seek I hope.
Note that I agree that this API seems cosmetic. _do_seek_from_event() as proposed by Tim could make more sense then trying to extend GstSegment (which is not strictly related to seek, in fact it should stay seek agnostic in my opinion).
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/112.