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 725898 - Lose data when producing data faster than sendt during tunneling rtps/rtp(TCP)
Lose data when producing data faster than sendt during tunneling rtps/rtp(TCP)
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
unspecified
Other Linux
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-03-07 13:55 UTC by Göran Jönsson
Modified: 2014-04-10 14:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (3.94 KB, patch)
2014-03-07 13:55 UTC, Göran Jönsson
committed Details | Review
patch (7.75 KB, patch)
2014-03-07 13:56 UTC, Göran Jönsson
committed Details | Review
patch (8.39 KB, patch)
2014-03-25 12:03 UTC, Göran Jönsson
none Details | Review
patch (2.33 KB, patch)
2014-03-25 12:04 UTC, Göran Jönsson
none Details | Review
gst-plugins-base patch (3.03 KB, patch)
2014-03-26 13:01 UTC, Göran Jönsson
none Details | Review
gst-plugins-base patch (790 bytes, patch)
2014-04-01 12:54 UTC, Göran Jönsson
committed Details | Review
gst-rtsp-server patch (4.94 KB, patch)
2014-04-01 12:59 UTC, Göran Jönsson
needs-work Details | Review
gst-rtsp-server patch (5.27 KB, patch)
2014-04-08 06:02 UTC, Göran Jönsson
rejected Details | Review

Description Göran Jönsson 2014-03-07 13:55:00 UTC
Created attachment 271227 [details] [review]
patch

Lose data when producing data faster than sendt during tunneling rtps7rtp (TCP) 

IF the server producing data faster than transferred to remote end then data ends up in a queue and finally the queue get full and throw away data.

I attach two patches for this matter.

wait.patch    : 
This provide a method for waiting until there is room in queue.

setwait.patch : 
This make it configurable to resend data that otherwise have been lost after waiting until there is room in queue.
Comment 1 Göran Jönsson 2014-03-07 13:56:26 UTC
Created attachment 271228 [details] [review]
patch
Comment 2 Wim Taymans 2014-03-10 16:31:00 UTC
commited with some changes:

 * changed function name
 * added macro to check for queue fullness
 * check for bytes limits as well
 * signal gcond when limits change
 * make function prototype like other functions with GstRTSPResult and GTimeVal
   timeout
 * handle invalid parameters like we handle them elsewhere
 * add to docs and defs
 * Add Since markers

commit 0b30fdbfbe577fda0db9bcaf2d824a6cdbbf2ea1
Author: Göran Jönsson <goranjn@axis.com>
Date:   Thu Mar 6 13:55:17 2014 +0100

    rtspconnection: gst_rtsp_watch_wait_backlog
    
    New method that wait until there is room in backlog queue.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=725898
Comment 3 Ognyan Tonchev (redstar_) 2014-03-18 16:07:11 UTC
just an idea, why not doing something like gst_rtsp_connection_poll (conn, GST_RTSP_EV_READ, 20) from the server in case of GST_RTSP_ENOMEM?
Wouldn't that be a better approach? This way we can also cancel the poll in case we want to, e.g. when client disconnects, and entirely skip gst_rtsp_watch_wait_backlog().
Comment 4 Göran Jönsson 2014-03-25 12:02:09 UTC
The problem sill remain. So

I adding two new patches and set one the old ones as obsolete.

The unblock_wait_queue.patch (gst-plugins-base) adding functionality to unblock  
the function gst_rtsp_watch_wait_backlog.

The set_wait_queue.patch (gst-rtsp-server) is.
* Using new name on function gst_rtsp_watch_wait_backlog
* Unblock gst_rtsp_watch_wait_backlog when close and finalizing.
* Change in while loop that resend when queue is full.
Comment 5 Göran Jönsson 2014-03-25 12:03:37 UTC
Created attachment 272843 [details] [review]
patch

