Skip to content

[Impeller] Explore options for the const-on-mutation pattern in GLES TextureGLES #186371

Description

@bdero

Use case

engine/src/flutter/impeller/renderer/backend/gles/texture_gles.{h,cc} has a long-standing const-on-mutation pattern in its initialization tracking. The following methods are declared const but mutate internal state via a mutable member (slice_mip_initialized_):

  • TextureGLES::MarkSliceInitialized(size_t slice) const
  • TextureGLES::MarkContentsInitialized() const
  • TextureGLES::MarkSliceMipLevelInitialized(size_t slice, size_t mip_level) const
  • TextureGLES::InitializeContentsIfNecessary() const

These methods are called from other const methods on TextureGLES (Bind(), SetAsFramebufferAttachment(...)), which in turn are called from render-pass and framebuffer code paths that hold const TextureGLES&. The call graph:

flowchart TD
    R["Render pass & framebuffer code paths<br/>(hold const TextureGLES&)"]
    R --> B["Bind() const"]
    R --> S["SetAsFramebufferAttachment(...) const"]
    B --> I[InitializeContentsIfNecessary]
    S --> I
    I --> M["Mark* methods<br/>(MarkSliceInitialized,<br/>MarkContentsInitialized,<br/>MarkSliceMipLevelInitialized)"]
Loading

Arrows are in call direction (caller to callee).

The reason this exists is GLES-specific: glTexSubImage2D on a level not previously defined by glTexImage2D raises GL_INVALID_OPERATION, so every per-level upload has to track "has this (slice, mip) had its storage allocated yet?" and allocate on first write. Metal and Vulkan allocate the full mip chain upfront at MTLTexture / VkImage creation and do not need any equivalent bookkeeping.

The texture's observable state (size, format, the contents the shader sees) does not change when the bitset is flipped; only the implementation detail of when GL storage was allocated does. That makes this arguably a textbook mutable case (lazy init / cache). But the pattern is also load-bearing for surprisingly many callers, and the const declarations are doing real work to keep the rest of the renderer compiling against const TextureGLES&.

Flagged during code review on #186302 (review comment). The cleanup is deliberately out of scope for that bug-fix PR.

Proposal

Explore which of the following options (or some combination) best fits the GLES backend, and document the chosen direction:

A. De-const the whole mutation chain. Drop const from Mark*, InitializeContentsIfNecessary, Bind(), SetAsFramebufferAttachment(...), and the renderer callers that hold const TextureGLES&. Remove mutable from slice_mip_initialized_ (and fence_). The type system reflects reality. Cost: a GLES-driver workaround propagates into the type signatures of code paths that conceptually treat textures as read-only, including code paths shared with backends that have no such constraint.

B. Percolate mutable upward (suggested by @gaaclarke). Rather than de-const the chain, push the mutable-ness up so the relationship between texture and its init bookkeeping is more honest. One concrete shape: hold the init bitset (and fence_) in a separate TextureGLESMutableState sub-object that the TextureGLES exposes via an accessor returning a mutable-friendly reference. The bookkeeping is then visibly the only mutating thing, and Bind()/Init/Mark* can be cleanly factored. Needs a sketch to know if it actually reads better than the status quo.

C. Separate texture handle from init bookkeeping. More invasive: split TextureGLES into two objects, where the texture handle is fully immutable after construction and the bookkeeping has its own lifetime/owner. Likely overkill for the scope of this bug, but the most architecturally clean.

D. Accept the pattern as a deliberate use of mutable. Document inline that the mutable bitset is the canonical lazy-allocation / cache case (per the C++ standard's allowance) and that the methods are intentionally const-with-mutable-impl. Add a comment block on slice_mip_initialized_ explaining why this is mutable and why the const is justified, so future contributors do not re-litigate it. No code changes.

The right answer may not be a pure pick: it could be a combination (e.g. D for slice_mip_initialized_ with the rationale documented, plus a narrower cleanup of methods that mutate observable state).

Out of scope for this issue: the corresponding patterns (if any) in the Metal and Vulkan backends; those can be addressed in their own issues.

History

Originally filed as a specific de-const refactor proposal (option A above) during PR #186302 review. After further discussion the framing was widened to an exploration, since it became clear that the right direction was not obvious. The original specific-refactor framing is preserved as option A.

Metadata

Metadata

Assignees

No one assigned

    Labels

    c: tech-debtTechnical debt, code quality, testing, etc.e: impellerImpeller rendering backend issues and features requestsengineflutter/engine related. See also e: labels.

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions