GNOME Bugzilla – Bug 754457
segment: Rewording of struct field descriptions
Last modified: 2015-09-25 22:24:01 UTC
Created attachment 310499 [details] [review] Patch file The new wording makes it easier to understand exactly what each field of the GstSegment struct represents.
Comment on attachment 310499 [details] [review] Patch file Some nitpicking >- * @rate: the rate of the segment >+ * @rate: the desired playback rate of the segment It is the rate, not the "desired" rate. The desired rate is what you pass to gst_event_new_seek(). >- * @applied_rate: the already applied rate to the segment >+ * @applied_rate: the already applied rate to the stream Why? It's not constant for the entire stream, but might change, no? >- * @start: the start of the segment >+ * @start: the start buffer timestamp of the segment >- * @stop: the stop of the segment >+ * @stop: the stop buffer timestamp of the segment I think these might add more confusion actually, people might interpret it as the timestamp of the first buffer, but the timestamp of the first buffer may be before the start.
(In reply to Tim-Philipp Müller from comment #1) > Comment on attachment 310499 [details] [review] [review] > Patch file > > Some nitpicking > > >- * @rate: the rate of the segment > >+ * @rate: the desired playback rate of the segment > > It is the rate, not the "desired" rate. The desired rate is what you pass to > gst_event_new_seek(). > > >- * @applied_rate: the already applied rate to the segment > >+ * @applied_rate: the already applied rate to the stream > > Why? It's not constant for the entire stream, but might change, no? I actually copied these two from gstreamer/docs/design/part-synchronisation.txt . The two files should become consistent, in that case. > >- * @start: the start of the segment > >+ * @start: the start buffer timestamp of the segment > >- * @stop: the stop of the segment > >+ * @stop: the stop buffer timestamp of the segment > > I think these might add more confusion actually, people might interpret it > as the timestamp of the first buffer, but the timestamp of the first buffer > may be before the start. Pasting your suggestion from our private discussion: perhaps "the start of the segment in buffer timestamp time (pts)" Sounds good to me, I like it :)
What Tim suggests makes sense, should also update the design doc accordingly.
Created attachment 310516 [details] [review] Updated patch file Also updated the design docs accordingly.
Created attachment 310526 [details] [review] Re-updated patch file Minor rewording in base: need to take into account the offset here.
Does this look OK now, enough to get in? Or is there something more to be done?
commit 60130eb5f030358c6068ae5a84d94613d3b723d2 Author: Vivia Nikolaidou <vivia@ahiru.eu> Date: Wed Sep 2 16:36:35 2015 +0300 segment: Rewording of struct field descriptions The new wording makes it easier to understand exactly what each field of the GstSegment struct represents.