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 750104 - rtmpsink: Do not crash when receiving buffers after GST_FLOW_ERROR
rtmpsink: Do not crash when receiving buffers after GST_FLOW_ERROR
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-29 13:10 UTC by Vivia Nikolaidou
Modified: 2015-05-29 14:33 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch file (5.49 KB, patch)
2015-05-29 13:10 UTC, Vivia Nikolaidou
none Details | Review
Patch file after comments (4.05 KB, patch)
2015-05-29 13:35 UTC, Vivia Nikolaidou
committed Details | Review

Description Vivia Nikolaidou 2015-05-29 13:10:01 UTC
Created attachment 304256 [details] [review]
Patch file

If the RTMP URI is invalid, the rtmpsink will return GST_FLOW_ERROR.
If it still receives buffers after that, it shouldn't crash.
Comment 1 Jan Schmidt 2015-05-29 13:32:51 UTC
Review of attachment 304256 [details] [review]:

Thanks! good catch. A couple of comments.

::: ext/rtmp/gstrtmpsink.c
@@ +187,1 @@
 

I think this would be better if it were named "have_write_error"  and the logic inverted.

@@ +220,3 @@
+    GST_ELEMENT_ERROR (sink, RESOURCE, WRITE, (NULL), ("Failed to write data"));
+    return GST_FLOW_ERROR;
+  }

This should definitely go in as-is - silly oversight  setting the rtmp object to null on a connect error . I'm surprised more people haven't complained.

@@ +256,3 @@
   }
 
+  if (sink->keep_pushing) {

Just check the condition and jump to write_error to minimise the diff

@@ +403,3 @@
+static GstStateChangeReturn
+gst_rtmp_sink_change_state (GstElement * element, GstStateChange transition)
+{

This change_state function is unnecessary - the variable will already be set in start() above.
Comment 2 Vivia Nikolaidou 2015-05-29 13:35:17 UTC
Created attachment 304258 [details] [review]
Patch file after comments

Thanks for the comments. New patch attached.
Comment 3 Jan Schmidt 2015-05-29 14:27:46 UTC
Thanks - pushed :)