GNOME Bugzilla – Bug 790674
gst-validate failures on RTSP tests
Last modified: 2018-03-14 08:35:38 UTC
Since this commit, almost all RTSP-related tests have started failing: https://cgit.freedesktop.org/gstreamer/gst-rtsp-server/commit/?id=a7732a68e8bc6b4ba15629c652056c240c624ff0 https://ci.gstreamer.net/job/GStreamer-master-validate/5591/testReport/ The failures all seem to be in the rtsp-server itself.
To reproduce: gst-validate-launcher -fs -j4 -m -t rtsp
It can also be reproduced with just the rtsp-server examples: Terminal 1: gst-rtsp-server/examples$ ./test-uri ~/foo.mp4 stream ready at rtsp://127.0.0.1:8554/test Terminal 2: $ gst-play-1.0 rtsp://127.0.0.1:8554/test => Error Terminal 1: => 0:00:10.406825269 ERROR default rtsp-sdp.c:548:gst_rtsp_sdp_from_stream: stream 0x7faa4c0102b0 has no caps 0:00:10.406851797 ERROR default rtsp-sdp.c:482:gst_rtsp_sdp_from_media: could not get SDP from stream 0x7faa4c0102b0 0:00:10.406857996 ERROR default rtsp-sdp.c:521:gst_rtsp_sdp_from_media: could not get SDP from media 0x7faa68012210 0:00:10.406862290 ERROR rtspclient rtsp-client.c:2723:create_sdp: client 0x5647591b0180: could not create SDP 0:00:10.406870340 ERROR rtspclient rtsp-client.c:2858:handle_describe_request: client 0x5647591b0180: can't create SDP
I'll have a look at this problem.
Pushed a commit that adds extra checks to the media unit test (which now fail)
This problem is related to the dynamic payloader case. We should wait for GstRTSPStreamBlocking message before doing a position query.
Created attachment 364190 [details] [review] rtsp-media: Fix ASYNC_DONE handling When receiving ASYNC_DONE in PREPARING state, we only want to switch to PREPARED if: * We weren't currently blocking waiting for data * Or if we were, all streams are currently blocking Fixes some initial issues, but there are still failures with seeking
The above patch fixes *simple* playback cases. It completely fails once we start doing slightly more advanced uses (the various seek validate tests for example). The problem seems to be some big confusion when "unsuspending" after a PAUSE. I never see the pipeline being set to PLAYING.
Created attachment 364262 [details] [review] test-patch
Created attachment 364287 [details] [review] Corrected ASYNC_DONE handling
Created attachment 364288 [details] [review] Removed fakesink elements
Created attachment 364292 [details] [review] Fix thread pool leak
Created attachment 364293 [details] [review] Fix handling in default_unsuspend()
All validate.rtsp tests pass with the above patch fixes. There are still some problems with validate.rtsp2.playback.seek_* tests.
The remaining problems with RTSP2 is because for some reason the server is *not* reporting those streams as seekable.
Commited all other patches in the time being. Remaining issue is that rtsp2 corner case. commit bb29d2e2ee46a1e1b3ffc52a3c0da91e42d82e0b (HEAD -> master, origin/master, origin/HEAD) Author: Patricia Muscalu <patricia@axis.com> Date: Thu Nov 23 20:34:03 2017 +0100 rtsp-media: Fix handling in default_unsuspend() Handle the case when streams are not blocked and media is suspended from PAUSED. Change-Id: I2f3d222ea7b9b20a0732ea5dc81a32d17ab75040 https://bugzilla.gnome.org/show_bug.cgi?id=790674 commit de930f2e4d74f1cbf1679aa03ff6f603a5eff5ef Author: Patricia Muscalu <patricia@axis.com> Date: Thu Nov 23 18:51:21 2017 +0100 check/media: Fix thread pool leak. Change-Id: I0f92b1caca0ee518ae64a7dacfbd28a214c3eea1 https://bugzilla.gnome.org/show_bug.cgi?id=790674 commit 132e00adfd0333be082594bf7e64d3b96d95d263 Author: Patricia Muscalu <patricia@axis.com> Date: Thu Nov 23 18:39:44 2017 +0100 rtsp-media: Removed fakesink elements There is not need of adding fakesink elements to the media pipeline in the dynamic-payloader case. The media pipeline itself is dynamically updated with the receiver and sender parts that are based on the client transport information known after SETUP has been received. Change-Id: I4e88c9b500c04030669822f0d03b1842913f6cb9 https://bugzilla.gnome.org/show_bug.cgi?id=790674 commit ac6169d50a6e1bf03323b7bada4bd43f4126b012 Author: Patricia Muscalu <patricia@axis.com> Date: Thu Nov 23 09:10:54 2017 +0100 rtsp-media: Corrected ASYNC_DONE handling Media is complete when all the transport based parts are added to the media pipeline. At this point ASYNC_DONE is posted by the media pipeline and media is ready to enter the PREPARED state. Change-Id: I50fb8dfed88ebaf057d9a35fca2d7f0a70e9d1fa https://bugzilla.gnome.org/show_bug.cgi?id=790674
Created attachment 364344 [details] [review] rtsp-media: Enable seeking query before pipeline is complete SDP are now provided *before* the pipeline is fully complete. In order to know whether a media is seekable or not therefore requires asking the invididual streams.
This latest commit fixes the remaining RTSP2 problems *but* causes weird errors in the unit test.
The problem is that when gst_rtsp_media_create_stream() is called, the src pad of the payloader is linked and it shouldn't be. Just unref pad in gst_rtsp_stream_seekable() to solve this problem.
Created attachment 364376 [details] [review] rtsp-media: Enable seeking query before pipeline is complete SDP are now provided *before* the pipeline is fully complete. In order to know whether a media is seekable or not therefore requires asking the invididual streams. API: gst_rtsp_stream_seekable
Created attachment 364377 [details] [review] check: Add seekability testing on medias Make sure that once GstRTSPMedia are prepared they returned the expected seekability results
(In reply to Patricia Muscalu from comment #18) > The problem is that when gst_rtsp_media_create_stream() is called, the src > pad of the payloader is linked and it shouldn't be. > Just unref pad in gst_rtsp_stream_seekable() to solve this problem. Well spotted :) Last patches fix the validate regression. Thanks !
(In reply to Edward Hervey from comment #21) > (In reply to Patricia Muscalu from comment #18) > > The problem is that when gst_rtsp_media_create_stream() is called, the src > > pad of the payloader is linked and it shouldn't be. > > Just unref pad in gst_rtsp_stream_seekable() to solve this problem. > > Well spotted :) Last patches fix the validate regression. Thanks ! Great work! Thank you very much for your help.
I've found some other issues: 1. media->priv->seekable is not protected in gst_rtsp_media_seekable (bug introduced before the mega-patch) => I'll send a patch 2. priv->blocking is incorrectly reset in set_blocked (rtsp-stream.c). I have written a unittest that actually exposes this issue. => I'll send a patch 3. I think that query_position() on media that contains only one active stream returns an incorrect value (I think that we have the same behavior in 1.12 as well). The same unittest from 2. can catch this problem. Should this problem be reported in a separate ticket?
Created attachment 364391 [details] [review] rtsp-media: Fix missing lock in gst_rtsp_media_seekable()
Created attachment 364392 [details] [review] rtsp-stream: Do not reset 'blocking' if stream is already blocked
Created attachment 364393 [details] [review] check/media: Add seekability test case: not all streams are active
Thanks for the additional patches ! For new/other issues, please open a different bugzilla entry. commit abeb896232be1aad8867093f0dbeed3a95f56200 (HEAD -> master, origin/master, origin/HEAD) Author: Patricia Muscalu <patricia@dovakhiin.com> Date: Sat Nov 25 20:34:16 2017 +0100 check/media: Add seekability test case: not all streams are active Media contains two streams but only one is complete and prepared for playing. https://bugzilla.gnome.org/show_bug.cgi?id=790674 commit caa3f1caac3d3ce43a86e27c06d3c4c7da15473c Author: Patricia Muscalu <patricia@dovakhiin.com> Date: Sat Nov 25 20:32:02 2017 +0100 rtsp-stream: Do not reset 'blocking' if stream is already blocked https://bugzilla.gnome.org/show_bug.cgi?id=790674 commit 0015791f8fe72e648cef264f49f6e3c9c224caf7 Author: Patricia Muscalu <patricia@dovakhiin.com> Date: Sat Nov 25 20:45:44 2017 +0100 rtsp-media: Fix missing lock in gst_rtsp_media_seekable() https://bugzilla.gnome.org/show_bug.cgi?id=790674
(In reply to Patricia Muscalu from comment #23) > 3. I think that query_position() on media that contains only one active > stream > returns an incorrect value (I think that we have the same behavior in > 1.12 as > well). > The same unittest from 2. can catch this problem. > Should this problem be reported in a separate ticket? We will report this specific problem in a new bugzilla ticket.