Closed Bug 1647946 Opened 4 years ago Closed 4 years ago

Investigate correctness and stability issues with RenderCompositorNativeSWGL

Categories

(Core :: Graphics: WebRender, task, P3)

Product:
Core
Component:
Graphics: WebRender
Platform:
Unspecified
macOS
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla80
Tracking Flags:
Tracking Status
firefox80 --- fixed

People

(Reporter: lsalzman, Assigned: bpeers)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Screen Shot 2020-07-14 at 11.19.40 AM.png
416.88 KB, image/png
Details
B47DD1E1-A0BC-4E87-825C-18AB86458F51.jpeg
315.38 KB, image/jpeg
Details
Bug 1647946 - SWGL Mac Correctness issues r=lsalzman
47 bytes, text/x-phabricator-request
Details | Review

Markus reported that there are both problems with update regions and crashing when using SWGL via RenderCompositorNativeSWGL on mac from bug 1646835. Once bug 1646835 lands, we can investigate that here as follow-up.

My best bet for the correctness issues is that it has to do with origin top left vs bottom left, somehow.

(In reply to Markus Stange [:mstange] from comment #1)

My best bet for the correctness issues is that it has to do with origin top left vs bottom left, somehow.

Can you post a screenshot of what's going on?

I also have a suspicion that we may need to use kIOSurfaceAllocSize in MacIOSurface::CreateIOSurface to enforce a SIMD unit of slop at the end of the buffer, which SWGL currently relies upon for masking off partial stores quickly. The data in that case just gets read and written back out without modification (masked to fall within the valid range of a span), but this can cause traps if there's not space allocated. This usually only causes a problem on the last row of an image. That could be the source of some small amount of crashiness. Though possibly if coordinates are being wrongly flipped and things being improperly accessed, that might also be a source of crashes, or both...

For future ref: https://developer.apple.com/documentation/iosurface/iosurface_constants

Would it be enough to enforce a larger stride, or is space at the bottom required as well?

Here's a screen recording of the problems: https://drive.google.com/file/d/1uA_5p89lUKDRR4NN7cw4ARb9goFLzp-S/view?usp=sharing

Just need to ensure there are 16 bytes of unused space at the end of the last row. The other rows don't matter at all, since an overread of an intermediate row will just read some junk data from the next row, which is okay. But on the last row, it can run off the end without the extra bytes of padding for it to read. The masked reads themselves are potentially unaligned, so even if the stride of the row was somehow aligned to SIMD width, if the draw started on, say, the last pixel of that row, it will still read 3 pixels past it off the end (up to 4 pixels packed into one read).

In any case, doing it via stride alignment would be pretty wasteful since it would put 16 bytes of unused space at the end of every row, though it could still work. SWGL internally just inflates every texture allocation size by 16 bytes to handle this.

Quick call stack of one crash:

* thread #23, name = 'Renderer', stop reason = EXC_BAD_ACCESS (code=1, address=0x165504000)
  * frame #0: 0x000000010a1c8af5 XUL`void draw_quad_spans<unsigned int>(int, glsl::vec2_scalar*, unsigned short, glsl::vec3*, Texture&, int, Texture&, ClipRect const&) [inlined] unsigned char vector[16] glsl::Unaligned<unsigned char vector[16]>::load<unsigned int>(p=<unavailable>) at vector_type.h:418:5 [opt]
    frame #1: 0x000000010a1c8af5 XUL`void draw_quad_spans<unsigned int>(int, glsl::vec2_scalar*, unsigned short, glsl::vec3*, Texture&, int, Texture&, ClipRect const&) [inlined] unsigned char vector[16] glsl::unaligned_load<unsigned char vector[16], unsigned int>(p=<unavailable>) at vector_type.h:447 [opt]
    frame #2: 0x000000010a1c8af5 XUL`void draw_quad_spans<unsigned int>(int, glsl::vec2_scalar*, unsigned short, glsl::vec3*, Texture&, int, Texture&, ClipRect const&) [inlined] void discard_output<false>(buf=<unavailable>, mask=([0] = 0x00, [1] = 0x00, [2] = 0x00, [3] = 0x00, [4] = 0xff, [5] = 0xff, [6] = 0xff, [7] = 0xff, [8] = 0xff, [9] = 0xff, [10] = 0xff, [11] = 0xff, [12] = 0xff, [13] = 0xff, [14] = 0xff, [15] = 0xff)) at gl.cc:2474 [opt]
    frame #3: 0x000000010a1c8af5 XUL`void draw_quad_spans<unsigned int>(int, glsl::vec2_scalar*, unsigned short, glsl::vec3*, Texture&, int, Texture&, ClipRect const&) [inlined] void commit_output<false, false, unsigned int, unsigned char vector[16]>(buf=<unavailable>, mask=([0] = 0x00, [1] = 0x00, [2] = 0x00, [3] = 0x00, [4] = 0xff, [5] = 0xff, [6] = 0xff, [7] = 0xff, [8] = 0xff, [9] = 0xff, [10] = 0xff, [11] = 0xff, [12] = 0xff, [13] = 0xff, [14] = 0xff, [15] = 0xff)) at gl.cc:2564 [opt]
    frame #4: 0x000000010a1c8aeb XUL`void draw_quad_spans<unsigned int>(int, glsl::vec2_scalar*, unsigned short, glsl::vec3*, Texture&, int, Texture&, ClipRect const&) [inlined] void commit_output<false, false, unsigned int, unsigned short>(buf=0x0000000165503ffc, z=65318, zbuf=0x000000012f0ffffe, span=1) at gl.cc:2595 [opt]
    frame #5: 0x000000010a1c8a22 XUL`void draw_quad_spans<unsigned int>(int, glsl::vec2_scalar*, unsigned short, glsl::vec3*, Texture&, int, Texture&, ClipRect const&) [inlined] void draw_span<false, false, unsigned int, void draw_quad_spans<unsigned int>(int, glsl::vec2_scalar*, unsigned short, glsl::vec3*, Texture&, int, Texture&, ClipRect const&)::'lambda'()>(buf=0x0000000165503ffc, depth=0x000000012f0ffffe, span=1, z=<unavailable>)::'lambda'()) at gl.cc:2827 [opt]
    frame #6: 0x000000010a1c859d XUL`void draw_quad_spans<unsigned int>(nump=<unavailable>, p=<unavailable>, z=65318, interp_outs=0x00007000059e2280, colortex=0x0000000128032e20, layer=0, depthtex=0x000000012af4fa10, clipRect=0x00007000059e2520) at gl.cc:3129 [opt]
    frame #7: 0x000000010a1bc144 XUL`draw_quad(nump=4, colortex=0x0000000128032e20, layer=0, depthtex=0x000000012af4fa10) at gl.cc:3595:5 [opt]
    frame #8: 0x000000010a0d84b0 XUL`::DrawElementsInstanced(GLenum, GLsizei, GLenum, void *, GLsizei) at gl.cc:3643:7 [opt]
    frame #9: 0x000000010a0d83df XUL`::DrawElementsInstanced(mode=<unavailable>, count=<unavailable>, type=5123, indicesptr=<unavailable>, instancecount=307) at gl.cc:3714 [opt]
    frame #10: 0x0000000109ff19d2 XUL`_$LT$gleam..gl..ErrorReactingGl$LT$F$GT$$u20$as$u20$gleam..gl..Gl$GT$::draw_elements_instanced::h03b3c6f8be1c917b(self=0x000000012afee650, mode=<unavailable>, count=<unavailable>, element_type=<unavailable>, indices_offset=<unavailable>, primcount=<unavailable>) at gl.rs:97:25 [opt]
    frame #11: 0x0000000109be2407 XUL`webrender::renderer::Renderer::draw_instanced_batch::h8c7d885fbadc98df(self=0x000000012b043000, data=&[webrender::gpu_types::PrimitiveInstanceData] @ 0x00007fc383731760, vertex_array_kind=<unavailable>, textures=<unavailable>, stats=0x00007000059e34f8) at renderer.rs:4073:12 [opt]
    frame #12: 0x0000000109f3bc29 XUL`webrender::renderer::Renderer::draw_alpha_batch_container::hecc3dc13e79324fa(self=<unavailable>, alpha_batch_container=0x000000012a5325a0, draw_target=DrawTarget @ 0x00007000059e2c70, content_origin=<unavailable>, framebuffer_kind=<unavailable>, projection=0x00007000059e2e50, render_tasks=0x000000012b841a90, stats=0x00007000059e34f8) at renderer.rs:4524:16 [opt]
    frame #13: 0x0000000109f3b3e9 XUL`webrender::renderer::Renderer::draw_picture_cache_target::hb949e0b95663f2c2(self=0x000000012b043000, target=<unavailable>, draw_target=DrawTarget @ 0x00007000059e2f78, content_origin=<unavailable>, projection=0x00007000059e2e50, render_tasks=0x000000012b841a90, stats=0x00007000059e34f8) at renderer.rs:4340:8 [opt]
    frame #14: 0x0000000109f3fe64 XUL`webrender::renderer::Renderer::draw_frame::hbc7d08efe2b886ce(self=0x000000012b043000, frame=0x000000012b841808, device_size=<unavailable>, frame_id=<unavailable>, results=0x00007000059e34f8, clear_framebuffer=true) at renderer.rs:6128:28 [opt]
    frame #15: 0x0000000109eeb68a XUL`webrender::renderer::Renderer::render_impl::_$u7b$$u7b$closure$u7d$$u7d$::h1145b324ea927246 at renderer.rs:3511:16 [opt]
    frame #16: 0x0000000109ee977b XUL`webrender::profiler::TimeProfileCounter::profile::hce02789c80dc195c(self=0x00007000059e3558, callback=closure-1 @ 0x00007000059e3678) at profiler.rs:471:18 [opt]
    frame #17: 0x0000000109f3934a XUL`webrender::renderer::Renderer::render_impl::h5b79ba3799c199a2(self=<unavailable>, device_size=<unavailable>) at renderer.rs:3477:8 [opt]
    frame #18: 0x0000000109f38bb6 XUL`webrender::renderer::Renderer::render::h7c9e8abd80c1c178(self=0x000000012b043000, device_size=<unavailable>) at renderer.rs:3255:21 [opt]
    frame #19: 0x0000000109ba83f8 XUL`wr_renderer_render(renderer=<unavailable>, width=<unavailable>, height=<unavailable>, out_stats=0x00007000059e39f0, out_dirty_rects=0x00007000059e3940) at bindings.rs:606:10 [opt]
    frame #20: 0x0000000103bf9e57 XUL`mozilla::wr::RendererOGL::UpdateAndRender(this=0x000000012aff4580, aReadbackSize=0x00007000059e3b30, aReadbackFormat=0x00007000059e3ae6, aReadbackBuffer=0x00007000059e3af0, aOutStats=0x00007000059e39f0) at RendererOGL.cpp:142:8 [opt]
    frame #21: 0x0000000103bf9049 XUL`mozilla::wr::RenderThread::UpdateAndRender(this=0x000000011ce68370, aWindowId=<unavailable>, aStartId=0x00007000059e3b48, aStartTime=<unavailable>, aRender=true, aReadbackSize=0x00007000059e3b30, aReadbackFormat=0x00007000059e3ae6, aReadbackBuffer=0x00007000059e3af0) at RenderThread.cpp:478:31 [opt]
    frame #22: 0x0000000103bf8903 XUL`mozilla::wr::RenderThread::HandleFrameOneDoc(this=0x000000011ce68370, aWindowId=<unavailable>, aRender=<unavailable>) at RenderThread.cpp:356:3 [opt]
    frame #23: 0x0000000103c0405f XUL`mozilla::detail::RunnableMethodImpl<mozilla::wr::RenderThread*, void (mozilla::wr::RenderThread::*)(mozilla::wr::WrWindowId, bool), true, (mozilla::RunnableKind)0, mozilla::wr::WrWindowId, bool>::Run() [inlined] decltype(o=<unavailable>, m=<unavailable>, args=<unavailable>).*fp0(Get<0ul>(fp1).PassAsParameter(), Get<1ul>(fp1).PassAsParameter())) mozilla::detail::RunnableMethodArguments<mozilla::wr::WrWindowId, bool>::applyImpl<mozilla::wr::RenderThread, void (mozilla::wr::RenderThread::*)(mozilla::wr::WrWindowId, bool), StoreCopyPassByConstLRef<mozilla::wr::WrWindowId>, StoreCopyPassByConstLRef<bool>, 0ul, 1ul>(mozilla::wr::RenderThread*, void (mozilla::wr::RenderThread::*)(mozilla::wr::WrWindowId, bool), mozilla::Tuple<StoreCopyPassByConstLRef<mozilla::wr::WrWindowId>, StoreCopyPassByConstLRef<bool> >&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>) at nsThreadUtils.h:1188:12 [opt]

I confirmed that the crash is indeed a 3-pixel-span going past the end of a 2MB buffer (from a 1024x512 RGBA TileCache).
I can't immediately fix it by allocating 16 extra bytes in the IOSurface, but I also can't make it crash right away with an obviously-wrong-size, so I may be doing it wrong.
Or, instead, not every allocation of a colortex is coming from that IOSurface?
Also, what about the depthtex? I imagine reading past the end of a 4KB page could also crash.

An alternative is for each span < 8 pixels to run the fragment shader on a local rgba[16] and then only transfer the valid pixels? I'm not sure how much perf. that might cost though.
Edit: or a variation on this idea is to shift the coordinates until all 4/8 pixels are within range, and adjust the mask accordingly. Eg. for a span == 3, the mask disables the first pixel and writes the last 3, instead of enabling the first 3 and disabling the last pixel-which-is-out-of-bounds.

diff --git a/gfx/2d/MacIOSurface.cpp b/gfx/2d/MacIOSurface.cpp
index 90d9a40ec2c8..89bbe4fd9885 100644
--- a/gfx/2d/MacIOSurface.cpp
+++ b/gfx/2d/MacIOSurface.cpp
@@ -36,7 +36,7 @@ already_AddRefed<MacIOSurface> MacIOSurface::CreateIOSurface(
   if (aContentsScaleFactor <= 0) return nullptr;
 
   CFMutableDictionaryRef props = ::CFDictionaryCreateMutable(
-      kCFAllocatorDefault, 4, &kCFTypeDictionaryKeyCallBacks,
+      kCFAllocatorDefault, 5, &kCFTypeDictionaryKeyCallBacks,
       &kCFTypeDictionaryValueCallBacks);
   if (!props) return nullptr;
 
@@ -47,6 +47,9 @@ already_AddRefed<MacIOSurface> MacIOSurface::CreateIOSurface(
   size_t intScaleFactor = ceil(aContentsScaleFactor);
   aWidth *= intScaleFactor;
   aHeight *= intScaleFactor;
+  size_t allocSize = aWidth * aHeight * bytesPerElem + /* SWGL SIMD slop */ 16;
+  // test if this does anything at all
+  allocSize = 1024;
   CFNumberRef cfWidth = ::CFNumberCreate(nullptr, kCFNumberSInt32Type, &aWidth);
   CFNumberRef cfHeight =
       ::CFNumberCreate(nullptr, kCFNumberSInt32Type, &aHeight);
@@ -59,6 +62,10 @@ already_AddRefed<MacIOSurface> MacIOSurface::CreateIOSurface(
   ::CFDictionaryAddValue(props, kIOSurfaceBytesPerElement, cfBytesPerElem);
   ::CFRelease(cfBytesPerElem);
   ::CFDictionaryAddValue(props, kIOSurfaceIsGlobal, kCFBooleanTrue);
+  CFNumberRef cfAllocSize =
+      ::CFNumberCreate(nullptr, kCFNumberSInt32Type, &allocSize);
+  ::CFDictionaryAddValue(props, kIOSurfaceAllocSize, cfAllocSize);
+  ::CFRelease(cfAllocSize);
 
   CFTypeRefPtr<IOSurfaceRef> surfaceRef =
       CFTypeRefPtr<IOSurfaceRef>::WrapUnderCreateRule(::IOSurfaceCreate(props));

Thanks, I'll try that.

I worked around the crash (don't rasterize the last line) and noticed that the glitchiness goes away if you disable picture caching -- but then everything is upside down (with flip == false in gl.cc Composite).

Allocate extra 16 bytes to support past-the-end reads of SWGL.

Assignee: nobody → bpeers
Status: NEW → ASSIGNED
Keywords: leave-open
Depends on: 1652597
No longer depends on: 1652597
Keywords: leave-open
Depends on: 1652597

Comment on attachment 9163312 [details]
Bug 1647946 - SWGL crash on Mac r=mstange

Revision D83389 was moved to bug 1652597. Setting attachment 9163312 [details] to obsolete.

Attachment #9163312 - Attachment is obsolete: true
Depends on: 1652601

It seems like we can get 80% there by clearing out surfaces when they are recycled from the pool:

diff --git a/gfx/layers/SurfacePoolCA.mm b/gfx/layers/SurfacePoolCA.mm
index 7d7f05c17db8..7b180a752288 100644
--- a/gfx/layers/SurfacePoolCA.mm
+++ b/gfx/layers/SurfacePoolCA.mm
@@ -154,6 +154,15 @@ CFTypeRefPtr<IOSurfaceRef> SurfacePoolCA::LockedPool::ObtainSurfaceFromPool(cons
       mInUseEntries.insert({surface, std::move(*iterToRecycle)});
       mAvailableEntries.RemoveElementAt(iterToRecycle);
     });
+
+    // TESTING: Zero-initialize any reused surfaces.
+    IOSurfaceLock(surface.get(), 0, nullptr);
+    void* base = IOSurfaceGetBaseAddress(surface.get());
+    size_t bytesPerRow = IOSurfaceAlignProperty(kIOSurfaceBytesPerRow, aSize.width * 4);
+    size_t allocSize = IOSurfaceAlignProperty(kIOSurfaceAllocSize, bytesPerRow * aSize.height);
+    memset(base, 0, allocSize);
+    IOSurfaceUnlock(surface.get(), 0, nullptr);
+
     return surface;
   }

This could explain why the problem is SWGL-Mac only -- other platforms may not pool surfaces, or do it in a way that zero-clears them on reuse, and non-SWGL may clear reused surfaces when it binds them to OpenGL or somesuch (I'm speculating, please correct me if I'm wrong).

If it makes sense but the extra cost is unwarranted for non-SWGL, then we could only do this (and the SIMD padding) if prefs::IsSoftware.

The remaining problems seem to be some kind of misalignment or shifting -- ad banner slices are half hidden behind chrome, tiles of text are broken up and shifted around. Some pages now look quite good, others not so much. The scrollbar is broken in an interesting way too.

This must mean that there's a mismatch between the update region passed to NativeLayerCA::NextSurfaceAsDrawTarget and what pixels actually end up being drawn to. NativeLayerCA::HandlePartialUpdate and NativeLayerCA::NotifySurfaceReady() have assertions that try hard to make sure that no uninitialized pixels are getting to the screen.

Maybe this shows it a bit better;
Non-SWGL on the left, SWGL on the right.

You can use this patch to highlight areas that should be drawn to but aren't:

diff --git a/gfx/layers/NativeLayerCA.mm b/gfx/layers/NativeLayerCA.mm
--- a/gfx/layers/NativeLayerCA.mm
+++ b/gfx/layers/NativeLayerCA.mm
@@ -566,16 +566,24 @@ RefPtr<gfx::DrawTarget> NativeLayerCA::N
           for (auto iter = copyRegion.RectIter(); !iter.Done(); iter.Next()) {
             const gfx::IntRect& r = iter.Get();
             dt->CopySurface(sourceSurface, r, r.TopLeft());
           }
         }
         source->Unlock(true);
       });
 