This will be replaced
Comment 6 Göran Jönsson 2014-03-25 12:04:22 UTC
Created attachment 272844 [details] [review]
patch
Comment 7 Göran Jönsson 2014-03-26 13:01:42 UTC
Created attachment 272986 [details] [review]
gst-plugins-base patch
Comment 8 Wim Taymans 2014-03-27 11:59:24 UTC
(In reply to comment #3)
> just an idea, why not doing something like gst_rtsp_connection_poll (conn,
> GST_RTSP_EV_READ, 20) from the server in case of GST_RTSP_ENOMEM?

> Wouldn't that be a better approach? This way we can also cancel the poll in
> case we want to, e.g. when client disconnects, and entirely skip
> gst_rtsp_watch_wait_backlog().

It sounds like a good idea to integrate it into poll but then poll needs to take into account the backlog queue as well.
Comment 9 Wim Taymans 2014-03-27 12:34:32 UTC
I think it is also not racefree, you usually need a flush start and stop operation because else you might unblock the wait before you start the wait operation.
Comment 10 Wim Taymans 2014-03-28 08:36:25 UTC
commit 8d439edd7abbb9a6b30c1320734e7eb041136cba
Author: Wim Taymans <wtaymans@redhat.com>
Date:   Fri Mar 28 09:32:20 2014 +0100

    rtspconnection: add flush method
    
    Add a method to set/unset the flushing state that makes _wait_backlog()
    unlock.
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=725898
Comment 11 Wim Taymans 2014-03-28 08:37:40 UTC
(In reply to comment #8)
> (In reply to comment #3)
> > just an idea, why not doing something like gst_rtsp_connection_poll (conn,
> > GST_RTSP_EV_READ, 20) from the server in case of GST_RTSP_ENOMEM?
> 
> > Wouldn't that be a better approach? This way we can also cancel the poll in
> > case we want to, e.g. when client disconnects, and entirely skip
> > gst_rtsp_watch_wait_backlog().
> 
> It sounds like a good idea to integrate it into poll but then poll needs to
> take into account the backlog queue as well.

After some investigations, it doesn't sound like a good idea anymore. The backlog is at the watch level and poll at the lower connection level, best not to mix them and implement a simple flush method.
Comment 12 Göran Jönsson 2014-04-01 12:54:28 UTC
Created attachment 273399 [details] [review]
gst-plugins-base patch

Small corection
Comment 13 Göran Jönsson 2014-04-01 12:59:28 UTC
Created attachment 273400 [details] [review]
gst-rtsp-server patch

New patch in gst-rtsp-server

* Change due to change in a api gstrtspconnection.h
* Changes off mutexes in rtsp-stream.c not longer needed due to patch
  stream: release lock while pushing out packets
Comment 14 Göran Jönsson 2014-04-01 13:03:40 UTC
Added new patches for 
* gst-plugins-base .
  small correction.

* gst-rtsp-server
  Change because of chang in api gstrtspconnection.h
  
  Change because split off mutex in rtsp-stream.c not longer needed
  after patch "stream: release lock while pushing out packets"
Comment 15 Wim Taymans 2014-04-03 11:31:10 UTC
commit a483e9095505c6b0c2a07855770005ce09f74f9c
Author: Göran Jönsson <goranjn@axis.com>
Date:   Tue Apr 1 10:38:23 2014 +0200

    rtspconnection: Added gst_rtsp_watch_set_flushing to list.
    
    Added gst_rtsp_watch_set_flushing to list in file
    libgstrtsp.def
Comment 16 Göran Jönsson 2014-04-04 04:53:34 UTC
The gst-rtsp-server patch is wrong. pleasew do not commit.
Comment 17 Göran Jönsson 2014-04-08 06:02:42 UTC
Created attachment 273762 [details] [review]
gst-rtsp-server patch

Some correction done to previous gst-rtsp-server patch
Comment 18 Wim Taymans 2014-04-10 12:23:25 UTC
it does not seem to pass make check, gst/rtspserver hangs forever.
Comment 19 Wim Taymans 2014-04-10 13:22:07 UTC
The problem is that you set the watch to flushing before you send the teardown reply, which makes the reply never arrive at the client. What did you want to do here?
Comment 20 Wim Taymans 2014-04-10 14:13:05 UTC
I rewrote the patch a little like this:

commit 11369d38ef6bc6a054b6da521cb9e2dbe6226373
Author: Göran Jönsson <goranjn@axis.com>
Date:   Tue Apr 1 13:04:21 2014 +0200

    client: Add drop-backlog property
    
    When we have too many messages queued for a client (currently hardcoded
    to 100) we overflow and drop the messages. Add a drop-backlog property
    to control this behaviour. Setting this property to FALSE will retry
    to send the messages to the client by waiting for more room in the
    backlog.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=725898