GNOME Bugzilla – Bug 766607
player: problems with unit tests
Last modified: 2016-06-06 08:22:15 UTC
I spotted this couple of issues with tests/check/libs/player.c: - It doesn't work GST_DEBUG, I have a patch for this one. - Some tests, including at least test_play_media_info, are sometimes failing. It's a race when unreffing the player doesn't drop the last reference (because an internal operation is keeping a temporary ref) and then the media_info_updated_cb callback is called with a TestPlayerState pointer which is now invalid. Not sure what's the proper way to fix this one.
Created attachment 328138 [details] [review] player: use gst_check_init() in test Calling GST_DEBUG() in test rely on the default category to be defined, which is done in gst_check_init().
Comment on attachment 328138 [details] [review] player: use gst_check_init() in test Attachment 328138 [details] pushed as 0f19a48 - player: use gst_check_init() in test
(In reply to Guillaume Desmottes from comment #0) > - Some tests, including at least test_play_media_info, are sometimes > failing. It's a race when unreffing the player doesn't drop the last > reference (because an internal operation is keeping a temporary ref) and > then the media_info_updated_cb callback is called with a TestPlayerState > pointer which is now invalid. Not sure what's the proper way to fix this one. The test should a) call gst_player_stop() at the end (not solving the problem but still), and b) disconnect from all signal it connected to before it makes the player go away.
Created attachment 328384 [details] [review] player: don't call gst_player_set_subtitle_uri twice in test The test_play_external_suburi_cb callback is called during our call to gst_player_set_subtitle_uri(). As the step count was not incremeted yet at this stage we were re-entering the first if block a second time.
Created attachment 328385 [details] [review] player: fix suburi getter debug message The property is a string, not a boolean.
Created attachment 328386 [details] [review] player: stop player and disconnect sigs in test Prevent the callback to be called after TestPlayerState has been destroyed.
Created attachment 328387 [details] [review] player: test: wait until the player has actually be stopped This ensures that the player has no pending operation left and so we'll destroy it when dropping our ref, making the leak detector a happy detector.
Comment on attachment 328384 [details] [review] player: don't call gst_player_set_subtitle_uri twice in test You mean the callback is reentrant? gst_player_set_subtitle_uri() calls the callback again directly? That seems suspicious
Comment on attachment 328386 [details] [review] player: stop player and disconnect sigs in test Maybe we should also make GstPlayer not emit any signals after stop, other than the state change signal
Created attachment 328440 [details] [review] player: inhibit signals after stop has been called
(In reply to Sebastian Dröge (slomo) from comment #8) > Comment on attachment 328384 [details] [review] [review] > player: don't call gst_player_set_subtitle_uri twice in test > > You mean the callback is reentrant? gst_player_set_subtitle_uri() calls the > callback again directly? That seems suspicious Yes. We first get STATE_CHANGED (playing) and then call gst_player_set_subtitle_uri(). While it's running we get a POSITION_UPDATED, as the state is still playing and step hasn't been incremented yet we re-enter the block. Another way to fix this could be to add a "change == STATE_CHANGE_STATE_CHANGED" check before entering into the first if block. (In reply to Sebastian Dröge (slomo) from comment #9) > Comment on attachment 328386 [details] [review] [review] > player: stop player and disconnect sigs in test > > Maybe we should also make GstPlayer not emit any signals after stop, other > than the state change signal Something like the patch I just attached?
(In reply to Guillaume Desmottes from comment #11) > (In reply to Sebastian Dröge (slomo) from comment #8) > > Comment on attachment 328384 [details] [review] [review] [review] > > player: don't call gst_player_set_subtitle_uri twice in test > > > > You mean the callback is reentrant? gst_player_set_subtitle_uri() calls the > > callback again directly? That seems suspicious > > Yes. We first get STATE_CHANGED (playing) and then call > gst_player_set_subtitle_uri(). While it's running we get a POSITION_UPDATED, > as the state is still playing and step hasn't been incremented yet we > re-enter the block. > Another way to fix this could be to add a "change == > STATE_CHANGE_STATE_CHANGED" check before entering into the first if block. But the signals should all come from the same thread! :) There should be no other signals while we're in the signal handler of another one. > (In reply to Sebastian Dröge (slomo) from comment #9) > > Comment on attachment 328386 [details] [review] [review] [review] > > player: stop player and disconnect sigs in test > > > > Maybe we should also make GstPlayer not emit any signals after stop, other > > than the state change signal > > Something like the patch I just attached? Does it ensure that we a) get the state_changed=STOPPED signal and b) the state_changed=BUFFERING signal next time we start playing something, and c) we get the uri_loaded signal for the next run too?
(In reply to Sebastian Dröge (slomo) from comment #12) > > (In reply to Sebastian Dröge (slomo) from comment #9) > > > Comment on attachment 328386 [details] [review] [review] [review] [review] > > > player: stop player and disconnect sigs in test > > > > > > Maybe we should also make GstPlayer not emit any signals after stop, other > > > than the state change signal > > > > Something like the patch I just attached? > > Does it ensure that we a) get the state_changed=STOPPED signal and b) the > state_changed=BUFFERING signal next time we start playing something, and c) > we get the uri_loaded signal for the next run too? It would be good to add a test for this btw, I think all current tests only go to stop() once and never try getting up again.
See this for the suburi issue: commit 3af9fd7322ed3ff69abe0eecf480707ad05f0f7a Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed May 25 09:53:15 2016 +0300 player: Don't set subtitle URI directly but dispatch to the player thread Otherwise we do state changes and everything in the application thread, which might block and more important can cause reentrant signals. https://bugzilla.gnome.org/show_bug.cgi?id=766607 For all the other patches, I think the inhibiting of the signals makes sense if done correctly. But nonetheless the test would then still have to wait for the final stopped state to be reached. Can you update the patches accordingly and squash together as makes sense?
(In reply to Sebastian Dröge (slomo) from comment #14) > See this for the suburi issue: > > commit 3af9fd7322ed3ff69abe0eecf480707ad05f0f7a > Author: Sebastian Dröge <sebastian@centricular.com> > Date: Wed May 25 09:53:15 2016 +0300 > > player: Don't set subtitle URI directly but dispatch to the player thread > > Otherwise we do state changes and everything in the application thread, > which > might block and more important can cause reentrant signals. > > https://bugzilla.gnome.org/show_bug.cgi?id=766607 Indeed that fixes the issue. Dropping my patch then.
Great, thanks for confirming :) Are you currently updating the other patch about the signals?
(In reply to Sebastian Dröge (slomo) from comment #16) > Great, thanks for confirming :) Are you currently updating the other patch > about the signals? Yep, on it!
Created attachment 328510 [details] [review] player: handle uri-loaded in test Had to adapt the existing tests because of this new callback.
Created attachment 328511 [details] [review] player: inhibit signals after gst_player_stop() has been called Also wait for the state change to STOP to have been announced before destroying the player so it won't appear as leaked by leak detector tools.
Review of attachment 328511 [details] [review]: ::: gst-libs/gst/player/gstplayer.c @@ +2744,3 @@ + g_mutex_lock (&self->lock); + self->inhibit_sigs = FALSE; Also has to happen for gst_player_pause(). Main problem I see here is the uri-loaded signal. This one is triggered even in stopped state. It happens automatically from the player thread when setting an URI, so you would inhibit that one now too which seems not very desireable
Created attachment 328545 [details] [review] player: inhibit signals after gst_player_stop() has been called Also wait for the state change to STOP to have been announced before destroying the player so it won't appear as leaked by leak detector tools.
Attachment 328510 [details] pushed as b00f0d0 - player: handle uri-loaded in test Attachment 328545 [details] pushed as 0467923 - player: inhibit signals after gst_player_stop() has been called
Hi the last patch ("inhibit signals") causes me problems when starting the pipeline by calling gst_player_pause then all signals are not triggered since inhibit_sigs flags is set to TRUE. I think it should set it to TRUE only when the pause state has been reached.
pause() should work the same way as play() with regard to the signals. Do you want to provide a patch?
This should actually fix it: commit a40ecc11689e20cf4a9145903beb3c8c0a1490a8 Author: Sebastian Dröge <sebastian@centricular.com> Date: Mon Jun 6 11:13:00 2016 +0300 player: pause() should not inhibit signals but work exactly like play() https://bugzilla.gnome.org/show_bug.cgi?id=766607#c23
Thx.