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 766525 - framepositionner: add a weak ref on track element to know when it is finalized
framepositionner: add a weak ref on track element to know when it is finalized
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-editing-services
git master
Other Linux
: Normal minor
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-05-16 16:13 UTC by Aurélien Zanelli
Modified: 2016-05-16 18:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
framepositionner: add a weak ref on track element to know when it is finalized (1.51 KB, patch)
2016-05-16 16:18 UTC, Aurélien Zanelli
committed Details | Review

Description Aurélien Zanelli 2016-05-16 16:13:42 UTC
Currently, framepositionner store a pointer to the GstTrackElement given in ges_frame_positioner_set_source_and_filter() and install a signal handler to be notified fo GstTrackElement property change which is disconnected on finalize.
But we don't own any ref on GstTrackElement so when framepositionner instance is finalized, GstTrackElement could be already freed leading in the best case, the deconnection to raise a critical message.

That's happens when applying patch from bug 766449. When the timeline is finalized and ressources cleanup, the GstTrackElement is freed before the GstFramePositionner.

This can be tested with the following code snippet:

GESTimeline *timeline = ges_timeline_new_audio_video ();
GESLayer *layer = ges_timeline_append_layer (timeline);
GESClip *clip = ges_test_clip_new ();
ges_layer_add_clip (layer, clip);
gst_object_unref (timeline);
Comment 1 Thibault Saunier 2016-05-16 16:18:09 UTC
Until now it was correct to concider the FramePositionner would always be destroyed before its parent GESTrackElement.

If that changes it is right to add a weak ref, but you should probably do so in the same patch as where you change that behaviour.
Comment 2 Aurélien Zanelli 2016-05-16 16:18:37 UTC
Created attachment 327994 [details] [review]
framepositionner: add a weak ref on track element to know when it is finalized

Add a weak reference on GstTrackElement to know when it's finalized.
Comment 3 Aurélien Zanelli 2016-05-16 18:22:13 UTC
(In reply to Thibault Saunier from comment #1)
> Until now it was correct to concider the FramePositionner would always be
> destroyed before its parent GESTrackElement.
> 
> If that changes it is right to add a weak ref, but you should probably do so
> in the same patch as where you change that behaviour.

The patch in bug 766449 only reveals this issue by fixing element leaks (including framepositionner), it does not change the destroy order. So I would prefer not to merge this with the other patch.

I quickly check the current life of track element and framepositionner in the example I give, here is my current understanding:
We create a new GESTestClip which create a new GESVideoTestSource.
GESVideoTestSource object instantiate a 'framepositionner' and set itself as source. Then clip is added to the layer.
When timeline is disposed, we first remove layers which causes GESVideoTestSource to be disposed. Then we remove tracks and the disposal of the videotrack causes the last reference to the framepositionner to be dropped.

If we expect FramePositionner to be destroyed before its linked GESTrackElement and so not live after it, it may be worth analyzing this issue further.
Comment 4 Thibault Saunier 2016-05-16 18:36:49 UTC
Sounds about right actually
Comment 5 Thibault Saunier 2016-05-16 18:43:49 UTC
Attachment 327994 [details] pushed as df9921f - framepositionner: add a weak ref on track element to know when it is finalized