GNOME Bugzilla – Bug 768510
appsrc: handle pushing custom segments
Last modified: 2018-11-03 11:47:11 UTC
Currently applications can't control precisely the SEGMENT event being pushed out from appsrc in push-mode. An option would be to make appsrc handle the (optional) segment present on GstSample when calling the "push-sample" action/method.
In case of changing Period adpativedemux, SEGMENT event will be triggered which was followed by stream-start event. On the other hand, let's assume a case that DASH/HLS streaming without adaptivedemux... In that case, current appsrc implementation cannot handle SEGMENT event which was driven by application (might be stream-start event also). So, we need to open an optional way to handle external (application) driven SEGMENT event and stream-start event.
Thanks for taking the time to report this. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find. *** This bug has been marked as a duplicate of bug 760707 ***
Erm, wrong bug. Sorry for the noise :)
Created attachment 331149 [details] [review] Remove-trailing-whitespace.patch
Created attachment 331150 [details] [review] handle-pushing-custom-segments.patch
Created attachment 331151 [details] [review] handle-pushing-custom-segments.patch
Review of attachment 331151 [details] [review]: Thanks, this looks like a good start :) We should double-check if the seamless segment API of basesrc does the right thing though, it was implemented for a completely different use-case. That's why it has the "seamless" in the name. This here is not really for anything seamless anymore. I'm not sure there should be a new property for this. It should be safe to just always do that if the sample contains a valid segment ::: gst-libs/gst/app/gstappsrc.c @@ +167,3 @@ #define DEFAULT_PROP_CURRENT_LEVEL_BYTES 0 #define DEFAULT_PROP_DURATION GST_CLOCK_TIME_NONE +#define DEFAULT_PROP_DRIVEN_BY_APPLICTAION FALSE Typo, APPLICATION @@ +185,3 @@ PROP_CURRENT_LEVEL_BYTES, PROP_DURATION, + PROP_DRIVEN_BY_APPLICTAION, Here too @@ +427,3 @@ + * GstAppSrc::driven-by-application: + * + * Allows applictaion to push custom segment with calling the "push-sample" And here @@ +1202,2 @@ if (!GST_IS_BUFFER (obj)) { + if (!GST_IS_CAPS (obj)) { Store the GstSample in the queue, and then use gst_sample_get_buffer/caps/segment() here instead ::: tests/check/elements/appsrc.c @@ +512,3 @@ + GList *expected = NULL; + + segment = gst_segment_new (); Allocate on the stack :) @@ +514,3 @@ + segment = gst_segment_new (); + + for (i = 0; i < sizeof (modes) / sizeof (modes[0]); i++) { G_N_ELEMENTS @@ +521,3 @@ + sink = gst_element_factory_make ("fakesink", NULL); + gst_bin_add_many (GST_BIN (pipe), src, sink, NULL); + gst_element_link (src, sink); Some error checking here @@ +534,3 @@ + gst_app_src_set_callbacks (GST_APP_SRC (src), &cb, NULL, NULL); + + gst_element_set_state (pipe, GST_STATE_PLAYING); Check if this returns an error @@ +576,3 @@ + + /* Give some time to the appsrc loop to push the buffers */ + g_usleep (G_USEC_PER_SEC * 1); Why? @@ +580,3 @@ + if (modes[i] == GST_APP_STREAM_TYPE_SEEKABLE) { + /* Client request seek to 7 sec position (which belongs to 2nd period) + * Applictaion must provides corresponding buffer (of 2nd period) with Typo, Application @@ +626,3 @@ + msg_types = GST_MESSAGE_EOS; + + msg = gst_bus_timed_pop_filtered (GST_ELEMENT_BUS (pipe), -1, msg_types); Should probably check for errors @@ +643,3 @@ + + +GST_START_TEST (test_appsrc_custom_segment_twice) Same comments as above @@ +804,3 @@ + * - Application has been notified that it can control pipeline timeline + * (by setting 'driven-by-application' property) + * - Both appsrc segment and the custom segment have TIME format BYTES format and BYTES segment and other compatible pairs should also work though @@ +892,3 @@ + /* 1st sample includes buffer and segment */ + fail_unless (gst_app_src_push_sample (GST_APP_SRC (src), sample) + == GST_FLOW_OK); Shouldn't this give a GST_FLOW_ERROR then?
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 331151 [details] [review] [review]: > > Thanks, this looks like a good start :) We should double-check if the > seamless segment API of basesrc does the right thing though, it was > implemented for a completely different use-case. That's why it has the > "seamless" in the name. This here is not really for anything seamless > anymore. Thanks for your detailed review. Frankly, the "seamless" is confusing me because it's used only dvb related element, (I've never use it....). Can I ask why it's called seamless? (in the sense the SEGMENT for seamless playback?) What I've studied about previous use-cases, and they are - DVDNAV_NAV_PACKET event ==> might pts discont. happen. For the "seamless" playback, it need to push new segment without seek/flush - DVDNAV_HOP_CHANNEL ==> obviously "non-seamless" Both cases are use case of the seamless API if my understanding is correct. So I though this API can be used for seek/flush independent SEGMENT event. I don't think that my use case of this patch is not much different. - DASH multi-period causes PTS discont, so need new SEGMENT without seek/flush. - Playing with non-zero PTS is possible by using new SEGMENT (without seek/flush). I might be wrong, so please correct me > I'm not sure there should be a new property for this. It should be safe to > just always do that if the sample contains a valid segment I agree with you. I'll do rework. > @@ +804,3 @@ > + * - Application has been notified that it can control pipeline timeline > + * (by setting 'driven-by-application' property) > + * - Both appsrc segment and the custom segment have TIME format > > BYTES format and BYTES segment and other compatible pairs should also work > though I'll try to support BYTES format and BYTES segment, but could you explain more about compatible pairs?
(In reply to Seungha Yang from comment #8) > (In reply to Sebastian Dröge (slomo) from comment #7) > > Review of attachment 331151 [details] [review] [review] [review]: > > > > Thanks, this looks like a good start :) We should double-check if the > > seamless segment API of basesrc does the right thing though, it was > > implemented for a completely different use-case. That's why it has the > > "seamless" in the name. This here is not really for anything seamless > > anymore. > > Thanks for your detailed review. > Frankly, the "seamless" is confusing me because it's used only dvb related > element, (I've never use it....). Can I ask why it's called seamless? (in > the sense the SEGMENT for seamless playback?) > [...] It seems to do exactly what we need here, so let's use it :) It's going to output the new segment right before the next data. > > @@ +804,3 @@ > > + * - Application has been notified that it can control pipeline timeline > > + * (by setting 'driven-by-application' property) > > + * - Both appsrc segment and the custom segment have TIME format > > > > BYTES format and BYTES segment and other compatible pairs should also work > > though > > I'll try to support BYTES format and BYTES segment, but could you explain > more about compatible pairs? The segment format must be the same as the value of the format property on appsrc. That's what I meant.
Created attachment 331618 [details] [review] handle-pushing-custom-segments.patch
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 331151 [details] [review] [review]: > @@ +1202,2 @@ > if (!GST_IS_BUFFER (obj)) { > + if (!GST_IS_CAPS (obj)) { > > Store the GstSample in the queue, and then use > gst_sample_get_buffer/caps/segment() here instead Do we need to queueing all the members as a GstSample? I'd like to suggest that queueing only SEGMENT/CAPS using GstSample at gst_app_src_push_sample_internal() method. And queueing buffer without any modification from previous way... Actually, I tried some ways, but everything in gst_app_src_push_buffer_full () method is need at that moment of "push-sample" method/action is called (such as blocking, do timestamp and flow return).
Review of attachment 331618 [details] [review]: Looks better already, thanks for the update :) ::: gst-libs/gst/app/gstappsrc.c @@ +1871,3 @@ + /* initialized segment (i.e., gst_segment_init (segment, format)) + * we don't know whether it's pushed intentionally or not. ignore */ + GST_LOG_OBJECT (appsrc, "initialized segment, ignore segment"); It might very well be intentional, don't ignore it here. @@ +1876,3 @@ + /* if we are here, application pushed segment intentionally for + * random-access stream-type. Something wrong, return error */ + GST_LOG_OBJECT (appsrc, "random-access stream type, error"); Why? @@ +1904,3 @@ + gst_caps_unref (g_queue_pop_tail (priv->queue)); + } + gst_caps_replace (&priv->last_caps, caps); Maybe this code should be unified with the other caps setting code, that is, maybe always pass a GstSample around. Or does that actually complicate the code? @@ +1915,3 @@ + gst_sample_new (NULL, caps ? gst_caps_ref (caps) : NULL, segment, NULL); + GST_DEBUG_OBJECT (appsrc, "queueing sample %p", new_sample); + g_queue_push_tail (priv->queue, new_sample); Why not just enqueue the complete GstSample and take the buffer out of it on the other side?
Created attachment 332330 [details] [review] 0001-appsrc-Split-methods-to-be-more-reusable appsrc: Split methods to be more reusable This patch is a pre-work to rework push_sample method.
Created attachment 332331 [details] [review] 0002-appsrc-Handle-pushing-custom-segments
Created attachment 332332 [details] [review] 0000-sample-Modify-initialized-segment-format sample: Modify initialized segment format As a member variable of GstSample, segment has uncertainty. If there was no indication about segment, gst_sample_new () method will initialize segment with time-format. So, from the getter point of view, it's hard to figure out whether it's intentional segment or not. To clarify, let's initialize segment with undefined-format.
(In reply to Sebastian Dröge (slomo) from comment #12) > Review of attachment 331618 [details] [review] [review]: > > Looks better already, thanks for the update :) > > ::: gst-libs/gst/app/gstappsrc.c > @@ +1871,3 @@ > + /* initialized segment (i.e., gst_segment_init (segment, format)) > + * we don't know whether it's pushed intentionally or not. ignore */ > + GST_LOG_OBJECT (appsrc, "initialized segment, ignore segment"); > > It might very well be intentional, don't ignore it here. Because current GstSample implementation uses time-format as a default (please refer to gst_sample_new() method), I think there is no way to clarify whether this segment is intentionally or not. Unlike to the other member variable (caps/buffer/info, they can be NULL), segment always has value. I can cause uncertainty. Could we change default format of segment in GstSample to GST_FORMAT_UNDEFINED, in order to notify that this is not meaningful segment? Please review patch for GstSample > > @@ +1876,3 @@ > + /* if we are here, application pushed segment intentionally for > + * random-access stream-type. Something wrong, return error */ > + GST_LOG_OBJECT (appsrc, "random-access stream type, error"); > > Why? the restriction for random-access stream was removed :) > > @@ +1904,3 @@ > + gst_caps_unref (g_queue_pop_tail (priv->queue)); > + } > + gst_caps_replace (&priv->last_caps, caps); > > Maybe this code should be unified with the other caps setting code, that is, > maybe always pass a GstSample around. Or does that actually complicate the > code? > I tried to unify them on top of attachment 332330 [details] [review]. > @@ +1915,3 @@ > + gst_sample_new (NULL, caps ? gst_caps_ref (caps) : NULL, segment, > NULL); > + GST_DEBUG_OBJECT (appsrc, "queueing sample %p", new_sample); > + g_queue_push_tail (priv->queue, new_sample); > > Why not just enqueue the complete GstSample and take the buffer out of it on > the other side? complete GstSample will be queued.
Created attachment 332335 [details] [review] 0002-appsrc-Handle-pushing-custom-segments
Created attachment 333490 [details] [review] 0001-appsrc-Handle-pushing-custom-segments
Created attachment 333491 [details] [review] 0001-basesrc-Add-new-API-for-handling-external-SEGMENT
I'd like to revisit about _new_seamless_segment() method. I think we need a new API on basesrc for handling external SEGMENT. That is, _new_seamless_segment() cannot support flags on segment. And, segment.base cannot be correctly updated if input buffer was not timestamped, because segment.position in basesrc will be updated based on buffer timestamp. So, could we define new API on basesrc for accepting new segment? please review the patch about basesrc.
-- 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-base/issues/274.