Bug 201727

Summary: ImageDecoders: use a thread safe data buffer for Cairo backing store
Product: WebKit Reporter: Charlie Turner <cturner>
Component: New BugsAssignee: Charlie Turner <cturner>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cgarcia, clopez, commit-queue, Hironori.Fujii, magomez, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=202807
Bug Depends on: 203488    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing
none
Patch Hironori.Fujii: review+

Description Charlie Turner 2019-09-12 11:01:30 PDT
ImageDecoders: use a thread safe data buffer for Cairo backing store
Comment 1 Charlie Turner 2019-09-12 11:10:17 PDT
Created attachment 378659 [details]
Patch
Comment 2 Charlie Turner 2019-09-16 09:52:05 PDT
This is sometimes crashing in release builds only, i'm starting to suspect the compiler, but in the meantime it's not ready for landing it seems.
Comment 3 Carlos Alberto Lopez Perez 2019-10-23 11:09:27 PDT
This has been here ignored because there isn't any reviewer CC'ed. Next time try to CC any reviewer. You can pass the flag "--suggest-reviewers" to the webkit-patch tool and it will automatically suggest them
Comment 4 Miguel Gomez 2019-10-23 23:51:38 PDT
This totally makes sense. The introduction of async image decoding is what caused buffers to be accessed from different threads (before that everything happened on the main thread). The patch looks good IMO.
Comment 5 Carlos Garcia Campos 2019-10-24 00:10:06 PDT
Comment on attachment 378659 [details]
Patch

