Bug 105552 – [GStreamer][Soup] Let GStreamer provide the buffer data is downloaded to, to avoid copying

• Component: New Bugs
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

[GStreamer][Soup] Let GStreamer provide the buffer data is downloaded to, to avoid copying

[Gustavo Noronha (kov)]

Created attachment 180368 [details]
Patch

[Gustavo Noronha (kov)]

A bit more rationale on the background for the change: memcpy() was hot in some profiles we were running on a custom ARM platform, and was much higher in WebKit than with gst-launch, causing performance to degrade significantly. By letting the GStreamer media backend provide the buffers we eliminate the need for memcpy() inside our custom source element.

There is another patch also focused on eliminating as much copying as possible that requests buffers from the pipeline if possible, so that we can take advantage of hw accelerated decoders when available. I still need to rebase it and make it more webkitty, will post it on another bug later today or tomorrow.

[Early Warning System Bot]

Comment on attachment 180368 [details]
Patch

Attachment 180368 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15457097

[Early Warning System Bot]

Comment on attachment 180368 [details]
Patch

Attachment 180368 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15433005

[EFL EWS Bot]

Comment on attachment 180368 [details]
Patch

Attachment 180368 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15451122

[Philippe Normand]

Comment on attachment 180368 [details]
Patch

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

Pretty cool stuff Gustavo! I've just glanced at the patch but I'll try to do a full review tomorrow.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:892
> +#if GST_API_VERSION_1

#ifdef

[Philippe Normand]

Comment on attachment 180368 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:895
> +    GstMapInfo* mapInfo = g_slice_new(GstMapInfo);

I wonder, isn't the mapInfo leaked if the buffer can't be mapped?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:478
> +#ifdef GST_API_VERSION_1
> +        GstMiniObject* miniObject = reinterpret_cast<GstMiniObject*>(d->m_gstBuffer);
> +
> +        GstMapInfo* mapInfo = static_cast<GstMapInfo*>(gst_mini_object_steal_qdata(miniObject, g_quark_from_string("webkit-gst-map-info")));
> +        if (mapInfo)
> +            gst_buffer_unmap(d->m_gstBuffer, mapInfo);
> +#endif

Maybe this can be moved to a function in GStreamerVersioning? Could be used in the source element too I think.

[Gustavo Noronha (kov)]

Created attachment 180502 [details]
Patch

[Gustavo Noronha (kov)]

Thanks for the review!

