[Wayland][WebRender] WebRender/EGL does not work with SharedSurfaceType::Basic

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
9 months ago
3 months ago

People

(Reporter: stransky, Assigned: sotaro)

Tracking

(Blocks 2 bugs)

Trunk
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Details

(URL)

Attachments

(2 attachments, 8 obsolete attachments)

987 bytes, patch
Details | Diff | Splinter Review
1.06 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 months ago
When WebRender is enabled, Wayland native build fails (crashes) at  https://ncase.me/trust/ 

Also filed at https://github.com/servo/webrender/issues/2953

It's at SurfaceFormatToImageFormat(). It's called from TextureHost.cpp and gfx::SurfaceFormat::R8G8B8X8 which is unsupported by WebRender.
(Reporter)

Updated

9 months ago
No longer depends on: 1480414
(Reporter)

Updated

9 months ago
Summary: [Wayland] Support WebRender → [Wayland][WebRender] Crash at https://ncase.me/trust/
(Reporter)

Comment 1

9 months ago
There's a relevant backtrace part:

#7  0x00007fbcef506352 in mozilla::wr::SurfaceFormatToImageFormat(mozilla::gfx::SurfaceFormat) (aFormat=mozilla::gfx::SurfaceFormat::R8G8B8A8) at /home/komat/tmp676-trunk-gtk3/src-wayland/objdir/dist/include/mozilla/webrender/WebRenderTypes.h:67
#8  0x00007fbcef5063ee in mozilla::wr::ImageDescriptor::ImageDescriptor(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, unsigned int, mozilla::gfx::SurfaceFormat) (this=0x7fbcdb653bd0, aSize=..., aByteStride=5064, aFormat=mozilla::gfx::SurfaceFormat::R8G8B8A8)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/objdir/dist/include/mozilla/webrender/WebRenderTypes.h:118
#9  0x00007fbcef67788b in mozilla::layers::BufferTextureHost::PushResourceUpdates(mozilla::wr::TransactionBuilder&, mozilla::layers::TextureHost::ResourceUpdateOp, mozilla::Range<mozilla::wr::ImageKey> const&, mozilla::wr::WrExternalImageId const&) (this=0x7fbcb72774d0, aResources=..., aOp=mozilla::layers::TextureHost::ADD_IMAGE, aImageKeys=..., aExtID=...)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/composite/TextureHost.cpp:652
#10 0x00007fbcef490ba5 in mozilla::layers::WebRenderTextureHost::PushResourceUpdates(mozilla::wr::TransactionBuilder&, mozilla::layers::TextureHost::ResourceUpdateOp, mozilla::Range<mozilla::wr::ImageKey> const&, mozilla::wr::WrExternalImageId const&) (this=0x7fbcaad7f120, aResources=..., aOp=mozilla::layers::TextureHost::ADD_IMAGE, aImageKeys=..., aExtID=...)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/wr/WebRenderTextureHost.cpp:154
#11 0x00007fbcef508d0b in mozilla::layers::AsyncImagePipelineManager::UpdateImageKeys(mozilla::wr::Epoch const&, mozilla::wr::PipelineId const&, mozilla::layers::AsyncImagePipelineManager::AsyncImagePipeline*, nsTArray<mozilla::wr::ImageKey>&, mozilla::wr::TransactionBuilder&, mozilla::wr::TransactionBuilder&) (this=0x7fbcd2b4b820, aEpoch=..., aPipelineId=..., aPipeline=0x7fbcab295820, aKeys=..., aSceneBuilderTxn=..., aMaybeFastTxn=...)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/wr/AsyncImagePipelineManager.cpp:279
#12 0x00007fbcef50923f in mozilla::layers::AsyncImagePipelineManager::ApplyAsyncImageForPipeline(mozilla::wr::Epoch const&, mozilla::wr::PipelineId const&, mozilla::layers::AsyncImagePipelineManager::AsyncImagePipeline*, mozilla::wr::TransactionBuilder&, mozilla::wr::TransactionBuilder&) (this=0x7fbcd2b4b820, aEpoch=..., aPipelineId=..., aPipeline=0x7fbcab295820, aSceneBuilderTxn=..., aMaybeFastTxn=...) at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/wr/AsyncImagePipelineManager.cpp:357
#13 0x00007fbcef509b63 in mozilla::layers::AsyncImagePipelineManager::ApplyAsyncImageForPipeline(mozilla::wr::PipelineId const&, mozilla::wr::TransactionBuilder&) (this=0x7fbcd2b4b820, aPipelineId=..., aSceneBuilderTxn=...)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/wr/AsyncImagePipelineManager.cpp:454
#14 0x00007fbcef5171e3 in mozilla::layers::WebRenderBridgeParent::ProcessWebRenderParentCommands(nsTArray<mozilla::layers::WebRenderParentCommand> const&, mozilla::wr::TransactionBuilder&) (this=0x7fbcd6054400, aCommands=..., aTxn=...)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/wr/WebRenderBridgeParent.cpp:979
(Reporter)

