2087301 - drawImage() of a WebGL canvas onto a 2D canvas swaps red and blue when the 2D canvas is accelerated (regression in 159)

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Leo]

Attachment: graphics_info.txt

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:159.0) Gecko/20100101 Firefox/159.0

Steps to reproduce

Open this page:

<!doctype html>
<meta charset="utf-8">
<title>drawImage(webglCanvas) swaps R/B on larger 2D canvases</title>
<p>Both boxes should be red. With gfx.canvas.accelerated=true, the large one is blue.</p>
<canvas id="small" width="100" height="50"></canvas>
<canvas id="large" width="400" height="200"></canvas>
<script>
const glCanvas = document.createElement("canvas");
const gl = glCanvas.getContext("webgl");

for (const id of ["small", "large"]) {
  const canvas = document.getElementById(id);
  glCanvas.width = canvas.width;
  glCanvas.height = canvas.height;
  gl.clearColor(1, 0, 0, 1);
  gl.clear(gl.COLOR_BUFFER_BIT);
  canvas.getContext("2d").drawImage(glCanvas, 0, 0);
}
</script>

Actual results

The 100×50 canvas is red, the 400×200 canvas is blue.

Expected results

Both red, as in Chromium and in Firefox before 159. (maybe a little earlier)

Note: Setting gfx.canvas.accelerated = false and reloading fixes it. This affects Bokeh plots that use the WebGL backend with scatter plots.

Additional Info:

– The small canvas is below gfx.canvas.accelerated.min-size (128), so it skips the accelerated drawImage path. This suggests the bug is in that path.
– This probably belongs in Core :: Graphics: Canvas2D. Possibly regressed by bug 1938053.
– Environment: Firefox 159.0 (flatpak), Linux x86_64, Wayland. graphics info attached.

Comment 1

[BugBot [:suhaib / :marco]]

The Bugbug bot thinks this bug should belong to the 'Core::Graphics: Canvas2D' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Timothy Nikkel (:tnikkel)]

Thanks for the report. I couldn't reproduce this, but I'm on macOS, and your setup uses Linux-specific paths (Wayland, DMABuf, Mesa iris), so it may depend on platform or driver. Could you:

