Bug 189699 - [GStreamer] Utilities cleanups
Summary: [GStreamer] Utilities cleanups
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
: 189875 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-09-18 06:49 PDT by Philippe Normand
Modified: 2018-09-24 03:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (13.87 KB, patch)
2018-09-18 10:19 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (14.03 KB, patch)
2018-09-19 02:57 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (14.49 KB, patch)
2018-09-19 07:49 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2018-09-21 13:44 PDT, Philippe Normand
calvaris: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2018-09-18 06:49:09 PDT
Some unmapGstBuffer() call were left for years in the httpsrc element. createGstBufferForData() has only one call-site so is useless as an abstraction.
The mapGstBuffer functions are used only by the webaudiosrc element and could likely be replaced by the GstMappedBuffer class now.
Comment 1 Philippe Normand 2018-09-18 10:19:22 PDT
Created attachment 350030 [details]
Patch
Comment 2 Charlie Turner 2018-09-19 01:25:14 PDT
Comment on attachment 350030 [details]
Patch

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

Nice cleanup :-). Informal r+ from me with a couple of comments I leave to your taste.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:333
> +        mappedBuffers.reserveInitialCapacity(priv->sources.size());

Do you prefer this to Vector<RefPtr<GstMappedBuffer>> mappedBuffers(priv->sources.size())?

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:350
> +            mappedBuffers.uncheckedAppend(WTFMove(mappedBuffer));

Since they're being moved here, it seems we could instead have a move ctor rather than inheriting the RefCounted class. The intent for the mapped buffer class is more for local scopes rather than being passed around function boundaries.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:356
> +        priv->provider->render(nullptr, priv->bus, priv->framesToPull);

We have to be very careful not to drop this line out of its scope, there's more indirection now as to what memory this relies on (indirection from the setChannelMemory bit above). This code would be easier to read if the allocateBuffersAndRenderAudio step was in a separate method from the pushRenderedAudioBuffers step imho, I leave it to your taste however.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:935
> +        GstBuffer* subBuffer = gst_buffer_new_wrapped(g_memdup(data + currentOffset, currentOffsetSize), currentOffsetSize);

What's the reason for forcing copies here? I initially went with gst_buffer_copy_region because for memory blocks that aren't NO_SHARE, it can save reallocating and instead reuse the underlying memory objects.
Comment 3 Charlie Turner 2018-09-19 01:27:57 PDT
Comment on attachment 350030 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:935
>> +        GstBuffer* subBuffer = gst_buffer_new_wrapped(g_memdup(data + currentOffset, currentOffsetSize), currentOffsetSize);
> 
> What's the reason for forcing copies here? I initially went with gst_buffer_copy_region because for memory blocks that aren't NO_SHARE, it can save reallocating and instead reuse the underlying memory objects.

Sorry, I misread this as still using the old buffer! Ignore please.
Comment 4 Philippe Normand 2018-09-19 02:53:22 PDT
Comment on attachment 350030 [details]
Patch

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

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:333
>> +        mappedBuffers.reserveInitialCapacity(priv->sources.size());
> 
> Do you prefer this to Vector<RefPtr<GstMappedBuffer>> mappedBuffers(priv->sources.size())?

I'm not sure what's the difference, to be honest :) I've just reused the same pattern as `channelBufferList`

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:350
>> +            mappedBuffers.uncheckedAppend(WTFMove(mappedBuffer));
> 
> Since they're being moved here, it seems we could instead have a move ctor rather than inheriting the RefCounted class. The intent for the mapped buffer class is more for local scopes rather than being passed around function boundaries.

Ok, that makes sense!

>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:356
>> +        priv->provider->render(nullptr, priv->bus, priv->framesToPull);
> 
> We have to be very careful not to drop this line out of its scope, there's more indirection now as to what memory this relies on (indirection from the setChannelMemory bit above). This code would be easier to read if the allocateBuffersAndRenderAudio step was in a separate method from the pushRenderedAudioBuffers step imho, I leave it to your taste however.

Great idea!
Comment 5 Philippe Normand 2018-09-19 02:57:36 PDT
Created attachment 350100 [details]
Patch
Comment 6 Charlie Turner 2018-09-19 06:22:42 PDT
Comment on attachment 350100 [details]
Patch

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

A couple comments about the resource design.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:-85
> -    WTF_MAKE_NONCOPYABLE(GstMappedBuffer);