+  float r = float(rand()) / float(RAND_MAX);
+  float g = float(rand()) / float(RAND_MAX);
+  float b = float(rand()) / float(RAND_MAX);
+  gfxUtils::ClipToRegion(dt, aUpdateRegion);
+  dt->FillRect(gfx::Rect(aUpdateRegion.GetBounds()),
+               gfx::ColorPattern(gfx::DeviceColor(r, g, b, 1.0)));
+  dt->PopClip();
+
   return dt;
 }
 
 Maybe<GLuint> NativeLayerCA::NextSurfaceAsFramebuffer(const IntRect& aDisplayRect,
                                                       const IntRegion& aUpdateRegion,
                                                       bool aNeedsDepth) {
   MutexAutoLock lock(mMutex);
   if (!NextSurface(lock)) {

I found one problem with it but I think there's multiple problems. (The problem I found was that the data/buf pointer that MapTile returns points to the tile origin, not to the valid rect origin, but SWGL expects it to point to the valid rect origin. I guess Lee needs to decide what the semantics of MapTile should be.)

Further experimentation leads me to believe that SWGL has a bug when clearing something with transparency. You can see this in the overlay arrows for the swipe back/forward indicators at the sides of the window. The layers that contain these arrows have some transparent padding at the top, but that padding never gets written. Only the part of the layer that contains the arrow graphic is written. So the part at the top stays uninitialized.

Oh, nice find, thanks! Yes things do look a lot better with this adjustment (not sure if we want the DirtyRect or the ValidRect).

diff --git a/gfx/webrender_bindings/RenderCompositorNative.cpp b/gfx/webrender_bindings/RenderCompositorNative.cpp
index 528128c1b3e5..3cab71572e10 100644
--- a/gfx/webrender_bindings/RenderCompositorNative.cpp
+++ b/gfx/webrender_bindings/RenderCompositorNative.cpp
@@ -433,7 +433,7 @@ bool RenderCompositorNativeSWGL::InitDefaultFramebuffer(
       return false;
     }
     wr_swgl_init_default_framebuffer(mContext, aBounds.width, aBounds.height,
-                                     mLayerStride, mLayerData);
+                                     mLayerStride, mLayerDirtyRectData);
   }
   return true;
 }
