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 794149 - basesrc: regression with giosrc ssh:// protocol
basesrc: regression with giosrc ssh:// protocol
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal blocker
: 1.13.91
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-03-07 10:32 UTC by Tim-Philipp Müller
Modified: 2018-03-08 13:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
log (3.02 KB, text/plain)
2018-03-07 10:48 UTC, Sebastian Dröge (slomo)
  Details
basesrc: No need to stop flushing in start_complete (1.36 KB, patch)
2018-03-07 16:22 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
basesrc: Balance unlock/unlock_stop in _src_stop() (1.12 KB, patch)
2018-03-07 16:22 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Tim-Philipp Müller 2018-03-07 10:32:50 UTC
$ totem ssh://server/path/to/file

$ gst-play-1.0 ssh://server/path/to/file

Don't seem to work any more in git master, whereas the same works with 1.12.4.
Comment 1 Sebastian Dröge (slomo) 2018-03-07 10:44:24 UTC
This is caused by

commit 523de1a9dc7b7f79c78120bed15c364336f067cb
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Jun 1 10:36:26 2017 -0400

    basesrc: Don't hold LIVE_LOCK in create/alloc/fill
    
    Holding this lock on live source prevents the source from changing
    the caps in ::create() without risking a deadlock. This has consequences
    as the LIVE_LOCK was replacing the STREAM_LOCK in many situation. As a
    side effect:
    
    - We no longer need to unlock when doing play/pause as the LIVE_LOCK
      isn't held. We then let the create() call finish, but will block if
      the state have changed meanwhile. This has the benefit that
      wait_preroll() calls in subclass is no longer needed.
    - We no longer need to change the state to unlock, simplifying the
      set_flushing() interface
    - We need different handling for EOS depending if we are in push or pull
      mode.
    
    This patch also document the locking of each private class member and
    the locking order.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=783301



What happens is that unlock() is called, and then afterwards start() is called without unlock_stop() first. So the cancellable is still cancelled, and all the GIO operations in start() just error out because of that.
Comment 2 Sebastian Dröge (slomo) 2018-03-07 10:48:26 UTC
Created attachment 369404 [details]
log

Relevant part of the log. Line 4 and 5 seem to be in the wrong order (first unlock_stop is called, then unlock).
Comment 3 Nicolas Dufresne (ndufresne) 2018-03-07 16:22:18 UTC
Created attachment 369410 [details] [review]
basesrc: No need to stop flushing in start_complete

The flushing state is handled a bit differently, there is no need
to stop flushing in start_complete. This would other result in
unlock_stop being called without unlock_start.

Unlike what the old comment says, there is no need to take the live
lock here, we are still single threaded at this point (app thread
or the state change thread). Also, we will wait for playing state
in create/getrange, no need to do that twice.
Comment 4 Nicolas Dufresne (ndufresne) 2018-03-07 16:22:23 UTC
Created attachment 369411 [details] [review]
basesrc: Balance unlock/unlock_stop in _src_stop()

Otherwise it's possible that we won't be able to start again
depending the implementation. We do start/stop in normal use cases
whenever GST_QUERY_SCHEDULING happens before we are started.
Comment 5 Nicolas Dufresne (ndufresne) 2018-03-07 16:22:55 UTC
This passes the unit tests and fixes the regression we see here.
Comment 6 Nicolas Dufresne (ndufresne) 2018-03-08 02:07:12 UTC
Just ran validate locally and it's clean to.
Comment 7 Nicolas Dufresne (ndufresne) 2018-03-08 13:30:13 UTC
Attachment 369410 [details] pushed as 12c5d90 - basesrc: No need to stop flushing in start_complete
Attachment 369411 [details] pushed as cbd03e2 - basesrc: Balance unlock/unlock_stop in _src_stop()