[GStreamer][Soup] Let GStreamer provide the buffer data is downloaded to, to avoid copying
Created attachment 180368 [details] Patch
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.
Comment on attachment 180368 [details] Patch Attachment 180368 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15457097
Comment on attachment 180368 [details] Patch Attachment 180368 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15433005
Comment on attachment 180368 [details] Patch Attachment 180368 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15451122
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
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.
Created attachment 180502 [details] Patch
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!
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 :)
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.
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.
Comment on attachment 180502 [details] Patch I'll use Dan's idea.
(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.
Created attachment 180582 [details] Patch
Comment on attachment 180582 [details] Patch Attachment 180582 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15475170
Comment on attachment 180582 [details] Patch Attachment 180582 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15450628
(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?
Created attachment 180618 [details] Patch
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
(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.
(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.
(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? ;)
(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.
(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. :)
Any news on this one Gustavo?
o/ was out on vacations, back now, will post an updated patch soonish
Created attachment 181894 [details] Patch
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 :)
(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 =)
Created attachment 182554 [details] Patch
Committed r139877: <http://trac.webkit.org/changeset/139877>
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!
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.
Created attachment 183041 [details] Patch
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.
(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.
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
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.
+Ossy the buildbot lord :) Is there a qt debug bot? We need a detailed stack trace for a crash
I am trying to build QtWebKit, but am hitting build failures. I guess I'll try building qt5 instead of installing from the PPA
Created attachment 183688 [details] Patch
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
Created attachment 183823 [details] Patch
(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 =(
Comment on attachment 183823 [details] Patch Clearing flags on attachment: 183823 Committed r140420: <http://trac.webkit.org/changeset/140420>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 107557
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
Can we land this again Gustavo? I'm not sure of the status of the patch across EFL, Qt and GTK :)
I finished my proposed refactorings now, so I'll build and test this patch on GTK+, Qt and EFL and hopefully land it today ;)
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.
Committed r141821: <http://trac.webkit.org/changeset/141821>
Looks like this time it stuck ;)