Do we only need the refcounting to be thread safe? or also the data can be accessed from multiple threads? because in that case we would need a lock to protect the pixels, no?
Comment 6 Miguel Gomez 2019-10-24 00:41:54 PDT
(In reply to Carlos Garcia Campos from comment #5)
> Comment on attachment 378659 [details]
> Patch
> 
> Do we only need the refcounting to be thread safe? or also the data can be
> accessed from multiple threads? because in that case we would need a lock to
> protect the pixels, no?

The buffer is going to be created on the decoding thread and filled there. Once the decoding is finished a repaint is requested and at that point it's when the main thread will start accessing the buffer contents, and the decoding thread shouldn't be accessing it anymore. So I don't think the data can be accessed from both threads at the same time.
Comment 7 Carlos Garcia Campos 2019-10-24 00:53:46 PDT
(In reply to Miguel Gomez from comment #6)
> (In reply to Carlos Garcia Campos from comment #5)
> > Comment on attachment 378659 [details]
> > Patch
> > 
> > Do we only need the refcounting to be thread safe? or also the data can be
> > accessed from multiple threads? because in that case we would need a lock to
> > protect the pixels, no?
> 
> The buffer is going to be created on the decoding thread and filled there.
> Once the decoding is finished a repaint is requested and at that point it's
> when the main thread will start accessing the buffer contents, and the
> decoding thread shouldn't be accessing it anymore. So I don't think the data
> can be accessed from both threads at the same time.

Ok, it doesn't matter much because it's a private helper class, but I think I would rename it as RGBAPixelBufferThreadSafeRefCounted to make it clearer.
Comment 8 Carlos Garcia Campos 2019-10-24 00:54:59 PDT
Comment on attachment 378659 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBackingStore.h:188
> +    class ThreadSafeRGBAPixelBuffer : public ThreadSafeRefCounted<ThreadSafeRGBAPixelBuffer> {

As I suggested in the comments I would rename this, because this name is confusing.
Comment 9 Zan Dobersek 2019-10-24 23:29:30 PDT
Comment on attachment 378659 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageBackingStore.h:199
> +            m_isValid = m_pixels.tryReserveCapacity(initialSize.area().unsafeGet());

This operation on Vector, as name suggests, only reserve-expands the capacity -- it doesn't change the size of it. But the zero-fill in zeroPixelData() does fill the Vector's data based on the size, i.e. it zeroes out nothing.

An additional Vector<>::resize() call here (if reservation was successful) would avoid that.
Comment 10 Charlie Turner 2019-10-28 02:16:01 PDT
Created attachment 382052 [details]
Patch for landing
Comment 11 Charlie Turner 2019-10-28 02:16:43 PDT
Created attachment 382053 [details]
Patch for landing
Comment 12 Charlie Turner 2019-10-28 02:17:35 PDT
Created attachment 382054 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2019-10-28 03:11:58 PDT
The commit-queue encountered the following flaky tests while processing attachment 382054 [details]:

imported/w3c/web-platform-tests/css/css-position/position-absolute-container-dynamic-002.html bug 203473 (author: simon.fraser@apple.com)
imported/w3c/web-platform-tests/css/css-position/position-absolute-crash-chrome-005.html bug 203474 (author: simon.fraser@apple.com)
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/integrity.html bug 203394 (author: ysuzuki@apple.com)
The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2019-10-28 03:12:50 PDT
Comment on attachment 382054 [details]
Patch for landing

Clearing flags on attachment: 382054

Committed r251651: <https://trac.webkit.org/changeset/251651>
Comment 15 WebKit Commit Bot 2019-10-28 03:12:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-10-28 03:13:17 PDT
<rdar://problem/56665041>
Comment 17 Carlos Garcia Campos 2019-10-28 07:58:01 PDT
I had to revert this locally because it's causing crashes here with my riot ephjy web app (I don't know why it doesn't happen with normal ephy). It's crashing when  creating the favicon, see:

Thread 1 (Thread 0x7fffeaf03cc0 (LWP 2247)):
#0  0x00007ffff53b91f0 in WebCore::ImageBackingStore::image() const () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#1  0x00007ffff53a87b1 in WebCore::ScalableImageDecoder::createFrameImageAtIndex(unsigned long, WebCore::SubsamplingLevel, WebCore::DecodingOptions const&) () from /home/cgarcia/gnome/lib/li--Type <RET> for more, q to quit, c to continue without paging--
bwebkit2gtk-4.0.so.37
#2  0x00007ffff4ce65f3 in WebCore::ImageSource::frameAtIndexCacheIfNeeded(unsigned long, WebCore::ImageFrame::Caching, WTF::Optional<WebCore::SubsamplingLevel> const&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#3  0x00007ffff4ce6830 in WebCore::ImageSource::frameImageAtIndexCacheIfNeeded(unsigned long, WebCore::SubsamplingLevel) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#4  0x00007ffff4c87297 in WebCore::BitmapImage::frameImageAtIndexCacheIfNeeded(unsigned long, WebCore::SubsamplingLevel, WebCore::GraphicsContext const*) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#5  0x00007ffff4c872f3 in WebCore::BitmapImage::nativeImageForCurrentFrame(WebCore::GraphicsContext const*) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#6  0x00007ffff3827d2e in WTF::Detail::CallableWrapper<WebKit::IconDatabase::loadIconForPageURL(WTF::String const&, WebKit::IconDatabase::AllowDatabaseWrite, WTF::CompletionHandler<void (WTF::RefPtr<_cairo_surface, WTF::DumbPtrTraits<_cairo_surface> >&&)>&&)::{lambda()#1}::operator()()::{lambda()#1}, void>::call() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37
#7  0x00007ffff1697f85 in WTF::RunLoop::performWork() () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18
#8  0x00007ffff16f4a29 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18
#9  0x00007ffff74a6a1e in g_main_dispatch (context=0x55555567f120) at ../glib/gmain.c:3179
#10 g_main_context_dispatch (context=context@entry=0x55555567f120) at ../glib/gmain.c:3844
#11 0x00007ffff74a6da0 in g_main_context_iterate (context=context@entry=0x55555567f120, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:3917
#12 0x00007ffff74a6e2f in g_main_context_iteration (context=context@entry=0x55555567f120, may_block=may_block@entry=1) at ../glib/gmain.c:3978
#13 0x00007ffff76a9a4d in g_application_run (application=0x55555595c2e0, argc=<optimized out>, argv=<optimized out>) at ../gio/gapplication.c:2559
#14 0x0000555555559bf9 in main (argc=1, argv=0x7fffffffd928) at ../src/ephy-main.c:427
Comment 18 Carlos Garcia Campos 2019-10-28 08:01:36 PDT
Bots are also exiting early after r251651 due to crashes, so I'll roll this out.
Comment 19 WebKit Commit Bot 2019-10-28 08:02:31 PDT
Re-opened since this is blocked by bug 203488
Comment 20 Fujii Hironori 2019-10-28 19:27:17 PDT
*** Bug 203546 has been marked as a duplicate of this bug. ***
Comment 21 Carlos Garcia Campos 2019-10-29 03:36:12 PDT
Comment on attachment 382054 [details]
Patch for landing

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

This patch can be simplified simply using SharedBuffer::DataSegment which is already thread safe ref counted.

> Source/WebCore/platform/graphics/ImageBackingStore.h:-205
> -        m_pixels = SharedBuffer::create(other.m_pixels->data(), other.m_pixels->size());
> -        m_pixelsPtr = reinterpret_cast<RGBA32*>(const_cast<char*>(m_pixels->data()));

I think this is the problem, we actually need to copy the data here.
Comment 22 Carlos Garcia Campos 2019-10-29 03:38:02 PDT
Created attachment 382173 [details]
Patch
Comment 23 Charlie Turner 2019-10-29 08:11:52 PDT
Comment on attachment 382173 [details]
Patch

It LGTM, I come to same conclusion about the copy ctor and was about to upload :-), but the DataSegment is much nicer, thanks for taking a look!
Comment 24 Carlos Garcia Campos 2019-10-30 00:59:10 PDT
Committed r251771: <https://trac.webkit.org/changeset/251771>