@@ -466,6 +466,7 @@ bool RenderCompositorNativeSWGL::MapNativeLayer(
              format == gfx::SurfaceFormat::B8G8R8X8);
   mLayerTarget = std::move(dt);
   mLayerData = data;
+  mLayerDirtyRectData = data + aDirtyRect.y * stride + aDirtyRect.x * 4;
   mLayerStride = stride;
   return true;
 }
@@ -475,6 +476,7 @@ void RenderCompositorNativeSWGL::UnmapNativeLayer() {
   mLayerTarget->ReleaseBits(mLayerData);
   mLayerTarget = nullptr;
   mLayerData = nullptr;
+  mLayerDirtyRectData = nullptr;
   mLayerStride = 0;
 }
 
@@ -494,7 +496,7 @@ bool RenderCompositorNativeSWGL::MapTile(wr::NativeTileId aId,
     UnbindNativeLayer();
     return false;
   }
-  *aData = mLayerData;
+  *aData = mLayerDirtyRectData;
   *aStride = mLayerStride;
   return true;
 }
diff --git a/gfx/webrender_bindings/RenderCompositorNative.h b/gfx/webrender_bindings/RenderCompositorNative.h
index eb356651c38f..140067946360 100644
--- a/gfx/webrender_bindings/RenderCompositorNative.h
+++ b/gfx/webrender_bindings/RenderCompositorNative.h
@@ -191,6 +191,7 @@ class RenderCompositorNativeSWGL : public RenderCompositorNative {
   void* mContext = nullptr;
   RefPtr<gfx::DrawTarget> mLayerTarget;
   uint8_t* mLayerData = nullptr;
+  uint8_t* mLayerDirtyRectData = nullptr;
   int32_t mLayerStride = 0;
 };

