Bug 151256 – [GStreamer] Use RunLoop instead of GMainLoop in AudioFileReaderGStreamer

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

We could use the platform abstraction instead of GMainLoop directly.

[Carlos Garcia Campos]

Created attachment 265476 [details]
Patch

[WebKit Commit Bot]

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.

[Zan Dobersek]

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?

[Carlos Garcia Campos]

(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.

[Zan Dobersek]

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.

[Zan Dobersek]

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).

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 265573 [details]
Updated patch

Use always a helper thread

[WebKit Commit Bot]

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.

[Zan Dobersek]

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?

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r192511: <http://trac.webkit.org/changeset/192511>