Bug 189699 – [GStreamer] Utilities cleanups

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

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 350030 [details]
Patch

[Charlie Turner]

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.

[Charlie Turner]

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.

[Philippe Normand]

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!

[Philippe Normand]

Created attachment 350100 [details]
Patch

[Charlie Turner]

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.

[Charlie Turner]

(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 :-)

[Charlie Turner]

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

[Xabier Rodríguez Calvar]

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.

[Philippe Normand]

Created attachment 350115 [details]
Patch

[Charlie Turner]

Comment on attachment 350115 [details]
Patch

Informal r+ from me, thanks Philippe!

[Xabier Rodríguez Calvar]

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?

[Philippe Normand]

(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

[Philippe Normand]

Committed r236255: <https://trac.webkit.org/changeset/236255>

[Radar WebKit Bug Importer]

<rdar://problem/44634143>

[Michael Catanzaro]

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

[Michael Catanzaro]

Reverted r236255 for reason:

Many WebAudio crashes

Committed r236352: <https://trac.webkit.org/changeset/236352>

[Philippe Normand]

Created attachment 350417 [details]
Patch

[Philippe Normand]

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

[Philippe Normand]

*** Bug 189875 has been marked as a duplicate of this bug. ***

[Xabier Rodríguez Calvar]

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.

[Philippe Normand]

They apply on trunk. Original patch was rolled-out.

[Philippe Normand]

Committed r236396: <https://trac.webkit.org/changeset/236396>