GNOME Bugzilla – Bug 766647
adapter: Add a method to query current offset
Last modified: 2016-06-10 06:51:09 UTC
See commit
Created attachment 328175 [details] [review] adapter: Add a method to query current offset API: gst_buffer_get_offset This method allows retrieving the current offset based on the GST_BUFFER_OFFSET of the buffers that were pushed in. The offset will be set initially by the GST_BUFFER_OFFSET of DISCONT buffers, and then incremented by the sizes of the following buffers.
This assumes that buffer offsets are about bytes, which is not generally the case.
They tend to be in bytes for data that people feed into adapters though.
Hrm, expect maybe audio/video from a demuxer that's being parsed..
That doesn't change the behaviour. I can add something in the docs to point that out though.
It was just a note, not necessarily a problem :) Alternatively you could add something to get the last known offset (like we do for the timestamp), and the offset in bytes compared to where this offset came from. That would be more explicit about what we do and would also allow to easily work with raw video/audio.
Created attachment 328936 [details] [review] adapter: Add methods to query current offset API: gst_buffer_prev_offset API: gst_buffer_get_offset_from_discont The gst_buffer_get_offset_from_discont() method allows retrieving the current offset based on the GST_BUFFER_OFFSET of the buffers that were pushed in. The offset will be set initially by the GST_BUFFER_OFFSET of DISCONT buffers, and then incremented by the sizes of the following buffers. The gst_buffer_prev_offset() method allows retrievent the previous GST_BUFFER_OFFSET regardless of flags. It works in the same way as the other gst_buffer_prev_*() methods.
Review of attachment 328936 [details] [review]: ::: libs/gst/base/gstadapter.c @@ +1363,3 @@ + * The offset will be initially recorded for all buffers with + * %GST_BUFFER_FLAG_DISCONT on them, and then calculated for all other following + * buffers based on their size. I still don't like this very much as it's initialized with the initial offset and then incremented with bytes. You might mix different units here. What about having this function here always start at 0, and then adding a gst_adapter_get_pts_dts_and_offset_at_discont(adapter, &pts, &dts, &offset) for getting the timestamps and offsets at the discont? That way nothing would run into the risk of mixing units in calculations and you still have all the information that you need. Of course instead of a big function for all 3 values, it might make more sense to have 3 small functions :)
Review of attachment 328936 [details] [review]: ::: libs/gst/base/gstadapter.c @@ +1363,3 @@ + * The offset will be initially recorded for all buffers with + * %GST_BUFFER_FLAG_DISCONT on them, and then calculated for all other following + * buffers based on their size. I already specified in the documentation at the top of the file that it only makes sense for streams where offset corresponds to bytes.
The thing is that what I propose also adds value for the cases where the offset is not in bytes. I'll write a patch on Monday to show what I mean
Created attachment 329246 [details] [review] adapter: Add methods to query current offset API: gst_buffer_prev_offset API: gst_buffer_get_offset_from_discont The gst_buffer_get_offset_from_discont() method allows retrieving the current offset based on the GST_BUFFER_OFFSET of the buffers that were pushed in. The offset will be set initially by the GST_BUFFER_OFFSET of DISCONT buffers, and then incremented by the sizes of the following buffers. The gst_buffer_prev_offset() method allows retrievent the previous GST_BUFFER_OFFSET regardless of flags. It works in the same way as the other gst_buffer_prev_*() methods.
Created attachment 329247 [details] [review] adapter: Rename functions and implement new functions, update test We don't do calculations with different units (buffer offsets and bytes) anymore but have functions for: 1) getting the number of bytes since the last discont 2) getting the offset (and pts/dts) at the last discont and the previously added function to get the last offset and its distance from the current adapter position.
Created attachment 329272 [details] [review] adapter: Rename functions and implement new functions, update test We don't do calculations with different units (buffer offsets and bytes) anymore but have functions for: 1) getting the number of bytes since the last discont 2) getting the offset (and pts/dts) at the last discont and the previously added function to get the last offset and its distance from the current adapter position.
I agree that the renamed function scheme is clearer and more versatile. One could return the byte distance with the _get_foo_at_discont() I suppose like it's done with the prev_(dts|pts)? Don't mind keeping it as-is though, might actually be nicer.
Attachment 329246 [details] pushed as 67ae0ad - adapter: Add methods to query current offset Attachment 329272 [details] pushed as 8c7da1d - adapter: Rename functions and implement new functions, update test
Thanks for the review, let's get this in then :)