[Impeller][GLES] Rebind textures before buffer uploads - #192158

Thanks — I like the cleanup direction and agree that leaving texture bindings behind at self-contained upload boundaries is worth addressing.

My one concern is that post-use unbinding at these two sites may not repair a stale same-name binding that is already present when the next blit upload starts. In the deterministic trace, the old name 7 binding was created through TextureGLES::InitializeContentsIfNecessary and framebuffer attachment, rather than OnSetContents. If that deleted object is still represented by binding 7 in the resource context when glGenTextures returns 7 again, the first BindTexture(target, 7) in the proposed sequence may already be the operation that the driver treats as no state transition. The cleanup at the end would then be too late for that upload.

That said, this is inferred from the trace rather than confirmed driver internals. I will test your exact two-site cleanup without the current pre-bind workaround against the deterministic physical-device reproduction and report the result here before deciding which form to keep.

Thanks for the suggestion — I tested the exact cleanup-only variant: the current pre-bind was removed, and post-use BindTexture(target, 0) was added only in BlitCopyBufferToTextureCommandGLES and TextureGLES::OnSetContents.

The APK/lib SHA and the crashing tombstone Build ID matched, so there was no stale-build ambiguity. It still crashed in the same Scenario 9 after about 5.13 seconds; I did not repeat the run.

The preceding owner, logical handle 311/name 8, was created through InitializeContentsIfNecessary and a successful FBO attach, and passed through neither cleanup site. It was deleted from the other shared context, then name 8 was regenerated for logical handle 315. The new blit upload reported GL_NO_ERROR, but glIsTexture was false. The cleanup happened only after that upload; the pending GL_INVALID_OPERATION was followed by INCOMPLETE_ATTACHMENT and the blit_pass_gles.cc(88) fatal.

By contrast, the pre-bind 0 → name workaround passed 4/4 runs at 606 seconds each. This is limited evidence: the exact two-site cleanup is insufficient for this deterministic reproduction, while the driver-internal mechanism remains inferred from diagnostics on a single device.

The public evidence is summarized here.

I completed a diagnostics-free performance A/B for the unconditional pre-bind.

The same frozen app commit (01186607bfbf8794615061657cd99db088480f0b) was tested with the exact PR base (ce1f5ff8d3f0932e7ab683b7b7d2d8fa3d4e1c57) versus the tip (a7d9451e72ee62511197f17108203ecf8917fdde). DEPS was identical. The source diff was the production change in blit_command_gles.cc plus its unit test. Both builds were profile Impeller GLES arm64 APKs without diagnostics.

The target was Pixel 5 (redfin), Android 30, ko_KR, portrait. Scenario 7 used n=3 runs per variant in the preselected sequence B1, P1, P2, B2, B3, P3; all 6/6 runs passed in 602–603 seconds. The workload consists of batches of 20 1536x1536 PNG decode/upload/resize/display operations. It is not a tiny-upload microbenchmark.

Using the full paginated sample series, the run-percentile mean deltas were CPU p50 +0.104 percentage points, p90 -0.132 percentage points, and p99 +0.060 percentage points. End-to-end iterations were 1553.7 → 1518.7 (-2.25%), but same-variant run-to-run variation was substantial, so the bind cost remains inconclusive. memoryTotal changes are descriptive only and do not support a causal memory claim.

There was no consistent sampled CPU regression signal in this workload. graphicsStats was empty, so FPS, GPU time, frame pacing, thermal state, and DVFS effects were not measured. Test Lab did not guarantee the same physical unit. This experiment does not establish a performance upper bound; high-frequency tiny uploads such as glyph atlas, utility textures, and Flutter GPU overwrite remain unmeasured. I am not making a general negligible/no-regression claim.

The complete table, artifact identities, and limitations are documented here:
https://github.com/dfdgsdfg/mali-crash-app/blob/main/docs/evidence.md#unconditional-pre-bind-performance-ab

@flar can you be a secondary reviewer on this please

