Created attachment 281362 [details] [review]
tsdemux: implement convert queries on the sinkpad

Allow a source to query the pipeline downstream to convert byte/time offsets.
This is useful for upstream elements to convert position to/from bytes and
time.

There's nothing wrong with that of course, but what do you need it for? What's the use case? Chances are if you think you need this your whole approach is not quite right.

I need it to implement "time pause" in dlnasrc. I need to convert the current position (bytes from souphttpsrc) to time so I can send the proper TimeSeekRange query to the server.

Could also be useful in the future so that queue2 could be able to convert first/last bytes into time values (to better figure out bitrate, etc...)

Created attachment 284173 [details] [review]
tsdemux: implement convert queries on the sinkpad

Allow a source to query the pipeline downstream to convert byte/time offsets.
This is useful for upstream elements to convert position to/from bytes and
time.

Review of attachment 284173 [details] [review]:

Except for the locking, this looks good to me, the existing code already do most of the job. As dlansrc is probably the only user, to keep this working in the long term, it would be nice to have a light unit test.

e.g. filesrc ! tsdemux ! fakesink

Seek somewhere in time, convert to bytes, then seek there in bytes, check the time matches. Or something along these lines. Not need for anything super complex, just so make check triggers this code.

About the locking, you might only have to care about "reffing" and copying point to what you need during the operation (using OBJECT_LOCK), unless all these object are guarantied to stay around while you old a ref on the element itself. I would also like if you check if mpegts_packetizer is thread safe.

::: gst/mpegtsdemux/tsdemux.c
@@ +586,3 @@
+
+      if (G_UNLIKELY (demux->program == NULL))
+        return FALSE;

Should you also check base->packetizer, or is this one refcount "protected" ? (just to double check). You need some locking/refcount, as this may be called from any thread.

@@ +592,3 @@
+      if (src_fmt == GST_FORMAT_BYTES && dest_fmt == GST_FORMAT_TIME) {
+        dest_val = mpegts_packetizer_offset_to_ts (base->packetizer, src_val,
+            demux->program->pcr_pid);

Is this call thread safe ?

(In reply to comment #6)
> 
> About the locking, you might only have to care about "reffing" and copying
> point to what you need during the operation (using OBJECT_LOCK), unless all
> these object are guarantied to stay around while you old a ref on the element
> itself. I would also like if you check if mpegts_packetizer is thread safe.

  The tricky one is the program.
  I'd grab the object lock, take the pcr_pid, release the lock and then do the rest of the calculation (and error out properly if there's already no program).

> 
> ::: gst/mpegtsdemux/tsdemux.c
> @@ +586,3 @@
> +
> +      if (G_UNLIKELY (demux->program == NULL))
> +        return FALSE;
> 
> Should you also check base->packetizer, or is this one refcount "protected" ?
> (just to double check). You need some locking/refcount, as this may be called
> from any thread.

->packetizer always exists

> 
> @@ +592,3 @@
> +      if (src_fmt == GST_FORMAT_BYTES && dest_fmt == GST_FORMAT_TIME) {
> +        dest_val = mpegts_packetizer_offset_to_ts (base->packetizer, src_val,
> +            demux->program->pcr_pid);
> 
> Is this call thread safe ?

yes

Created attachment 286376 [details] [review]
tsdemux: implement convert queries on the sinkpad

Allow a source to query the pipeline downstream to convert byte/time offsets.
This is useful for upstream elements to convert position to/from bytes and
time.

-- 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/gst-plugins-bad/issues/162.