Bug 201727 – ImageDecoders: use a thread safe data buffer for Cairo backing store

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Charlie Turner]

ImageDecoders: use a thread safe data buffer for Cairo backing store

[Charlie Turner]

Created attachment 378659 [details]
Patch

[Charlie Turner]

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.

[Carlos Alberto Lopez Perez]

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

[Miguel Gomez]

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.

[Carlos Garcia Campos]

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?

[Miguel Gomez]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Zan Dobersek]

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.

[Charlie Turner]

Created attachment 382052 [details]
Patch for landing

[Charlie Turner]

Created attachment 382053 [details]
Patch for landing

[Charlie Turner]

Created attachment 382054 [details]
Patch for landing

[WebKit Commit Bot]

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: [e-mail])
imported/w3c/web-platform-tests/css/css-position/position-absolute-crash-chrome-005.html bug 203474 (author: [e-mail])
imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/integrity.html bug 203394 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 382054 [details]
Patch for landing

Clearing flags on attachment: 382054

Committed r251651: <https://trac.webkit.org/changeset/251651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56665041>

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Bots are also exiting early after r251651 due to crashes, so I'll roll this out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 203488

[Fujii Hironori]

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

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 382173 [details]
Patch

[Charlie Turner]

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!

[Carlos Garcia Campos]

Committed r251771: <https://trac.webkit.org/changeset/251771>