1. Attach the full about:support output ("Copy raw data to clipboard"), not only the graphics section, so we can see modified preferences.
2. Test in a fresh profile (about:profiles, then "Create a New Profile").
3. Set widget.dmabuf-webgl.enabled to false, restart Firefox (a restart is required), and say whether it still reproduces.
4. If you can, test a non-flatpak build (the tarball from mozilla.org) and say whether it reproduces. That tells us whether the flatpak runtime's Mesa is involved.
5. If you know when this started, say whether it lines up with a Firefox update or with a flatpak runtime or Mesa update. mozregression (https://mozilla.github.io/mozregression/) can find the exact build if it's a Firefox change.

Comment 3

[Jannik Kolodziej]

Thanks for looking into this.

I'm not the original reporter, but I'm affected by this bug too.

1. I'm going to attach my support.json from a fresh Firefox profile. Notably it reproduces for me under Linux with AMD and x11.
2. It reproduces with a fresh profile.
3. widget.dmabuf-webgl.enabled to false fixes the issue
4. The mozilla.org tarball for 159.0 reproduces the issue
5. It appears to me the first bad revision is f7c591a105d9323d1ae7e48a36deda0a7cfe5daf

1:21.70 INFO: No more integration revisions, bisection finished.
1:21.70 INFO: Last good revision: d23fa3afde2dbc18239a29d59a46c2c1872f237c
1:21.70 INFO: First bad revision: f7c591a105d9323d1ae7e48a36deda0a7cfe5daf
1:21.70 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d23fa3afde2dbc18239a29d59a46c2c1872f237c&tochange=f7c591a105d9323d1ae7e48a36deda0a7cfe5daf
1:23.65 DEBUG: Found commit message:
Bug 2070629 - Implement zero-copy DMABuf VideoFrame import to remote Canvas2D on Linux r=gfx-reviewers,lsalzman

Differential Revision: https://phabricator.services.mozilla.com/D327325

Note: For me, this surfaced in Jitsi Meet, where enabling any virtual webcam background changes colors.

Comment 4

[Jannik Kolodziej]

Attachment: about-support.json

Comment 5

[Timothy Nikkel (:tnikkel)]

Thanks Jannik, that bisection is very helpful.

Bug 2070629 added a red/blue swap to the DMABuf blit in GLBlitHelper::Blit(DMABufSurface*). A single-plane RGBA DMABuf gets swapped whenever the destination format is BGRA, because srcIsBGRA is hardcoded to false. It also made SharedContextWebgl::ImportSurfaceDescriptor pass the destination format to that blit. That function is shared with the WebGL snapshot import that accelerated canvas uses for drawImage(webglCanvas) (CanvasTranslator::ResolveExternalSnapshot), so WebGL backbuffers shared as DMABufs now get swapped too. The new gfx.canvas.remote.use-draw-image-fast-path-dmabuf pref doesn't cover this, so it happens with the pref at its default. That fits widget.dmabuf-webgl.enabled=false working around it.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Not sure if there's a quick fix we could land for 159.0.1 (backing out the regressing change looks a bit scary in its own right), but I'm open to suggestions if we have options here.

Comment 7

[Abhinav Singh]

Hi, I am the author of bug 2070629, so this regression is from my patch. I reproduced it locally on AMD: small canvas red, large canvas blue, same as the report.

tnikkel's analysis in comment 5 is correct. Some detail on the mechanism:

1. My patch made SharedContextWebgl::ImportSurfaceDescriptor pass the destination format into GLBlitHelper::Blit(DMABufSurface*), which swaps R/B whenever the destination is BGRA.
2. That function hardcodes "srcIsBGRA = false". That is right for YUV video frames (the color matrix outputs true RGB), but wrong for single-plane RGBA DMABufs. A WebGL backbuffer is "ARGB8888", already BGRA-ordered and sampled raw, so it must not be swapped.
3. ImportSurfaceDescriptor is shared with the WebGL snapshot path (CanvasTranslator::ResolveExternalSnapshot), so drawImage(webglCanvas) on an accelerated 2D canvas hits the same swap, the new pref does not cover this path.

My idea for the fix is to derive srcIsBGRA from surface->GetFormat() (B8G8R8A8/B8G8R8X8) instead of hardcoding it to false. Then the swap happens only when source and destination orders really differ: YUV sources keep the swap exactly as today, while BGRA-ordered RGBA sources skip it.
Does this approach sound right? If you agree I will put up a revision with local verification (the reproducer from this bug plus the original VideoFrame zero-copy path).

Comment 8

[Timothy Nikkel (:tnikkel)]

Can you just remove the line texDesc.destFormat = aFormat; in SharedContextWebgl::ImportSurfaceDescriptor? Nothing else sets destFormat, so everything else goes back to the old behaviour. It would probably break the new zero-copy path, but that's disabled by default. Then fix it properly on trunk.

Comment 9

[Abhinav Singh]

A fix is incoming: removing the destFormat assignment as the minimal 159.0.1 candidate, plus a trunk revision that preserves the pref-gated path. Build and local verification in progress, revision in the next few hours (by ~18:00 UTC).

Comment 10

[Abhinav Singh]

Could someone assign this bug to me? My account cannot change the Assignee field. I am working on the fix.

Comment 11

[Abhinav Singh]

Attachment: Bug 2087301 - Derive DMABuf blit channel order from source format. r?lsalzman

Comment 12

[Abhinav Singh]

Revision up: D338339. It derives srcIsBGRA from the DMABuf surface format instead of hardcoding false, so the swap applies only when source and destination orders differ. Verified locally on 161.0a1 with the reproducer from this bug (both canvases red; was small-red/large-blue before the fix).
tnikkel: this keeps the pref-gated zero-copy path correct, unlike the one-liner, but the one-liner remains available if release prefers it smaller for 159.0.1.
RyanVM: 3 lines of code (plus a comment), Linux-only, no new shaders. Requesting uplift to 159 (and 160) once reviewed.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Thanks. From a tracking standpoint, it'd probably make sense to keep this bug focused on the targeted fix to land and uplift across all branches, then file a new bug for any trunk-only work you'd like to land afterwards.

Comment 14

[Abhinav Singh]

D338339 is intended as the single targeted fix for all branches (159/160/161) - there is no separate trunk-only work planned. If release prefers the one-liner from comment 8 for 159.0.1, I can put that up instead

Comment 15

[Ryan VanderMeulen [:RyanVM]]

I'll defer to Lee on that. Thanks for the quick patch!

Comment 16

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Distorted rendering with accelerated canvas2d and webgl on Linux.
• Code covered by automated testing?: unknown
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Ensures source format for HW blit is correctly determined
• String changes made/needed?: None
• Is Android affected?: no

Comment 17

[Lee Salzman [:lsalzman]]

Attachment: Bug 2087301 - Derive DMABuf blit channel order from source format.

Original Revision: https://phabricator.services.mozilla.com/D338339

Comment 18

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: Distorted rendering with accelerated canvas2d and webgl on Linux.
• Code covered by automated testing?: unknown
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Ensures source format for HW blit is correctly determined
• String changes made/needed?: None
• Is Android affected?: no

Comment 19

[Lee Salzman [:lsalzman]]

Attachment: Bug 2087301 - Derive DMABuf blit channel order from source format.

Original Revision: https://phabricator.services.mozilla.com/D338339

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e71a438241ce
https://hg.mozilla.org/integration/autoland/rev/05c12b86383f
Derive DMABuf blit channel order from source format. r=lsalzman

Comment 21

[rperta]

https://hg.mozilla.org/mozilla-central/rev/05c12b86383f

Comment 22

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/6dc14bf2ab22
https://hg.mozilla.org/releases/mozilla-beta/rev/7195d4030baa

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Hi Leo, the fix for this bug is now available in Nightly builds. Can you try testing them to confirm that the issue is fixed on your end?

Comment 24

[Leo]

Yes, bug is fixed on Nightly. Thanks!

Comment 25

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/6f8f3733bdb4
https://hg.mozilla.org/releases/mozilla-release/rev/f2a41c6e6511

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Added to the 159.0.1 relnotes.

Fixed red and blue colors appearing swapped on some websites that use WebGL on Linux, including video call background effects.