The zero-clear is still necessary to prevent random chunks of other tabs from showing up, so that might still be a part of the fix.

I logged out all HandlePartialUpdate regions and didn't see any difference in swgl on or off, so if SWGL also makes no difference for anything downstream from that (I think it's all Skia, Blitter_Memcpy and such?) then the remaining bugs must be above that layer?

(In reply to Bert Peers [:bpeers] from comment #17)

Oh, nice find, thanks! Yes things do look a lot better with this adjustment (not sure if we want the DirtyRect or the ValidRect).

These lines seem to want the valid rect, not the dirty rect:
https://searchfox.org/mozilla-central/rev/82c04b9cad5b98bdf682bd477f2b1e3071b004ad/gfx/webrender_bindings/src/swgl_bindings.rs#444-445,471

The zero-clear is still necessary to prevent random chunks of other tabs from showing up, so that might still be a part of the fix.

I logged out all HandlePartialUpdate regions and didn't see any difference in swgl on or off, so if SWGL also makes no difference for anything downstream from that (I think it's all Skia, Blitter_Memcpy and such?) then the remaining bugs must be above that layer?

That's right. I think SWGL is failing to do its own zero-clear. Hardware GL does the zero clear successfully.

Yeah, it seems like the delayed clear mechanism is the cause: when disabled, all missing areas get filled in, and that includes the IOSurface (ie. no need for a zero-clear).
So there is probably just a prepare_texture missing, the question is where.