Comment 2

9 months ago
I tracked it down to GetActualReadFormats() at gfx/gl/GLReadTexImageHelper.cpp.

gl->IsGLES() = true on Wayland, GL_IMPLEMENTATION_COLOR_READ_FORMAT != destFormat so fallback values are choosen:

*out_readFormat = LOCAL_GL_RGBA;                                                                         *out_readType = LOCAL_GL_UNSIGNED_BYTE;                                                                  

Should we select LOCAL_GL_BGRA instead as it works with WebRender?
(Reporter)

Comment 3

9 months ago
To be clear there's the GetActualReadFormats() callback:

#0  0x00007fc08d84242b in mozilla::gl::GetActualReadFormats(mozilla::gl::GLContext*, unsigned int, unsigned int, unsigned int*, unsigned int*) (gl=0x7fc04dffd000, destFormat=32993, destType=5121, out_readFormat=0x7ffda11081e4, out_readType=0x7ffda11081e0) at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/gl/GLReadTexImageHelper.cpp:209
#1  0x00007fc08da0a33f in mozilla::layers::TexClientFromReadback(mozilla::gl::SharedSurface*, mozilla::layers::CompositableForwarder*, mozilla::layers::TextureFlags, mozilla::layers::LayersBackend) (src=0x7fc0426a9eb0, allocator=0x7fc07246c608, baseFlags=514, layersBackend=mozilla::layers::LayersBackend::LAYERS_WR)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/client/CanvasClient.cpp:289
#2  0x00007fc08da0af1d in mozilla::layers::CanvasClientSharedSurface::UpdateRenderer(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::MaybeOneOf<mozilla::layers::ShareableCanvasRenderer*, mozilla::layers::AsyncCanvasRenderer*>&) (this=0x7fc04e355160, aSize=..., aRenderer=...) at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/client/CanvasClient.cpp:457
#3  0x00007fc08da0a95c in mozilla::layers::CanvasClientSharedSurface::Update(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits>, mozilla::layers::ShareableCanvasRenderer*) (this=0x7fc04e355160, aSize=..., aCanvasRenderer=0x7fc04e2a9740)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/client/CanvasClient.cpp:380
#4  0x00007fc08d8cb88d in mozilla::layers::ShareableCanvasRenderer::UpdateCompositableClient() (this=0x7fc04e2a9740)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/ShareableCanvasRenderer.cpp:234
#5  0x00007fc090479845 in nsDisplayCanvas::CreateWebRenderCommands(mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&, mozilla::layers::StackingContextHelper const&, mozilla::layers::WebRenderLayerManager*, nsDisplayListBuilder*) (this=0x7fc0426af1e0, aBuilder=..., aResources=..., aSc=..., aManager=0x7fc072cc1800, aDisplayListBuilder=0x7ffda110a2e0)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/layout/generic/nsHTMLCanvasFrame.cpp:151
#6  0x00007fc08d921717 in mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList(nsDisplayList*, nsDisplayItem*, nsDisplayListBuilder*, mozilla::layers::StackingContextHelper const&, mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&) (this=0x7fc072cc1a58, aDisplayList=0x7fc0426a3dd0, aWrappingItem=0x7fc0426a3d20, aDisplayListBuilder=0x7ffda110a2e0, aSc=..., aBuilder=..., aResources=...)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/wr/WebRenderCommandBuilder.cpp:1381
#7  0x00007fc0906c490b in nsDisplayWrapList::CreateWebRenderCommands(mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&, mozilla::layers::StackingContextHelper const&, mozilla::layers::WebRenderLayerManager*, nsDisplayListBuilder*) (this=0x7fc0426a3d20, aBuilder=..., aResources=..., aSc=..., aManager=0x7fc072cc1800, aDisplayListBuilder=0x7ffda110a2e0)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/layout/painting/nsDisplayList.cpp:5986
#8  0x00007fc0906c6edc in nsDisplayOwnLayer::CreateWebRenderCommands(mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&, mozilla::layers::StackingContextHelper const&, mozilla::layers::WebRenderLayerManager*, nsDisplayListBuilder*) (this=0x7fc0426a3d20, aBuilder=..., aResources=..., aSc=..., aManager=0x7fc072cc1800, aDisplayListBuilder=0x7ffda110a2e0)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/layout/painting/nsDisplayList.cpp:6674
#9  0x00007fc08d921717 in mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList(nsDisplayList*, nsDisplayItem*, nsDisplayListBuilder*, mozilla::layers::StackingContextHelper const&, mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&) (this=0x7fc072cc1a58, aDisplayList=0x7ffda110a2c0, aWrappingItem=0x0, aDisplayListBuilder=0x7ffda110a2e0, aSc=..., aBuilder=..., aResources=...)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/wr/WebRenderCommandBuilder.cpp:1381
#10 0x00007fc08d920e8d in mozilla::layers::WebRenderCommandBuilder::BuildWebRenderCommands(mozilla::wr::DisplayListBuilder&, mozilla::wr::IpcResourceUpdateQueue&, nsDisplayList*, nsDisplayListBuilder*, mozilla::layers::WebRenderScrollData&, mozilla::wr::TypedSize2D<float, mozilla::wr::LayoutPixel>&, nsTArray<mozilla::wr::WrFilterOp> const&) (this=0x7fc072cc1a58, aBuilder=..., aResourceUpdates=..., aDisplayList=0x7ffda110a2c0, aDisplayListBuilder=0x7ffda110a2e0, aScrollData=..., aContentSize=..., aFilters=const nsTArray<mozilla::wr::WrFilterOp> &)
    at /home/komat/tmp676-trunk-gtk3/src-wayland/gfx/layers/wr/WebRenderCommandBuilder.cpp:1241

