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 766647 - adapter: Add a method to query current offset
adapter: Add a method to query current offset
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 767354
 
 
Reported: 2016-05-19 08:42 UTC by Edward Hervey
Modified: 2016-06-10 06:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
adapter: Add a method to query current offset (11.52 KB, patch)
2016-05-19 08:42 UTC, Edward Hervey
none Details | Review
adapter: Add methods to query current offset (17.35 KB, patch)
2016-06-02 09:46 UTC, Edward Hervey
none Details | Review
adapter: Add methods to query current offset (17.35 KB, patch)
2016-06-07 09:37 UTC, Sebastian Dröge (slomo)
committed Details | Review
adapter: Rename functions and implement new functions, update test (18.40 KB, patch)
2016-06-07 09:37 UTC, Sebastian Dröge (slomo)
none Details | Review
adapter: Rename functions and implement new functions, update test (24.12 KB, patch)
2016-06-07 13:12 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description Edward Hervey 2016-05-19 08:42:09 UTC
See commit
Comment 1 Edward Hervey 2016-05-19 08:42:14 UTC
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.
Comment 2 Sebastian Dröge (slomo) 2016-05-19 08:56:43 UTC
This assumes that buffer offsets are about bytes, which is not generally the case.
Comment 3 Tim-Philipp Müller 2016-05-19 09:05:25 UTC
They tend to be in bytes for data that people feed into adapters though.
Comment 4 Tim-Philipp Müller 2016-05-19 09:06:11 UTC
Hrm, expect maybe audio/video from a demuxer that's being parsed..
Comment 5 Edward Hervey 2016-05-19 12:10:20 UTC
That doesn't change the behaviour. I can add something in the docs to point that out though.
Comment 6 Sebastian Dröge (slomo) 2016-05-19 12:33:16 UTC
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.
Comment 7 Edward Hervey 2016-06-02 09:46:24 UTC
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.
Comment 8 Sebastian Dröge (slomo) 2016-06-02 09:55:25 UTC
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 :)
Comment 9 Edward Hervey 2016-06-03 12:28:16 UTC
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.
Comment 10 Sebastian Dröge (slomo) 2016-06-03 12:33:20 UTC
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
Comment 11 Sebastian Dröge (slomo) 2016-06-07 09:37:53 UTC
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.
Comment 12 Sebastian Dröge (slomo) 2016-06-07 09:37:58 UTC
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.
Comment 13 Sebastian Dröge (slomo) 2016-06-07 13:12:38 UTC
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.
Comment 14 Tim-Philipp Müller 2016-06-09 17:09:56 UTC
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.
Comment 15 Sebastian Dröge (slomo) 2016-06-10 06:50:05 UTC
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
Comment 16 Sebastian Dröge (slomo) 2016-06-10 06:50:59 UTC
Thanks for the review, let's get this in then :)