Updated proposal: would you consider applying the extra pre-bind to native GLES on Mali-G models with a numeric model identifier of 76 or lower, rather than restricting it to G52/G71/G72/G76 individually or applying it to all GPUs?

This scopes the workaround, not Impeller support. In the production records we have reviewed, this crash group includes G52, G71, G72, and G76; we have not identified a report on a Mali-G model numbered above 76. That is an observation from our available records, not evidence of sufficient exposure or proof that higher-numbered models are unaffected.

Why broaden the proposal? Driver versions can differ by manufacturer firmware and Android release. A passing test on one OS build does not establish that the same GPU model is unaffected on older firmware. A broader numerical cutoff would include untested models within the range instead of assuming they are safe merely because we lack reports.

Evidence so far:

• G71 / Galaxy A10 / Android 10: scenario 9 crashes with the G72-only guard inactive, using an ABI-matched armeabi-v7a APK. Test Lab reported a 16-second test, with Impeller OpenGLES and this sequence:

  GL_FRAMEBUFFER_INCOMPLETE_MISSING_ATTACHMENT
  [FATAL:flutter/impeller/renderer/backend/gles/blit_pass_gles.cc(88)]
  Check failed: result. Must be able to encode GL commands without error.
  SIGABRT
  

• G72 / S9 / Android 10: reproduced without pre-bind; the G72-conditional build completed 604 seconds.

• G52 / A31: present on Android 10 in our production report. The recent baseline pass was on Android 12, not the same OS/driver combination.

• G76: no suitable device was identified in the current Test Lab catalog. However, our Google Play Android vitals report includes Galaxy S10 5G / Exynos 9820 / Mali-G76 on Android 10 in the same reported error group: 10 reports / 1 user in that query window. This is production evidence, not a Test Lab reproduction.

• Controls: baseline scenario 9 completed on G57/A15/Android 14 (613 seconds) and G77/Note20 Ultra/Android 13 (602 seconds), one run each. These passes do not establish a driver-version boundary; under this proposal G57 would still receive the workaround.

Illustrative code, assuming the renderer has been parsed into an optional numeric Mali-G model identifier:

// Compute once during GLES description initialization.
// Parse the complete model token: G720 is 720, not G72.
needs_texture_upload_prebind_ =
    is_native_gles && !is_angle && mali_g_model.has_value() &&
    *mali_g_model > 0 && *mali_g_model <= 76;

// In BlitCopyBufferToTextureCommandGLES::Encode:
if (description.NeedsTextureUploadPrebind()) {
  gl.BindTexture(texture_type, 0u);
}
gl.BindTexture(texture_type, gl_handle.value());

The cutoff is explicitly numeric, not an architectural or chronological ordering: for example, G57 and G68 would be included, while G77 and G710 would not. It does not classify Mali-T-series or unknown renderer names. ANGLE would remain excluded. This is a conservative coverage proposal within that range, not proof that every included model is defective or that the cutoff is optimal.

Limitations: GPU labels in these device tests are model/SoC-based, without captured runtime GL renderer/driver strings. The G71 run differs in ABI/toolchain from the earlier arm64 baseline and is not a controlled A/B. We have not yet validated pre-bind enabled versus disabled on G71. A shared FBO/abort signature does not prove the same deleted-name-reuse mechanism on every family. The extra bind cost would remain on every direct buffer-to-texture upload on included GPUs; this proposal does not establish a performance upper bound or redesign other GLES binding paths.

For reference.

Mali-G72: reused texture name invalid after shared-context upload; zero-bind avoids failure.

Arm has confirmed this as a driver erratum. I filed it on the Arm developer forum and Arm replied with the errata writeup they're preparing:

Description — "Attaching a texture or renderbuffer to a framebuffer object
does not trigger synchronization with state updates made in a shared context."

Workaround — "You can bind the resource to a bind point using
glBindTexture() or glBindRenderBuffer() to force a state synchronization,
before attaching the resource to a framebuffer object."