Comment 4

9 months ago
Do I get this right and it's a bug in webrender using GLES? Because while it's great to see GLES fixed, an alternative would be to fix bug 1474281
(Reporter)

Comment 5

9 months ago
Also the ELG surface is created as SharedSurfaceType::Basic so it needs readback (at CanvasClientSharedSurface::UpdateRenderer()) so that may be another cause for this.
(Reporter)

Updated

9 months ago
Assignee: nobody → stransky

Comment 6

8 months ago
I can also reproduce this on https://threejs.org/examples/#webgl_geometries (which is expected, since ncase.me uses the threejs library).

There's more problems with that example, maybe that's useful to know: bug 1310741 (which is actually not resolved)
(Reporter)

Comment 7

8 months ago
Jeff, can you please look at it?
Attachment #9000185 - Flags: review?(jgilbert)
(Reporter)

Comment 8

8 months ago
Comment on attachment 9000185 [details] [diff] [review]
surface format patch

That's seems to break other parts of GL world.
Attachment #9000185 - Flags: review?(jgilbert)
(Reporter)

Updated

8 months ago
Summary: [Wayland][WebRender] Crash at https://ncase.me/trust/ → [Wayland][WebRender] WebRender/EGL does not work with SharedSurfaceType::Basic
(Reporter)

Comment 9

8 months ago
Looks like we need to implement a correct SurfaceFactory at GLScreenBuffer::CreateFactory() for Wayland/EGL to avoid use the Basic with readback:

https://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLScreenBuffer.cpp#66
(Reporter)

Comment 10

8 months ago
The crash reproducer for me is https://ncase.me/trust/
(Reporter)

Comment 11

8 months ago
WebRenderer backend supports only GL_BGRA texture format. When using SharedSurfaceType::Basic with
readback we need to provide this surface format instead of the default GL_RGBA.
(Reporter)

Comment 13

8 months ago
> jgilbert wrote:
> You'll either need to add support for RGBA to WR (which *should* be easy, and *should* be required,
> since BGRA doesn't have universal support in GLES in particular), or for the time being,
> flip R and B when crossing into WR.

Nicolas, do you think it's suitable to add GL_RGBA support to WR?
Flags: needinfo?(nical.bugzilla)
> Nicolas, do you think it's suitable to add GL_RGBA support to WR?