Because we're managing a resource in this class, either we disallow copying as was initially the case, or we start applying the rule-of-three, that being if the class has a destructor, it generally needs at least a copy and copy-assignment operator. Otherwise it's quite easy to get into situations where we "double-free" the mapped buffer. I would recommend (if possible) keeping the class as non-copyable but moveable, since that matches the use-case as I understand it here. If you need copy semantics, the destructor should at least be modified to set the m_isValid=false to avoid double unmappings, but there can be other problems relying on the default synthesized copy methods here.

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:82
> +    GstMappedBuffer(GstMappedBuffer&& other)

It is idiomatic to use initialization syntax here, : m_isValid(other.m_isValid), m_info(other.m_info) ... { other.m_isValid = true }, it avoids default constructing the members only to overwrite them.
Comment 7 Charlie Turner 2018-09-19 06:24:20 PDT
(In reply to Charlie Turner from comment #6)
> It is idiomatic to use initialization syntax here, :
> m_isValid(other.m_isValid), m_info(other.m_info) ... { other.m_isValid =
> true }, it avoids default constructing the members only to overwrite them.

I meant to say other.m_isValid = false :-)
Comment 8 Charlie Turner 2018-09-19 06:26:16 PDT
(In reply to Philippe Normand from comment #4)
> Comment on attachment 350030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350030&action=review
> 
> >> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:333
> >> +        mappedBuffers.reserveInitialCapacity(priv->sources.size());
> > 
> > Do you prefer this to Vector<RefPtr<GstMappedBuffer>> mappedBuffers(priv->sources.size())?
> 
> I'm not sure what's the difference, to be honest :) I've just reused the
> same pattern as `channelBufferList`

You save a separate method call to reserveInitialCapacity after the initial constructor, but this is a minor point. They'll basically be no-ops anyway if priv->sources.size() < ~16 with the SVO. Not sure what is idiomatic in this area of WebCore tbh.
Comment 9 Xabier Rodríguez Calvar 2018-09-19 07:21:27 PDT
Comment on attachment 350100 [details]
Patch

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

Other than the nit and what Charlie said already, no additional comments from my side.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:346
> +        GstMappedBuffer mappedBuffer(buffer.get(), GST_MAP_READWRITE);

A nit for the future: we could overload the GstMapperBuffer ctor to take a RefPtr<GstBuffer>& and move the get inside that. I think that API could be a bit more friendly.
Comment 10 Philippe Normand 2018-09-19 07:49:10 PDT
Created attachment 350115 [details]
Patch
Comment 11 Charlie Turner 2018-09-19 12:11:35 PDT
Comment on attachment 350115 [details]
Patch

Informal r+ from me, thanks Philippe!
Comment 12 Xabier Rodríguez Calvar 2018-09-20 01:15:26 PDT
Comment on attachment 350115 [details]
Patch

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

Thanks Phil for the patch! And thanks Charlie for the informal reviews!

> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:83
> +    GstMappedBuffer() = default;

Do we need this constructor? Does it make sense? I cannot think of a useful case of this. Maybe = delete?
Comment 13 Philippe Normand 2018-09-20 01:43:02 PDT
(In reply to Xabier Rodríguez Calvar from comment #12)
> Comment on attachment 350115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=350115&action=review
> 
> Thanks Phil for the patch! And thanks Charlie for the informal reviews!
> 

Thanks for the reviews Calvaris & Charlie :)

> > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:83
> > +    GstMappedBuffer() = default;
> 
> Do we need this constructor? Does it make sense? I cannot think of a useful
> case of this. Maybe = delete?

Yes and yes. This change is the consequence of not using mappedBuffers.reserveInitialCapacity() anymore. Without it:

In file included from ../../Source/WebCore/platform/audio/AudioBus.h:36,
                 from ../../Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:26:
DerivedSources/ForwardingHeaders/wtf/Vector.h: In instantiation of ‘static void WTF::VectorInitializer<true, false, T>::initializeIfNonPOD(T*, T*) [with T = WebCore::GstMappedBuffer]’:
DerivedSources/ForwardingHeaders/wtf/Vector.h:245:129:   required from ‘static void WTF::VectorTypeOperations<T>::initializeIfNonPOD(T*, T*) [with T = WebCore::GstMappedBuffer]’
DerivedSources/ForwardingHeaders/wtf/Vector.h:631:47:   required from ‘WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous> >::Vector(size_t) [with T = WebCore::GstMappedBuffer; long unsigned int inlineCapacity = 0; OverflowHandler = WTF::CrashOnOverflow; long unsigned int minCapacity = 16; size_t = long unsigned int]’
../../Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:329:63:   required from here
DerivedSources/ForwardingHeaders/wtf/Vector.h:85:13: error: no matching function for call to ‘WebCore::GstMappedBuffer::GstMappedBuffer()’
             new (NotNull, cur) T();
             ^~~~~~~~~~~~~~~~~~~~~~
