Bug 151256

Summary: [GStreamer] Use RunLoop instead of GMainLoop in AudioFileReaderGStreamer
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, pnormand, zan
Priority: P2 Keywords: Gtk
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Updated patch zan: review+

Description Carlos Garcia Campos 2015-11-13 03:23:41 PST
We could use the platform abstraction instead of GMainLoop directly.
Comment 1 Carlos Garcia Campos 2015-11-13 03:30:37 PST
Created attachment 265476 [details]
Patch
Comment 2 WebKit Commit Bot 2015-11-13 03:33:14 PST
Attachment 265476 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:350:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2015-11-14 11:48:25 PST
Comment on attachment 265476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265476&action=review

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:380
> +    if (isMainThread()) {
> +        RefPtr<AudioBus> returnValue;
> +        auto threadID = createThread("AudioFileReader", [&returnValue, filePath, mixToMono, sampleRate] {
> +            returnValue = AudioFileReader(filePath).createBus(sampleRate, mixToMono);
> +        });
> +        waitForThreadCompletion(threadID);
> +
> +        return returnValue;
> +    }

Would RunLoop::run() not set up a nested main loop in case of running this in the main thread?

What about the cases where the bus is created in a non-main WebWorker thread that's also managed via RunLoop, wouldn't that set up the nested loop?
Comment 4 Carlos Garcia Campos 2015-11-15 23:35:25 PST
(In reply to comment #3)
> Comment on attachment 265476 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265476&action=review
> 
> > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:380
> > +    if (isMainThread()) {
> > +        RefPtr<AudioBus> returnValue;
> > +        auto threadID = createThread("AudioFileReader", [&returnValue, filePath, mixToMono, sampleRate] {
> > +            returnValue = AudioFileReader(filePath).createBus(sampleRate, mixToMono);
> > +        });
> > +        waitForThreadCompletion(threadID);
> > +
> > +        return returnValue;
> > +    }
> 
> Would RunLoop::run() not set up a nested main loop in case of running this
> in the main thread?

Yes, but with the same main context, we need a different one to avoid deadlocks. Phil wrote the original code, so he should know the details.

> What about the cases where the bus is created in a non-main WebWorker thread
> that's also managed via RunLoop, wouldn't that set up the nested loop?

In that case a new RunLoop is created for the current thread. If those threads already a have a RunLoop running maybe we should always use a dedicated thread, though.
Comment 5 Zan Dobersek 2015-11-15 23:45:05 PST
Comment on attachment 265476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265476&action=review

>>> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:380
>>> +    }
>> 
>> Would RunLoop::run() not set up a nested main loop in case of running this in the main thread?
>> 
>> What about the cases where the bus is created in a non-main WebWorker thread that's also managed via RunLoop, wouldn't that set up the nested loop?
> 
> Yes, but with the same main context, we need a different one to avoid deadlocks. Phil wrote the original code, so he should know the details.

My bad, workers don't use RunLoop, they use a custom queue.
Comment 6 Zan Dobersek 2015-11-15 23:48:18 PST
Comment on attachment 265476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265476&action=review

>>>> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:380
>>>> +    }
>>> 
>>> Would RunLoop::run() not set up a nested main loop in case of running this in the main thread?
>>> 
>>> What about the cases where the bus is created in a non-main WebWorker thread that's also managed via RunLoop, wouldn't that set up the nested loop?
>> 
>> Yes, but with the same main context, we need a different one to avoid deadlocks. Phil wrote the original code, so he should know the details.
> 
> My bad, workers don't use RunLoop, they use a custom queue.

On that note, the new code goes on to create a RunLoop object in the worker thread.

I'd rather avoid that, and just use a new thread regardless where the AudioBus creation is requested (the main thread or a worker thread).
Comment 7 Carlos Garcia Campos 2015-11-15 23:55:29 PST
(In reply to comment #6)
> Comment on attachment 265476 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265476&action=review
> 
> >>>> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:380
> >>>> +    }
> >>> 
> >>> Would RunLoop::run() not set up a nested main loop in case of running this in the main thread?
> >>> 
> >>> What about the cases where the bus is created in a non-main WebWorker thread that's also managed via RunLoop, wouldn't that set up the nested loop?
> >> 
> >> Yes, but with the same main context, we need a different one to avoid deadlocks. Phil wrote the original code, so he should know the details.
> > 
> > My bad, workers don't use RunLoop, they use a custom queue.
> 
> On that note, the new code goes on to create a RunLoop object in the worker
> thread.
> 
> I'd rather avoid that, and just use a new thread regardless where the
> AudioBus creation is requested (the main thread or a worker thread).

Ok, it's definitely safer and makes the code simpler.
Comment 8 Carlos Garcia Campos 2015-11-16 01:00:19 PST
Created attachment 265573 [details]
Updated patch

Use always a helper thread
Comment 9 WebKit Commit Bot 2015-11-16 01:03:04 PST
Attachment 265573 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:350:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Zan Dobersek 2015-11-16 01:13:19 PST
Comment on attachment 265573 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265573&action=review

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:296
> +    m_pipeline = gst_pipeline_new(nullptr);

Should the returned pointer be adopted?

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:312
> +        gst_message_unref(message);

This was leaking before, was it not?
Comment 11 Carlos Garcia Campos 2015-11-16 01:40:49 PST
(In reply to comment #10)
> Comment on attachment 265573 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265573&action=review
> 
> > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:296
> > +    m_pipeline = gst_pipeline_new(nullptr);
> 
> Should the returned pointer be adopted?

No, it's a floating ref we should consume.

> > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:312
> > +        gst_message_unref(message);
> 
> This was leaking before, was it not?

No, when using the message signal, gst unrefs the message, when using the sync handler, you must unref it when returning DROP.
Comment 12 Carlos Garcia Campos 2015-11-17 01:57:54 PST
Committed r192511: <http://trac.webkit.org/changeset/192511>