Fixing gst bus watch issues

[lumip]

Second attempt for fixing #7120 (after the initial PR addressing that was reverted) while also addressing the issue of #7115 .

I did some digging and the problem that arose from the initial pull request seems to have been caused by replacing the uridecodebin in GstEnginePipeline::TransitionToNext. After removing the old uridecodebin from the pipeline, its reference count dropped to zero before its state was set to GST_STATE_NULL, which resulted in the crash reported in #7115.
This behavior is documented in the docs for gst_bin_remove: "Removes the element from the bin, unparenting it as well. Unparenting the element means that the element will be dereferenced, so if the bin holds the only reference to the element, the element will be freed in the process of removing it from the bin. If you want the element to still exist after removing, you need to call gst_object_ref before removing it from the bin."

I added a fix for this that increments the reference count on the old decodebin before removing it from the pipeline and accordingly modified GstElementDeleter::DeleteElement to decrement the reference count after setting its state to GST_STATE_NULL.

Now, the real question is why this was not a problem earlier. I have monitored the reference counts of all decodebins and noticed that previously they were never reaching zero. The change of the return value of BusCallback changed this. I'm not sure why this is but the current new behavior seems to be the correct one. I haven't had the opportunity to look into this in more depth, so if someone could verify that there was a memory leak of decodebins earlier and give an intuition why changing the return value of BusCallback might have solved this, that would be very helpful.

[lumip]

@AllKind @trougnouf @mohamedation @weearc @d10sfan @heldbrendel @ivovegter @shaygover @eris-price
If you have time and opportunity, could you be so kind as to check whether this works for you (i.e. the crashes no longer occur)? (and sorry for my earlier mess-up with this)

[trougnouf]

@AllKind @trougnouf @mohamedation @weearc @d10sfan @heldbrendel @ivovegter @shaygover @eris-price If you have time and opportunity, could you be so kind as to check whether this works for you (i.e. the crashes no longer occur)? (and sorry for my earlier mess-up with this)

Yes :) I've been on clementine 1.4.0rc1+759+gd033b38c4 with no issue, thank you!

[lumip]

@AllKind @trougnouf @mohamedation @weearc @d10sfan @heldbrendel @ivovegter @shaygover @eris-price If you have time and opportunity, could you be so kind as to check whether this works for you (i.e. the crashes no longer occur)? (and sorry for my earlier mess-up with this)

Yes :) I've been on clementine 1.4.0rc1+759+gd033b38c4 with no issue, thank you!

I meant the pending changes in this pull request, which are not included in 1.40rc1+759 yet ;)

[trougnouf]

Sorry I thought I was on the bug report :s
My connection is being too slow to download today. If anyone would like to test on Arch, here is the PKGBUILD
:

[trougnouf]

I got it :) So far no problem when skipping close to the end of songs and waiting for the next to play.

I will report back here if I encounter any issue (which would be at most 24 hours since I use Clementine all the time I'm awake)

[weearc]

@lumip Hi, I use the latest commit from https://github.com/lumip/Clementine/tree/fixing_gst_bus_watch_issues. Sometimes I got terminal output like this:

22:16:57.421 DEBUG GstEnginePipeline:896            Ignoring native format since pipeline is already running.
22:17:02.548 DEBUG GstEnginePipeline:657            New segment started, EOS will signal on next buffer discontinuity
22:17:02.548 DEBUG GstEnginePipeline:1039           Buffer discontinuity - emitting EOS

But it actually works perfectly as before. Thank you for your quick fix!

[AllKind]

@AllKind @trougnouf @mohamedation @weearc @d10sfan @heldbrendel @ivovegter @shaygover @eris-price If you have time and opportunity, could you be so kind as to check whether this works for you (i.e. the crashes no longer occur)? (and sorry for my earlier mess-up with this)

I don't build from source, so I don't think I can check. But of course thank you for your work!

[lumip]

@AllKind @trougnouf @mohamedation @weearc @d10sfan @heldbrendel @ivovegter @shaygover @eris-price If you have time and opportunity, could you be so kind as to check whether this works for you (i.e. the crashes no longer occur)? (and sorry for my earlier mess-up with this)

I don't build from source, so I don't think I can check. But of course thank you for your work!

Hi @AllKind , if you are interested, you can download binaries from the github build action for this change from here (scroll down to the bottom of the page to the list titled "Artifacts" and pick the one you need). However, since we've already got some reports that this is working, there's no urgent need for you to do so.

[AllKind]

Ok, did that. Seems to work with the bionic build on Linux Mint 19.3.
Thanks!