In file included from ../../Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:28:
../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:99:5: note: candidate: ‘WebCore::GstMappedBuffer::GstMappedBuffer(GstBuffer*, int)’
     GstMappedBuffer(GstBuffer* buffer, int flags)
     ^~~~~~~~~~~~~~~
../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:99:5: note:   candidate expects 2 arguments, 0 provided
../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:91:5: note: candidate: ‘WebCore::GstMappedBuffer::GstMappedBuffer(GstBuffer*, GstMapFlags)’
     GstMappedBuffer(GstBuffer* buffer, GstMapFlags flags)
     ^~~~~~~~~~~~~~~
../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:91:5: note:   candidate expects 2 arguments, 0 provided
../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:83:5: note: candidate: ‘WebCore::GstMappedBuffer::GstMappedBuffer(WebCore::GstMappedBuffer&&)’
     GstMappedBuffer(GstMappedBuffer&& other)
     ^~~~~~~~~~~~~~~
../../Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:83:5: note:   candidate expects 1 argument, 0 provided
Comment 14 Philippe Normand 2018-09-20 02:18:19 PDT
Committed r236255: <https://trac.webkit.org/changeset/236255>
Comment 15 Radar WebKit Bug Importer 2018-09-20 02:19:25 PDT
<rdar://problem/44634143>
Comment 16 Michael Catanzaro 2018-09-21 13:06:15 PDT
This introduced ~35 webaudio test crashes:

Results: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r236255%20(8237)/results.html

Example backtrace:

Thread 1 (Thread 0x7fbe579ff700 (LWP 41612)):
#0  __memset_avx2_unaligned_erms () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:163
#1  0x00007fbed0c4653b in _ZN7WebCore8AudioBus16speakersCopyFromERKS0_ () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#2  0x00007fbed040bb95 in _ZN7WebCore20AudioDestinationNode6renderEPNS_8AudioBusES2_m () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#3  0x00007fbed12a36e7 in _ZL21webKitWebAudioSrcLoopP18_WebKitWebAudioSrc () from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00007fbec9f43361 in gst_task_func () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/gstreamer-1.14.3/gst/gsttask.c:332
#5  0x00007fbec8e885ae in g_thread_pool_thread_proxy () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gthreadpool.c:307
#6  0x00007fbec8e87bd5 in g_thread_proxy () at /home/slave/webkitgtk/gtk-linux-64-release-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gthread.c:784
#7  0x00007fbecab43494 in start_thread (arg=0x7fbe579ff700) at pthread_create.c:333
#8  0x00007fbec6f6793f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97
Comment 17 Michael Catanzaro 2018-09-21 13:07:07 PDT
Reverted r236255 for reason:

Many WebAudio crashes

Committed r236352: <https://trac.webkit.org/changeset/236352>
Comment 18 Philippe Normand 2018-09-21 13:44:32 PDT
Created attachment 350417 [details]
Patch
Comment 19 Philippe Normand 2018-09-21 13:45:18 PDT
Comment on attachment 350417 [details]
Patch

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

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:331
> +    mappedBuffers.reserveInitialCapacity(priv->sources.size());

Turns out it was a bad idea to not call this....
Comment 20 Philippe Normand 2018-09-22 01:43:13 PDT
*** Bug 189875 has been marked as a duplicate of this bug. ***
Comment 21 Xabier Rodríguez Calvar 2018-09-23 23:00:15 PDT
Comment on attachment 350417 [details]
Patch

I agree on the changes but I guess they have to apply on what is on trunk, right? Cause the diff looks like the old one.
Comment 22 Philippe Normand 2018-09-24 00:58:33 PDT
They apply on trunk. Original patch was rolled-out.
Comment 23 Philippe Normand 2018-09-24 03:21:01 PDT
Committed r236396: <https://trac.webkit.org/changeset/236396>