Should be, I guess. This review comment suggests we don't really have a choice:

> BGRA does not have universal support, so we can't do this.

Jeff, do you know which platforms/configurations don't support BGRA exactly? I made a few online search but I couldn't find any precise information. It seems to be GL ES related mostly.

In the mean time it's probably fine to enforce BGRA for the webrender-specific code paths since it'll probably take webrender a little while before it reaches configurations that don't support it and we can fix the webgl readback code whenever webrender gets support for RGBA.
Flags: needinfo?(nical.bugzilla)
I really really don't want to try supporting BGRA again in WebGL. It was only pain.
It sounds like we'll need RGBA support in WR eventually, so very strong preference for just adding it.
Priority: -- → P3
See Also: → 1508096
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Attachment #9026632 - Attachment is obsolete: true
Component: Graphics → Graphics: WebRender
When I enabled Wayland on Ubuntu 18.04, Firefox nightly automatically used Wayland and caused crash in Comment 0.
Duplicate of this bug: 1508964
With the patch, I confirmed that the crash was addressed.
Attachment #9026643 - Attachment is obsolete: true
Attachment #9026655 - Flags: review?(nical.bugzilla)
Comment on attachment 9026655 [details] [diff] [review]
patch - Add RGBA8 support to WebRender

Review of attachment 9026655 [details] [diff] [review]:
-----------------------------------------------------------------

Jamie had plans for the RGBA/BGRA stuff.

::: gfx/webrender/src/texture_cache.rs
@@ +951,5 @@
>          descriptor: &ImageDescriptor,
>      ) -> bool {
>          let mut allowed_in_shared_cache = true;
>  
> +        // TODO(sotaro): For now, anything that requests BGRA8 just fails to allocate

I think you meant RGBA8 here.
Attachment #9026655 - Flags: review?(nical.bugzilla) → review?(jnicol)
Update comment.
Attachment #9026655 - Attachment is obsolete: true
Attachment #9026655 - Flags: review?(jnicol)
Attachment #9026868 - Flags: review?(jnicol)
Comment on attachment 9026868 [details] [diff] [review]
patch - Add RGBA8 support to WebRender

I think the fact that GL_IMPLEMENTATION_COLOR_READ_FORMAT does not return GL_BGRA means that glReadPixels shouldn't work for BGRA. (Though I think in many cases it would actually work, just the driver doesn't report it, I don't think we can rely on it.) So we need to read back the data in RGBA. It seems like in CreateR8G8B8AX8(), if we added webrender to the areRGBAFormatsBroken, and layersNeedsManualSwap, then we would manually swap R and B before sending the data to webrender? However, I think we are better off making webrender handle RGBA.

This patch seems like a good first step.

In the future, for GLES and it's sub-par BGRA support, we'll want (an option to have) RGBA in the shared cache (and BGRA as an exception), then move gecko to provide RGBA as much as possible. We should put a bug on file for that.

Also, not an expert on this code, but I believe comment 9 is a better long term solution. That would avoid the need for readback and presumably be faster.

But for now this seems like a good way to make webrender robust if we pass it RGBA data.
Attachment #9026868 - Flags: review?(jnicol) → review+
Posted patch patch - Change to gecko (obsolete) — Splinter Review
Attachment #9026973 - Flags: review+
Priority: P3 → P2
Change to P2 by  Comment 20.
(Reporter)

Comment 29

5 months ago
Sotaro, can you please take this bug as you did the patches? Thanks.
Assignee: stransky → nobody
Flags: needinfo?(sotaro.ikeda.g)
(Reporter)

Updated

5 months ago
Blocks: 1508082
(In reply to Martin Stránský [:stransky] from comment #29)
> Sotaro, can you please take this bug as you did the patches? Thanks.

No problem. Thanks.
Assignee: nobody → sotaro.ikeda.g
Flags: needinfo?(sotaro.ikeda.g)
Rebased.
Attachment #9026973 - Attachment is obsolete: true
Attachment #9028134 - Flags: review+
Attachment #9026868 - Attachment is obsolete: true

Comment 32

5 months ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9853cc2d3e2e
Enable RGBA8 support of WebRender r=jnicol

Comment 33

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9853cc2d3e2e
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Attachment #9001886 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.