https://community.arm.com/forums/f/mobile-graphics-and-gaming-forum/57861/mali-g72-reused-texture-name-invalid-after-shared-context-upload-zero-bind-avoids-failure/187047

Two things follow from that framing, and they bear on your original reviewcomment.

1. It explains why the zero-bind is load-bearing. Encode() already called BindTexture(target, handle), but the reused name was already the current binding, so that bind was redundant and no synchronization happened. Binding zero first makes it a real bind-point state change. That's also why an unbind-after-use variant works — either way the next bind is no longer redundant.

2. @gaaclarke You were right that "the current patch may still leave paths open". Arm describes the unsynchronized operation as the FBO attach, and TextureGLES::SetAsFramebufferAttachment() (texture_gles.cc:796) issues FramebufferTexture2D / FramebufferTexture2DMultisampleEXT / FramebufferRenderbuffer with no preceding bind at all. Callers are blit_command_gles.cc:49 and render_pass_gles.cc:290/297/305/657 — which is exactly the file your first suggested diff touched.

Per your original suggestion, I'll extend this PR to bind the resource before the attach in SetAsFramebufferAttachment and push an update shortly.

Arm has now identified the correct erratum for our API sequence: EN_ID 1,792,661, affecting Bifrost and Valhall drivers r17p0 through r23p0, with a fix in r24p0. They confirmed that binding texture zero before rebinding the desired name reliably avoids this issue.

Arm's updated response.

Based on that confirmation, I have kept the pre-upload zero-bind and have not added the post-use unbind in the two upload functions you suggested (BlitCopyBufferToTextureCommandGLES::Encode and TextureGLES::OnSetContents). To be precise, Arm confirmed the pre-bind workaround is sufficient for this erratum; they did not explicitly recommend against post-use cleanup. Omitting that additional cleanup is my implementation choice based on their response.

For now, commit cd0b145 targets the driver range Arm identified: on native GLES with a Mali-G / Immortalis-G renderer, the extra zero-bind applies to r17–r23, and is omitted for known releases before r17 or from r24 onward. If that driver version cannot be determined, the workaround remains enabled conservatively. OEM backports within the affected range cannot be detected.

This also corrects my earlier comment promising an extension to SetAsFramebufferAttachment: Arm subsequently clarified that their initial FBO-attachment diagnosis was a different issue.

@gaaclarke @flar Could you confirm which scope you would prefer? If you would like the workaround applied unconditionally across drivers, or the proposed post-use unbind added to both upload functions, I'm happy to revise the patch accordingly. Otherwise, could you please re-apply the CICD label so presubmit can run on the current driver-scoped implementation (cd0b145)? The PR is currently waiting on that label.

Validation: all 19 driver-scope regression cases pass; related GLES suites report 90 passed, 1 skipped. Host build and pre-push formatting checks passed. The new driver selector has not yet received physical-device validation; the prior device A/B results cover the zero-bind workaround itself.

@gaaclarke @flar Could you confirm which scope you would prefer? If you would like the workaround applied unconditionally across drivers, or the proposed post-use unbind added to both upload functions, I'm happy to revise the patch accordingly. Otherwise, could you please re-apply the CICD label so presubmit can run on the current driver-scoped implementation (cd0b145)? The PR is currently waiting on that label.

I was fine with applying it across drivers for now. If we are going to target specific drivers though I would want that added to CapabilitiesGLES not DescriptionGLES. Since you did good research to identify what is affected, it would be appreciated if you included the driver guard. It's up to you.

When you're ready for a rereview, just press the "Request re-review" button by my name.

@gaaclarke Updated in 5b2763f: moved the driver guard to CapabilitiesGLES, preserving the Arm-confirmed r17–r23 scope and the existing fallback for unparseable driver versions. Also merged the latest upstream/master available at the time (461663f).

Rebuilt with synced dependencies: related GLES tests passed (90 passed, 1 skipped), including all 19 driver-scope cases. Formatting checks also passed.

Could you please take another look and re-apply the CICD label so engine presubmit can run? Thanks!