(In reply to comment #6)
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:892
> > +#if GST_API_VERSION_1
> 
> #ifdef

Fixing this should fix qt and efl; wonder why my gst 0.10 build did not fail =/

(In reply to comment #7)
> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:895
> > +    GstMapInfo* mapInfo = g_slice_new(GstMapInfo);
> 
> I wonder, isn't the mapInfo leaked if the buffer can't be mapped?

It was being leaked, yes! Not only in this case, but even if it could be mapped heh.
 
> > Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:478
> > +#ifdef GST_API_VERSION_1
> > +        GstMiniObject* miniObject = reinterpret_cast<GstMiniObject*>(d->m_gstBuffer);
> > +
> > +        GstMapInfo* mapInfo = static_cast<GstMapInfo*>(gst_mini_object_steal_qdata(miniObject, g_quark_from_string("webkit-gst-map-info")));
> > +        if (mapInfo)
> > +            gst_buffer_unmap(d->m_gstBuffer, mapInfo);
> > +#endif
> 
> Maybe this can be moved to a function in GStreamerVersioning? Could be used in the source element too I think.

Done!

[Philippe Normand]

Comment on attachment 180368 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:135
> +char* getGstBufferDataPointer(GstBuffer* buffer)

I'm thinking of using this function in the WebAudio backend, but there the data pointer needs to be a float*. Maybe we could return a guint8* here and let the caller do the cast?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:890
> +GstBuffer* StreamingClient::getBufferForData(gsize requestedSize)

I wonder if most of this method could be moved to a function in GStreamerVersioning, something like allocateGstBuffer(gsize requestedSize). And I could reuse it in the webaudio src element :)

[Dan Winship]

You could reorganize this to keep all of the gst-specific stuff in WebKitWebSourceGStreamer.cpp. Just add a method to ResourceHandleClient:

    char* getBuffer(size_t length);

and then for each read, have ResourceHandleSoup call client->getBuffer() to get a buffer, read into it, and then client->didReceiveData() afterward. The default getBuffer() would be emulate the current behavior; something like:

    if (!m_buffer)
        m_buffer = static_cast<char*>(g_malloc(length));
    return m_buffer;

(except dealing with the possibility of length changing between calls too...)

and StreamingClient would override it to something like:

    m_gstBuffer = gst_buffer_new_and_alloc(length);
    return reinterpret_cast<char*>(GST_BUFFER_DATA(m_gstBuffer));

and its didReceiveData() would assert() that the passed-in data pointer was m_gstBuffer's data pointer, adjust m_gstBuffer's length if needed, and then hand the buffer off to the gstreamer pipeline.

[Gustavo Noronha (kov)]

That's a great idea, Dan! The only thing I don't yet know how to do with that approach is freeing the buffer if the handle is destroyed inbetween a read and a didReceiveData, but this also made me realize that was a problem with the patch as it is right now! I don't think we can rely on the client existing to tell us how to free the buffer.

So I'm thinking I can probably have the getBuffer method tell me whether the buffer is to be owned by the ResourceHandle, which implies it's been slice allocated and ResourceHandle should free it using its size. Otherwise the buffer is owned by the client, so we'd just ignore them/set to NULL.

[Gustavo Noronha (kov)]

Comment on attachment 180502 [details]
Patch

I'll use Dan's idea.

[Dan Winship]

(In reply to comment #12)
> That's a great idea, Dan! The only thing I don't yet know how to do with that approach is freeing the buffer if the handle is destroyed inbetween a read and a didReceiveData, but this also made me realize that was a problem with the patch as it is right now! I don't think we can rely on the client existing to tell us how to free the buffer.

I was assuming the client would deal with freeing the buffer itself, although I don't actually know the life cycle of ResourceHandleClients...

> So I'm thinking I can probably have the getBuffer method tell me whether the buffer is to be owned by the ResourceHandle, which implies it's been slice allocated and ResourceHandle should free it using its size.

oh, btw, the code shouldn't be using the slice allocator anyway; that's for small allocations. just use g_malloc/g_free.

[Gustavo Noronha (kov)]

Created attachment 180582 [details]
Patch

[Early Warning System Bot]

Comment on attachment 180582 [details]
Patch

Attachment 180582 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15475170

[Early Warning System Bot]

Comment on attachment 180582 [details]
Patch

Attachment 180582 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15450628

[Gustavo Noronha (kov)]

(In reply to comment #14)
> > That's a great idea, Dan! The only thing I don't yet know how to do with that approach is freeing the buffer if the handle is destroyed inbetween a read and a didReceiveData, but this also made me realize that was a problem with the patch as it is right now! I don't think we can rely on the client existing to tell us how to free the buffer.
> 
> I was assuming the client would deal with freeing the buffer itself, although I don't actually know the life cycle of ResourceHandleClients...

I guess we could add a default destructor to the client interface, but that sounds more intrusive than I'd like to be. The life cycle of the client is quite varied - the FrameLoader is one of the clients, for instance, and it may outlive the handle by quite a bit.
 
> > So I'm thinking I can probably have the getBuffer method tell me whether the buffer is to be owned by the ResourceHandle, which implies it's been slice allocated and ResourceHandle should free it using its size.
> 
> oh, btw, the code shouldn't be using the slice allocator anyway; that's for small allocations. just use g_malloc/g_free.

OK! Btw, one other thing we were experimenting with was bumping the default read size to read one or more MBs at once instead of the 8K we're doing now. Do you remember why we have 8K in the first place?

[Gustavo Noronha (kov)]

Created attachment 180618 [details]
Patch

[Philippe Normand]

Comment on attachment 180618 [details]
Patch

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

Looks pretty good to me but maybe Martin would like to give the final stamp. Thanks Dan for the informal review as well!

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:117
> +int getGstBufferSize(GstBuffer* buffer)

unsigned int or gsize to be coherent with setGstBufferSize?

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:139
> +    GstMapInfo* mapInfo = static_cast<GstMapInfo*>(gst_mini_object_get_qdata(miniObject, g_quark_from_string("webkit-gst-map-info")));

What about having the string declared static and use g_quark_from_static_string()? The doc suggests this would be more efficient.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:296
> +    GST_CALL_PARENT(G_OBJECT_CLASS, dispose, ((GObject* )(src)));

Is that a C-style cast? :P

[Dan Winship]

(In reply to comment #18)
> > I was assuming the client would deal with freeing the buffer itself, although I don't actually know the life cycle of ResourceHandleClients...
> 
> I guess we could add a default destructor to the client interface, but that sounds more intrusive than I'd like to be. The life cycle of the client is quite varied - the FrameLoader is one of the clients, for instance, and it may outlive the handle by quite a bit.

But if it only ever has a single ResourceHandle active on it at any given time, then this wouldn't be a problem; it would just end up reusing the same buffer for multiple consecutive requests, rather than reallocating a new one each time like we do now.

> OK! Btw, one other thing we were experimenting with was bumping the default read size to read one or more MBs at once instead of the 8K we're doing now. Do you remember why we have 8K in the first place?

The old SoupHTTPInputStream used 8K buffers internally, so using anything larger in WebKit would have been pointless (a read would always have returned 8k or less), and using anything smaller would have resulted in extra memcpy()s being needed sometimes. But with the new-and-improved I/O codepaths, you can use whatever buffer size you want and that will eventually get passed down to g_socket_receive().

As for why libsoup used 8K internally, there's no particular reason. Someone pulled that number out of a hat 10 years ago. It might not be optimal, although I'm guessing "one or more MBs" is way too big; unless your main loop is taking up a lot of time somewhere, it shouldn't be possible for that much network data to get buffered up between reads.

[Martin Robinson]

(In reply to comment #21)

> As for why libsoup used 8K internally, there's no particular reason. Someone pulled that number out of a hat 10 years ago. It might not be optimal, although I'm guessing "one or more MBs" is way too big; unless your main loop is taking up a lot of time somewhere, it shouldn't be possible for that much network data to get buffered up between reads.

Sometimes parsing and page layout can take one or two seconds.

[Gustavo Noronha (kov)]

(In reply to comment #20)
> > Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:117
> > +int getGstBufferSize(GstBuffer* buffer)
> 
> unsigned int or gsize to be coherent with setGstBufferSize?

I used int because it's the type used throughout ResourceHandle, I guess I could also change setGstBufferSize to use it, instead? What do you prefer?
 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:139
> > +    GstMapInfo* mapInfo = static_cast<GstMapInfo*>(gst_mini_object_get_qdata(miniObject, g_quark_from_string("webkit-gst-map-info")));
> 
> What about having the string declared static and use g_quark_from_static_string()? The doc suggests this would be more efficient.

Sure!

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:296
> > +    GST_CALL_PARENT(G_OBJECT_CLASS, dispose, ((GObject* )(src)));
> 
> Is that a C-style cast? :P

I copied it from finalize, I blame the existing code! Should I fix that one too, on a separate commit? ;)

[Gustavo Noronha (kov)]

(In reply to comment #21)
> But if it only ever has a single ResourceHandle active on it at any given time, then this wouldn't be a problem; it would just end up reusing the same buffer for multiple consecutive requests, rather than reallocating a new one each time like we do now.

I'll give a try and see what the code looks like when it's done, I guess it might not be as intrusive as I'm afraid it would.
 
> As for why libsoup used 8K internally, there's no particular reason. Someone pulled that number out of a hat 10 years ago. It might not be optimal, although I'm guessing "one or more MBs" is way too big; unless your main loop is taking up a lot of time somewhere, it shouldn't be possible for that much network data to get buffered up between reads.

So the case we were looking at was playing a 1080p movie stored locally. Apparently pushing lots of small chunks of data into the GST pipeline instead of fewer and bigger chunks hurts performance a bit. I will try to come up with some numbers.

[Philippe Normand]

(In reply to comment #23)
> (In reply to comment #20)
> > > Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:117
> > > +int getGstBufferSize(GstBuffer* buffer)
> > 
> > unsigned int or gsize to be coherent with setGstBufferSize?
> 
> I used int because it's the type used throughout ResourceHandle, I guess I could also change setGstBufferSize to use it, instead? What do you prefer?
> 

I don't have any preference as long as the API is coherent.

> 
> > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:296
> > > +    GST_CALL_PARENT(G_OBJECT_CLASS, dispose, ((GObject* )(src)));
> > 
> > Is that a C-style cast? :P
> 
> I copied it from finalize, I blame the existing code! Should I fix that one too, on a separate commit? ;)

Oops! Yes please fix it in this patch. :)

[Philippe Normand]

Any news on this one Gustavo?

[Gustavo Noronha (kov)]

o/ was out on vacations, back now, will post an updated patch soonish

[Gustavo Noronha (kov)]

Created attachment 181894 [details]
Patch

[Philippe Normand]

Comment on attachment 181894 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:158
> +    gst_mini_object_set_qdata(miniObject, g_quark_from_string("webkit-gst-map-info"), mapInfo, 0);

Would it be possible to use g_quark_from_static_string here as well? The webkitGstMapInfoQuarkString could be a global variable and reused here I think.

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:164
> +    GstMapInfo* mapInfo = static_cast<GstMapInfo*>(gst_mini_object_steal_qdata(miniObject, g_quark_from_string("webkit-gst-map-info")));

Ditto :)

[Gustavo Noronha (kov)]

(In reply to comment #29)
> Would it be possible to use g_quark_from_static_string here as well? The webkitGstMapInfoQuarkString could be a global variable and reused here I think.

Err, yep =)

[Gustavo Noronha (kov)]

Created attachment 182554 [details]
Patch

[Gustavo Noronha (kov)]

Committed r139877: <http://trac.webkit.org/changeset/139877>

[Gustavo Noronha (kov)]

I rolled out the patch because it broke these API tests:

Tests failed (1): /unittests/testdownload
Tests that timed out (1): /WebKit2APITests/TestDownloads

I'll investigate and post a new patch!

[János Badics]

139877 made two tests crash on Qt:
http/tests/security/mixedContent/insecure-audio-video-in-main-frame.html and
http/tests/security/video-poster-cross-origin-crash2.html .

Crash logs can be found at:
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r139877%20(46929)/http/tests/security/mixedContent/insecure-audio-video-in-main-frame-crash-log.txt
http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r139883%20(46933)/http/tests/security/video-poster-cross-origin-crash2-crash-log.txt

Thank you for taking a look.

[Gustavo Noronha (kov)]

Created attachment 183041 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 183041 [details]
Patch

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

Found the problem. I think we need to improve the robustness and readability of the code that deals with cancellation and with the client. I would like to propose two follow-up patches, let me know if you agree:

1. Abolish usage of client as a local variable

That means always doing d->client() or handle->client() as appropriate. The client *may* change after a ResourceHandleClient callback, so that would help avoid using a stale client.

2. Add a helper isCancelled() method to ResourceHandleSoup that will always check d->m_cancelled || handle->client()

We have been bitten by the client not existing before, and were just bitten again. I couldn't remember any reason why we would prefer to check only for the explicit cancellation. Maybe the method could be called something else to get across that it's not just checking for cancellation, though. Something like shouldBail?

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:701
> +    // didReceiveResponse may cause the client to be changed.
> +    client = d->client();

The WK2 test was timing out because the WebProcess crashed when calling getBuffer on the client (see bellow). The crash happened because the client used by this ResourceHandle had been changed, so client != d->client(), and the old one was presumably dead.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:703
> +    if (d->m_cancelled || !client) {

The WK1 test was crashing at the same place as the WK2 test, but for a different reason: the client was set to 0 in didReceiveResponse, so we were doing an use-after-free bellow. This worked before the patch because client was no longer used in this function and because readCallback does check for the client not being there.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:719
> +    d->m_buffer = client->getBuffer(READ_BUFFER_SIZE, &d->m_bufferSize);

Both crashes happened here.

[Martin Robinson]

(In reply to comment #36)

> Found the problem. I think we need to improve the robustness and readability of the code that deals with cancellation and with the client. I would like to propose two follow-up patches, let me know if you agree:

All your suggestions sound great. I'm all for improving the readability and robustness of the ResourceHandleSoup code, as it seems to drift toward the byzantine after each cleanup and simplification.

[WebKit Review Bot]

Comment on attachment 183041 [details]
Patch

Attachment 183041 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15914518

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Gustavo Noronha (kov)]

The chromium failure is a false positive I guess, since we're not touching any code it uses in theory. I realized, though, that my fixes are probably not enough to fix the qt crashes. Unfortunately the crash logs are pretty useless, I suppose there are no crash logs from a debug bot? I'll do a qt build if that's the case.

[Philippe Normand]

+Ossy the buildbot lord :)
Is there a qt debug bot? We need a detailed stack trace for a crash

[Gustavo Noronha (kov)]

I am trying to build QtWebKit, but am hitting build failures. I guess I'll try building qt5 instead of installing from the PPA

[Gustavo Noronha (kov)]

Created attachment 183688 [details]
Patch

[Philippe Normand]

Comment on attachment 183688 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:182
> +    gst_mini_object_set_qdata(miniObject, g_quark_from_static_string("webkit-gst-map-info"), mapInfo, 0);

Not using webkitGstMapInfoQuarkString here?

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:188
> +    GstMapInfo* mapInfo = static_cast<GstMapInfo*>(gst_mini_object_steal_qdata(miniObject, g_quark_from_static_string("webkit-gst-map-info")));

Ditto

[Gustavo Noronha (kov)]

Created attachment 183823 [details]
Patch

[Gustavo Noronha (kov)]

(In reply to comment #43)
> > Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:182
> > +    gst_mini_object_set_qdata(miniObject, g_quark_from_static_string("webkit-gst-map-info"), mapInfo, 0);
> 
> Not using webkitGstMapInfoQuarkString here?

Oops, I noobed this change really bad at least twice, sorry bout that =(

[Gustavo Noronha (kov)]

Comment on attachment 183823 [details]
Patch

Clearing flags on attachment: 183823

Committed r140420: <http://trac.webkit.org/changeset/140420>

[Gustavo Noronha (kov)]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 107557

[Gustavo Noronha (kov)]

Crashes in EFL. I think these may be caused by bad interaction between this patch and another one that was landed before that moved the allocation of the buffer around a bit, but I'm pretty sure the cause is the same as the ones we were seeing before in GTK+, so I think I'll go ahead and make patches for my proposed refactorings before landing this again. Also, we should probably get gtk+ building in debug mode again before this.

crash log for WebProcess (pid <unknown>):
STDOUT: <empty>
STDERR: 1   0x7fca56c360b7
STDERR: 2   0x7fca598484a0
STDERR: 3   0x7fca56b353ea getGstBufferDataPointer(_GstBuffer*)
STDERR: 4   0x7fca56b411b2 StreamingClient::didReceiveData(WebCore::ResourceHandle*, char const*, int, int)
STDERR: 5   0x7fca56b55b12
STDERR: 6   0x7fca50fbf775
STDERR: 7   0x7fca50fd48dd g_simple_async_result_complete
STDERR: 8   0x7fca50fd4a0c
STDERR: 9   0x7fca51306e53 g_main_context_dispatch
STDERR: 10  0x7fca529fcaee
STDERR: 11  0x7fca529f6cb9
STDERR: 12  0x7fca529f7845
STDERR: 13  0x7fca529f7b47 ecore_main_loop_begin
STDERR: 14  0x7fca56b1e5e9 WebCore::RunLoop::run()
STDERR: 15  0x7fca5a80b93a WebProcessMainEfl
STDERR: 16  0x400804 main
STDERR: 17  0x7fca5983376d __libc_start_main
STDERR: 18  0x400729
STDERR: LEAK: 1 WebPage
STDERR: LEAK: 1 WebFrame
STDERR: LEAK: 4 JSLazyEventListener
STDERR: LEAK: 95 RenderObject
STDERR: LEAK: 1 Page
STDERR: LEAK: 1 Frame
STDERR: LEAK: 4 CachedResource
STDERR: LEAK: 1 SubresourceLoader
STDERR: LEAK: 326 WebCoreNode

[Philippe Normand]

Can we land this again Gustavo? I'm not sure of the status of the patch across EFL, Qt and GTK :)

[Gustavo Noronha (kov)]

I finished my proposed refactorings now, so I'll build and test this patch on GTK+, Qt and EFL and hopefully land it today ;)

[Gustavo Noronha (kov)]

OK, updated the patch to get the buffer for every call to read_async - there were some added to replace skip and whatnot, and ran tests in gtk, gtk-wk2, qt, efl. I think I have all bases covered, let's try to land this again.

[Gustavo Noronha (kov)]

Committed r141821: <http://trac.webkit.org/changeset/141821>

[Gustavo Noronha (kov)]

Looks like this time it stuck ;)