You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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_):
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.
Use case
engine/src/flutter/impeller/renderer/backend/gles/texture_gles.{h,cc}has a long-standingconst-on-mutation pattern in its initialization tracking. The following methods are declaredconstbut mutate internal state via amutablemember (slice_mip_initialized_):TextureGLES::MarkSliceInitialized(size_t slice) constTextureGLES::MarkContentsInitialized() constTextureGLES::MarkSliceMipLevelInitialized(size_t slice, size_t mip_level) constTextureGLES::InitializeContentsIfNecessary() constThese methods are called from other
constmethods onTextureGLES(Bind(),SetAsFramebufferAttachment(...)), which in turn are called from render-pass and framebuffer code paths that holdconst 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)"]Arrows are in call direction (caller to callee).
The reason this exists is GLES-specific:
glTexSubImage2Don a level not previously defined byglTexImage2DraisesGL_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 atMTLTexture/VkImagecreation 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
mutablecase (lazy init / cache). But the pattern is also load-bearing for surprisingly many callers, and theconstdeclarations are doing real work to keep the rest of the renderer compiling againstconst 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
constfromMark*,InitializeContentsIfNecessary,Bind(),SetAsFramebufferAttachment(...), and the renderer callers that holdconst TextureGLES&. Removemutablefromslice_mip_initialized_(andfence_). 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
mutableupward (suggested by @gaaclarke). Rather than de-const the chain, push themutable-ness up so the relationship between texture and its init bookkeeping is more honest. One concrete shape: hold the init bitset (andfence_) in a separateTextureGLESMutableStatesub-object that theTextureGLESexposes via an accessor returning amutable-friendly reference. The bookkeeping is then visibly the only mutating thing, andBind()/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
TextureGLESinto 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 themutablebitset is the canonical lazy-allocation / cache case (per the C++ standard's allowance) and that the methods are intentionallyconst-with-mutable-impl. Add a comment block onslice_mip_initialized_explaining why this ismutableand why theconstis 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.