diff --git a/gfx/webrender_bindings/RenderCompositorNative.cpp b/gfx/webrender_bindings/RenderCompositorNative.cpp
index 528128c1b3e5..9548043d85a3 100644
--- a/gfx/webrender_bindings/RenderCompositorNative.cpp
+++ b/gfx/webrender_bindings/RenderCompositorNative.cpp
@@ -433,7 +433,7 @@ bool RenderCompositorNativeSWGL::InitDefaultFramebuffer(
       return false;
     }
     wr_swgl_init_default_framebuffer(mContext, aBounds.width, aBounds.height,
-                                     mLayerStride, mLayerData);
+                                     mLayerStride, mLayerValidRectData);
   }
   return true;
 }
@@ -466,6 +466,7 @@ bool RenderCompositorNativeSWGL::MapNativeLayer(
              format == gfx::SurfaceFormat::B8G8R8X8);
   mLayerTarget = std::move(dt);
   mLayerData = data;
+  mLayerValidRectData = data + aValidRect.y * stride + aValidRect.x * 4;
   mLayerStride = stride;
   return true;
 }
@@ -475,6 +476,7 @@ void RenderCompositorNativeSWGL::UnmapNativeLayer() {
   mLayerTarget->ReleaseBits(mLayerData);
   mLayerTarget = nullptr;
   mLayerData = nullptr;
+  mLayerValidRectData = nullptr;
   mLayerStride = 0;
 }
 
