Closed Bug 1709548 Opened 6 months ago Closed 6 months ago

Flickering black bars on Mali-G77

Categories

(Core :: Graphics: WebRender, defect)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox88 --- wontfix
firefox89 --- fixed
firefox90 --- verified

People

(Reporter: jnicol, Assigned: jnicol)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(3 files)

Attached image screenshot

From https://github.com/mozilla-mobile/fenix/issues/19342

Go to https://impfdashboard.de/ and scroll down.
Then press on one of the items.

Black bars flicker across the screen

User reported this on a Samsung S20 FE (Mali G77). I can reproduce on my Samsung S20 Ultra (also mali g77). I cannot reproduce on an S10 (Mali-G76).

This is a user-facing regression since bug 1685276, when we re-enabled partial present on Mali-G77. We had previously disabled partial present in bug 1676474 due to it being completely broken (garbage rendering on pretty much every page), which was a regression from bug 1675159. Interestingly, if I cherry pick the eventual fix from bug 1685276 on to the commit which completely broke it (bug 1675159) this black bars issue does not reproduce. So there was an additional regression which caused this more minor black bars issue, as well as the regression which completely broke partial present.

Bisecting by hand points to this commit

So the significance of the Bug 1676559 - Pt 4 Remove RenderPassKind commit is that on this page it causes us to continually composite even when nothing is changing. calculate_dirty_rects() keeps calculating an empty dirty rect, so in composite_simple() we call eglSetDamageRegion with an empty rect, then we clear the framebuffer (with an empty scissor rect), we don't composite tiles because they are clipped out, then we swap buffers. Prior to that commit, we didn't even get as far as calling calculate_dirty_rects() when nothing was changing.

Calling eglSetDamageRegion with an empty rect is valid according to the spec. But you are still not allowed to render outwith the dirty rect you specify. So in this case we cannot render to the backbuffer at all. As far as I can tell we are upholding our end of the bargain here, but the Mali-G77 driver doesn't like the call to glClear, even with the empty scissor rect set. Skipping calling glClear when the dirty rect is empty appears to prevent the bug.

Orthogonally to that fix, it doesn't seem great that we are continually doing extra work compared to before on this page when nothing is changing. In practice maybe it's okay if eglSwapBuffersWithDamage with an empty damage rect is implemented properly, but perhaps we can avoid compositing altogether. Nical, Glenn, do you think there's anything to be concerned about here?

Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(gwatson)

On Mali-G77 devices we have seen an flickering black bars appear on
pages where we are compositing with an empty dirty region. In such
cases we end up calling eglSetDamageRegion with an empty region,
followed by glClear with an empty scissor rect. Technically calling
eglSetDamageRegion with an empty valid region is allowed, it just
means you cannot render to the backbuffer at all that frame. The
subsequent glClear call should therefore be fine since an empty
scissor rect is set, but the Mali driver does not like it.

This patch skips calling glClear altogether if the dirty region is
empty, preventing the black bars from appearing.

Pushed by jnicol@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bcca3ab58292
Don't clear backbuffer if dirty region is empty. r=nical
Flags: needinfo?(gwatson)
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

We should uplift this to beta. The first revision of the patch should apply cleanly to beta, but the version which landed fixed a conflict on central so will not. ie We want uplift https://phabricator.services.mozilla.com/D114346?id=435729

Do I need to upload a separate patch and request beta uplift for that, or can I request uplift for the current attachment?

Flags: needinfo?(ryanvm)
Flags: needinfo?(nical.bugzilla)

(In reply to Jamie Nicol [:jnicol] from comment #5)

Do I need to upload a separate patch and request beta uplift for that, or can I request uplift for the current attachment?

Probably less confusing to just upload a new patch for the Beta uplift.

Flags: needinfo?(ryanvm)

On Mali-G77 devices we have seen an flickering black bars appear on
pages where we are compositing with an empty dirty region. In such
cases we end up calling eglSetDamageRegion with an empty region,
followed by glClear with an empty scissor rect. Technically calling
eglSetDamageRegion with an empty valid region is allowed, it just
means you cannot render to the backbuffer at all that frame. The
subsequent glClear call should therefore be fine since an empty
scissor rect is set, but the Mali driver does not like it.

This patch skips calling glClear altogether if the dirty region is
empty, preventing the black bars from appearing.

Comment on attachment 9220605 [details]
WIP: Bug 1709548 - Don't clear backbuffer if dirty region is empty.

Beta/Release Uplift Approval Request

  • User impact if declined: Black bars flickering on some pages for users with Mali-G77 GPUs
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Skips a redundant opengl call (clearing an empty region) to avoid driver bug
  • String changes made/needed:
Attachment #9220605 - Flags: approval-mozilla-beta?

Jamie, just checking that you attached the right patch for uplift as the patch starts with a ' WIP' mention :)

Flags: needinfo?(jnicol)

Yes that's the right patch. I think phabricator just added the "WIP" because I didn't request a review of it (as the same patch had already been approved)

Flags: needinfo?(jnicol)

Comment on attachment 9220605 [details]
WIP: Bug 1709548 - Don't clear backbuffer if dirty region is empty.

Approved for our next beta, thanks

Attachment #9220605 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified as fixed on the latest Nightly build 90.0a1 with Samsung Galaxy S20 FE (Android 11) Malig-77.
Note that it rendered correctly and no flickering black bar was displayed.

You need to log in before you can comment on or make changes to this bug.