GNOME Bugzilla – Bug 795330
pad: Handle changing sticky events in pad probes
Last modified: 2018-07-23 20:22:07 UTC
See commit message.
Created attachment 371040 [details] [review] debug: Make PADS debug background blue Red on red was... suboptimal!
Created attachment 371042 [details] [review] pad: Handle changing sticky events in pad probes In the case where the user sets a new padprobeinfo->data in a probe where the data is a sticky event, the new sticky event should be automatically sticked on the probed pad.
Review of attachment 371040 [details] [review]: lgtm
Review of attachment 371042 [details] [review]: The new behaviour makes sense to me, assuming you ran the test suite with valgrind I'd say go ahead apart from that one remark :) ::: gst/gstpad.c @@ +3811,2 @@ static gboolean +cleanup_event (GstPad * pad, PadEvent * ev, gint64 * prev_offset) That name is a bit non-descriptive, how about "reschedule_event" maybe ?
I fixed a minor leak in the test and valgrind passes just fine. Attachment 371040 [details] pushed as 588e054 - debug: Make PADS debug background blue Attachment 371042 [details] pushed as 11e0f45 - pad: Handle changing sticky events in pad probes
This causes refcounting problems where stored sticky events are destroyed but still stored on the pad. Looking into a fix.
Review of attachment 371042 [details] [review]: ::: gst/gstpad.c @@ +5268,1 @@ GstPadProbeType type) This new signature makes not much sense ownership-wise. It takes ownership of the passed event, might replace the event with a new one, and then gives back the (possibly changed) event at the end if all succeeded but in none of the early exits. And the caller does not own the returned event, it might even be destroyed already (if it was changed and make_writable() created a new event). And that's exactly the problem here, you might then store a newly created event that you don't own in ev->event (and leaked the original one there).
That said, I disagree with the change in principle so not going to debug it any further. Pad probes are on the "outside" of a pad (same for pad offsets). When pushing out of a pad, any changes (from a pad offset or probe) should only be applied to the event that actually goes out of the pad and not to the event that is stored inside the pad. Similarly when sending an event to a pad it is in reverse and there's a different bug about this being broken in annoying ways: https://bugzilla.gnome.org/show_bug.cgi?id=765049
Review of attachment 371042 [details] [review]: ::: tests/check/gst/gstpad.c @@ +3327,3 @@ + + srcpad = gst_element_get_static_pad (harness->element, "src"); + stream_start = gst_pad_get_sticky_event (srcpad, GST_EVENT_STREAM_START, 0); Basically this event should've never been changed. It's inside the fakesrc, it's before your pad probe has ever seen it and your pad probe should not change the event upstream of the probe. Interestingly this problem is very similar to the ownership problem in the changed send_event(), that probably tells us something :) You want to change something that is stored "before you" and where you don't own the storage.
Note that in https://bugzilla.gnome.org/show_bug.cgi?id=765049#c15 I basically said that your solution here is correct. But I disagree with that now. It seems wrong to modify events in these cases. The modified ones should only be stored for sink pads for downstream events, and for src pads for upstream events. Those are the events that pass through the actual pad probe (and pad offset) before arriving at the pad, the others don't
Review of attachment 371042 [details] [review]: ::: tests/check/gst/gstpad.c @@ +3327,3 @@ + + srcpad = gst_element_get_static_pad (harness->element, "src"); + stream_start = gst_pad_get_sticky_event (srcpad, GST_EVENT_STREAM_START, 0); I understand what you mean but what you describe is broken to me as if for some reason the sticky event is pushed again (unsticked), it will push the wrong version of what dowstream should receive, which was exactly my problem here iirc.
(In reply to Thibault Saunier from comment #11) > Review of attachment 371042 [details] [review] [review]: > > ::: tests/check/gst/gstpad.c > @@ +3327,3 @@ > + > + srcpad = gst_element_get_static_pad (harness->element, "src"); > + stream_start = gst_pad_get_sticky_event (srcpad, GST_EVENT_STREAM_START, > 0); > > I understand what you mean but what you describe is broken to me as if for > some reason the sticky event is pushed again (unsticked), it will push the > wrong version of what dowstream should receive, which was exactly my problem > here iirc. Why would it receive the wrong version? Your pad probe would replace the event again in the same way
(In reply to Sebastian Dröge (slomo) from comment #12) > (In reply to Thibault Saunier from comment #11) > > Review of attachment 371042 [details] [review] [review] [review]: > > > > ::: tests/check/gst/gstpad.c > > @@ +3327,3 @@ > > + > > + srcpad = gst_element_get_static_pad (harness->element, "src"); > > + stream_start = gst_pad_get_sticky_event (srcpad, GST_EVENT_STREAM_START, > > 0); > > > > I understand what you mean but what you describe is broken to me as if for > > some reason the sticky event is pushed again (unsticked), it will push the > > wrong version of what dowstream should receive, which was exactly my problem > > here iirc. > > Why would it receive the wrong version? Your pad probe would replace the > event again in the same way In the testcase yes, in the real world example it was not the case as the probe was removed, and without that patch I believe that the STREAM_START was unsticked before each buffer or something like that (I do not have that code anymore so my memory might be wrong and note that I do not need that patch anymore as I reworked the whole thing)
It also would seem wrong to me that the same pad probe would receive the same event twice in different versions, the second time with its own changes applied already
(In reply to Thibault Saunier from comment #13) > In the testcase yes, in the real world example it was not the case as the > probe was removed, Nothing is preventing upstream from sending the same event again, you can't just remove the pad probe and expect your changes to be there forever :) > and without that patch I believe that the STREAM_START > was unsticked before each buffer or something like that (I do not have that > code anymore so my memory might be wrong and note that I do not need that > patch anymore as I reworked the whole thing) That sounds like a bug we should fix then
(In reply to Sebastian Dröge (slomo) from comment #14) > It also would seem wrong to me that the same pad probe would receive the > same event twice in different versions, the second time with its own changes > applied already You have a point here indeed.
Ok, so let's revert this one and update the other bug with the new findings from here and mark this one as a duplicate of the other one?
(In reply to Sebastian Dröge (slomo) from comment #15) > (In reply to Thibault Saunier from comment #13) > > > In the testcase yes, in the real world example it was not the case as the > > probe was removed, > > Nothing is preventing upstream from sending the same event again, you can't > just remove the pad probe and expect your changes to be there forever :) Well, in my case I know when a stream-start should happen, and if I know that the sticky events are updated with the probe, then yes I can do that without any problem. Tbh I am not sure what is conceptually expected in that case, ie. sticked even are before probes or after probes? To me both option make sense and have their pros and cons :-) > > and without that patch I believe that the STREAM_START > > was unsticked before each buffer or something like that (I do not have that > > code anymore so my memory might be wrong and note that I do not need that > > patch anymore as I reworked the whole thing) > > That sounds like a bug we should fix then Well, this was one of the point of that patch ;)
(In reply to Sebastian Dröge (slomo) from comment #17) > Ok, so let's revert this one and update the other bug with the new findings > from here and mark this one as a duplicate of the other one? Well, in the other bug you say we should do exactly what I implemented here :-) That being said, I am fine reverting for now.
(In reply to Thibault Saunier from comment #18) > Tbh I am not sure what is conceptually expected in that case, ie. sticked > even are before probes or after probes? To me both option make sense and > have their pros and cons :-) Probes are something from the outside, so should not really have an effect on the internals of a pad/element. (In reply to Thibault Saunier from comment #19) > (In reply to Sebastian Dröge (slomo) from comment #17) > > Ok, so let's revert this one and update the other bug with the new findings > > from here and mark this one as a duplicate of the other one? > > Well, in the other bug you say we should do exactly what I implemented here > :-) That being said, I am fine reverting for now. I update that one then and also explain in the revert commit :)
commit 2aa9ad9c62d3071c90e837bc8a5923b6f16d7fcd (HEAD -> master, origin/master, origin/HEAD) Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Jul 23 23:17:54 2018 +0300 Revert "pad: Handle changing sticky events in pad probes" This reverts commit 11e0f451eb498e92d05d8208f7217625dc62848b. When pushing a sticky event out of a pad with a pad probe or pad offset, those should not be applied to the event that is actually stored in the event but only in the event sent downstream. The pad probe and pad offsets are conceptually *after* the pad, added by external code and should not affect any internal state of pads/elements. Also storing the modified event has the side-effect that a re-sent event would arrive with any previous modifications done by the same pad probe again inside that pad probe, and it would have to check if its modifications are already applied or not. For sink pads and generally for events arriving in a pad, some further changes are still needed and those are tracked in https://bugzilla.gnome.org/show_bug.cgi?id=765049 In addition, the commit also had a refcounting problem with events, causing already destroyed events to be stored inside pads. *** This bug has been marked as a duplicate of bug 765049 ***