Useful to report gst warnings and errors, like in the AudioFileReader implementation.
Created attachment 179941 [details] AudioDestination: GstMessage handler
Comment on attachment 179941 [details] AudioDestination: GstMessage handler View in context: https://bugs.webkit.org/attachment.cgi?id=179941&action=review > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:68 > + GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > + gst_bus_add_signal_watch(bus.get()); > + g_signal_connect(bus.get(), "message", G_CALLBACK(messageCallback), this); See below. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:91 > + GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > + g_signal_handlers_disconnect_by_func(bus.get(), reinterpret_cast<gpointer>(messageCallback), this); Why not just use a raw pointer? By using GRefPtr you are doing a ref and unref. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:140 > + g_warning("Warning: %d, %s. Debug output: %s", error->code, error->message, debug.get()); Wouldn't it make sense to use gst_debug_log here?
(In reply to comment #2) > (From update of attachment 179941 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179941&action=review > > > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:68 > > + GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > > + gst_bus_add_signal_watch(bus.get()); > > + g_signal_connect(bus.get(), "message", G_CALLBACK(messageCallback), this); > > See below. > > > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:91 > > + GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > > + g_signal_handlers_disconnect_by_func(bus.get(), reinterpret_cast<gpointer>(messageCallback), this); > > Why not just use a raw pointer? By using GRefPtr you are doing a ref and unref. The bus returned by gst_pipeline_get_bus() needs to be unreffed after usage. > > > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:140 > > + g_warning("Warning: %d, %s. Debug output: %s", error->code, error->message, debug.get()); > > Wouldn't it make sense to use gst_debug_log here? Well I wanted to use g_warning here so the user sees the message in the console, this is rather minimal error reporting but less hidden than GST_DEBUG :) I used the same approach in the AudioFileReader BTW, so any change needed in this patch would need to be done there as well.
Comment on attachment 179941 [details] AudioDestination: GstMessage handler View in context: https://bugs.webkit.org/attachment.cgi?id=179941&action=review >>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:68 >>> + g_signal_connect(bus.get(), "message", G_CALLBACK(messageCallback), this); >> >> See below. > > The bus returned by gst_pipeline_get_bus() needs to be unreffed after usage. Ah, then you are actually leaking it because you aren't using adoptGRef. >>> Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:140 >>> + g_warning("Warning: %d, %s. Debug output: %s", error->code, error->message, debug.get()); >> >> Wouldn't it make sense to use gst_debug_log here? > > Well I wanted to use g_warning here so the user sees the message in the console, this is rather minimal error reporting but less hidden than GST_DEBUG :) > > I used the same approach in the AudioFileReader BTW, so any change needed in this patch would need to be done there as well. I guess it's okay as long as there is a way to disable the output. The issue with libraries that print to stderr and stdout is that they cannot be used easily for command-line tools that produce output.
Created attachment 180858 [details] AudioDestination: GstMessage handler
So I removed all the g_warning stuff. The gst warnings and errors appear in the GST_DEBUG output anyway.
Comment on attachment 180858 [details] AudioDestination: GstMessage handler View in context: https://bugs.webkit.org/attachment.cgi?id=180858&action=review Oops! I think I miscommunicated. g_warning and g_debug are probably okay since you can control them with the G_DEBUG environment variable. On the other hand I think that the GstBus that you get from gst_pipeline_get_bus are still leaking in this patch. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:68 > + GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > + gst_bus_add_signal_watch(bus.get()); > + g_signal_connect(bus.get(), "message", G_CALLBACK(messageCallback), this); If gst_pipeline_get_bus doesn't return a new reference you can use a raw pointer. If it does, you should use adoptRef. The documentation says "transfer full" so I think this needs to be adopted. > Source/WebCore/platform/audio/gstreamer/AudioDestinationGStreamer.cpp:91 > + GRefPtr<GstBus> bus = gst_pipeline_get_bus(GST_PIPELINE(m_pipeline)); > + g_signal_handlers_disconnect_by_func(bus.get(), reinterpret_cast<gpointer>(messageCallback), this); Ditto.
Created attachment 180893 [details] AudioDestination: GstMessage handler
Committed r138545: <http://trac.webkit.org/changeset/138545>
This patch is causing a lot of crashes on the WK2 EFL Debug build bot: crash log for WebProcess (pid <unknown>): STDOUT: <empty> STDERR: ASSERTION FAILED: !gstObjectIsFloating(GST_OBJECT(ptr)) STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp(130) : WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstBus] STDERR: 1 0x7fd7d388adde WTF::GRefPtr<_GstBus> WTF::adoptGRef<_GstBus>(_GstBus*) STDERR: 2 0x7fd7d38bc9b1 WebCore::AudioFileReader::decodeAudioForBusCreation() STDERR: 3 0x7fd7d38bbd63 WebCore::enteredMainLoopCallback(void*) STDERR: 4 0x7fd7ce0c7a1b STDERR: 5 0x7fd7ce0c6e53 g_main_context_dispatch STDERR: 6 0x7fd7ce0c71a0 STDERR: 7 0x7fd7ce0c759a g_main_loop_run STDERR: 8 0x7fd7d38bcd88 WebCore::AudioFileReader::createBus(float, bool) STDERR: 9 0x7fd7d38bcf5f WebCore::createBusFromAudioFile(char const*, bool, float) STDERR: 10 0x7fd7d38baad7 WebCore::AudioBus::loadPlatformResource(char const*, float) STDERR: 11 0x7fd7d2d92589 STDERR: 12 0x7fd7d2d92a6e WebCore::HRTFElevation::calculateKernelsForAzimuthElevation(int, int, float, WTF::String const&, WTF::RefPtr<WebCore::HRTFKernel>&, WTF::RefPtr<WebCore::HRTFKernel>&) STDERR: 13 0x7fd7d2d92f3c WebCore::HRTFElevation::createForSubject(WTF::String const&, int, float) STDERR: 14 0x7fd7d2d8fd54 WebCore::HRTFDatabase::HRTFDatabase(float) STDERR: 15 0x7fd7d2d8fc8e WebCore::HRTFDatabase::create(float) STDERR: 16 0x7fd7d2d91b31 WebCore::HRTFDatabaseLoader::load() STDERR: 17 0x7fd7d2d91ab0 STDERR: 18 0x7fd7d39682f5 STDERR: 19 0x7fd7d398fdea STDERR: 20 0x7fd7ccfffe9a STDERR: 21 0x7fd7d643dcbd clone
Seems like adoptGRef was indeed not needed then?
Created attachment 180954 [details] Test patch using manual unref'ing FYI, switching to raw pointers and using gst_object_unref (see attached patch) fixes the crashes for me (I don't see any warnings on stderr either). I'm not quite sure why this is different from using a smart pointer and adopting though.
(In reply to comment #12) > Created an attachment (id=180954) [details] > Test patch using manual unref'ing > > FYI, switching to raw pointers and using gst_object_unref (see attached patch) fixes the crashes for me (I don't see any warnings on stderr either). I'm not quite sure why this is different from using a smart pointer and adopting though. Because adoptGref() checks if the object has a floating ref or not. So yeah either we use raw pointers or revert the parts of the landed patch that added the adoptGRef calls. I'd be for using raw pointers, I never really liked this smart pointer stuff :P
(In reply to comment #13) > (In reply to comment #12) > > Created an attachment (id=180954) [details] [details] > > Test patch using manual unref'ing > > > > FYI, switching to raw pointers and using gst_object_unref (see attached patch) fixes the crashes for me (I don't see any warnings on stderr either). I'm not quite sure why this is different from using a smart pointer and adopting though. > > Because adoptGref() checks if the object has a floating ref or not. > So yeah either we use raw pointers or revert the parts of the landed patch that added the adoptGRef calls. > > I'd be for using raw pointers, I never really liked this smart pointer stuff :P Ok, I uploaded a patch at Bug 105869. I also prefer using raw pointers in this case.