@@ -494,7 +496,7 @@ bool RenderCompositorNativeSWGL::MapTile(wr::NativeTileId aId,
     UnbindNativeLayer();
     return false;
   }
-  *aData = mLayerData;
+  *aData = mLayerValidRectData;
   *aStride = mLayerStride;
   return true;
 }
diff --git a/gfx/webrender_bindings/RenderCompositorNative.h b/gfx/webrender_bindings/RenderCompositorNative.h
index eb356651c38f..89811a6ae389 100644
--- a/gfx/webrender_bindings/RenderCompositorNative.h
+++ b/gfx/webrender_bindings/RenderCompositorNative.h
@@ -191,6 +191,7 @@ class RenderCompositorNativeSWGL : public RenderCompositorNative {
   void* mContext = nullptr;
   RefPtr<gfx::DrawTarget> mLayerTarget;
   uint8_t* mLayerData = nullptr;
+  uint8_t* mLayerValidRectData = nullptr;
   int32_t mLayerStride = 0;
 };
 
diff --git a/gfx/wr/swgl/src/gl.cc b/gfx/wr/swgl/src/gl.cc
index 16ff78ccd12b..8fde706a769f 100644
--- a/gfx/wr/swgl/src/gl.cc
+++ b/gfx/wr/swgl/src/gl.cc
@@ -1464,7 +1464,7 @@ void TexSubImage3D(GLenum target, GLint level, GLint xoffset, GLint yoffset,
   GLsizei row_length =
       ctx->unpack_row_length != 0 ? ctx->unpack_row_length : width;
   if (format == GL_BGRA) {
-    assert(ty == GL_UNSIGNED_BYTE);
+    //assert(ty == GL_UNSIGNED_BYTE);
     assert(t.internal_format == GL_RGBA8);
   } else {
     assert(t.internal_format == internal_format_for_data(format, ty));
@@ -2019,7 +2019,8 @@ void Clear(GLbitfield mask) {
       if (clear_requires_scissor(t)) {
         force_clear<uint32_t>(t, &ctx->scissor);
         clear_buffer<uint32_t>(t, color, fb.layer);
-      } else if (t.depth > 1) {
+      //} else if (t.depth > 1) {
+      } else if (true) {
         // Delayed clear is not supported on texture arrays.
         t.disable_delayed_clear();
         clear_buffer<uint32_t>(t, color, fb.layer);
@@ -2032,7 +2033,8 @@ void Clear(GLbitfield mask) {
       if (clear_requires_scissor(t)) {
         force_clear<uint8_t>(t, &ctx->scissor);
         clear_buffer<uint8_t>(t, color, fb.layer);
-      } else if (t.depth > 1) {
+      //} else if (t.depth > 1) {
+      } else if (true) {
         t.disable_delayed_clear();
         clear_buffer<uint8_t>(t, color, fb.layer);
       } else {

Bert, maybe look here: https://searchfox.org/mozilla-central/source/gfx/webrender_bindings/src/swgl_bindings.rs#496

That call to get_color_buffer is short-circuited a few lines above if there is a native compositor, where it calls unmap_tile and bails. If you move the call to get_color_buffer earlier, which will force it to flush the delayed clear, does that help things?

Thanks Lee, that does the trick. I was going to pass the tile.color_id to unmap_tile, but I like your fix better :o)
I'll send out a patch with the lock-offset and the resolve, and test that it didn't break anything on Windows.
Cheers!

1/ the texture binding expects the pixel pointer to start at the top
left of the valid rectangle, not the top left of the entire mapped
layer;

2/ pending clears must be resolved before compositing the texture;

Pushed by bpeers@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5f079456c2d
SWGL Mac Correctness issues r=lsalzman,mstange

https://hg.mozilla.org/mozilla-central/rev/b5f079456c2d

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: