[Fix] (Readback): size a cube-face pack buffer by one face, and give a cube view's face its owner layer

MobileGL/MG_Impl/GLImpl/Texture/GL_Texture.cpp

@@ -5078,6 +5078,13 @@ namespace MobileGL::MG_Impl::GLImpl {
     // The half of the GetTexImage/GetTextureImage error set (GL 4.6 core 8.11) that depends on the
     // resolved texture object rather than on how it was named. Shared because the by-name entry
     // point does not route through GetTexImage_State and so used to enforce none of it.
+    // A cube map's six faces are six independent images, and both readback spellings name one of
+    // them: glGetTexImage through the TARGET token, glGetTextureSubImage through zoffset. Both then
+    // have to tell the size checks below that ONE image is coming back, not six.
+    static Bool IsCubeMapFaceUploadTarget(TextureUploadTarget target) {
+        return target >= TextureUploadTarget::CubeMapPositiveX && target <= TextureUploadTarget::CubeMapNegativeZ;
+    }
+
     // `imagesQueried` is how many of the texture's upload-target images the query hands back, and
     // exists for the destination-size check at the bottom. Zero means "all of them", which is what
     // the whole-level forms return - every face of a cube map. glGetTextureSubImage naming ONE cube
@@ -5282,9 +5289,14 @@ namespace MobileGL::MG_Impl::GLImpl {
             isProxy ? TextureImpl::pProxyTextureManager->GetProxyTextureObject(textureUploadTarget)
                     : bindingSlot.GetBoundObject();
 
-        // glGetTexImage has no bufSize argument: -1 stands for "no client-side limit".
+        // glGetTexImage has no bufSize argument: -1 stands for "no client-side limit". That skips
+        // the destination-size branch but NOT the pixel-pack-buffer one, which measures the same
+        // `required` against the bound PBO's real size - so a cube FACE query has to say it returns
+        // one image here too, or a PBO sized for the one face this call packs is refused as too
+        // small while the copy that follows writes exactly that much into it.
         return ValidateTextureImageQuery(textureObject, level, textureInputFormat, texturePixelDataType, -1, pixels,
-                                         "GetTexImage_State");
+                                         "GetTexImage_State",
+                                         IsCubeMapFaceUploadTarget(textureUploadTarget) ? 1u : 0u);
     }
 
     // What this helper can and cannot answer.

MobileGL/MG_State/GLState/TextureState/TextureObjectView.cpp

@@ -97,27 +97,31 @@ namespace MobileGL::MG_State::GLState {
         return m_storageOwner->HasFixedSampleLocations();
     }
 
+    Uint TextureObjectView::ViewLayerIndex(TextureUploadTarget viewTarget) const {
+        if (GetTarget() != TextureTarget::TextureCubeMap) {
+            // One target, one layer: the view's origin is the whole answer.
+            return m_viewMinLayer;
+        }
+        for (Uint i = 0; i < static_cast<Uint>(m_uploadTargets.size()); ++i) {
+            if (m_uploadTargets[i] == viewTarget) return m_viewMinLayer + i;
+        }
+        return m_viewMinLayer;
+    }
+
     TextureUploadTarget TextureObjectView::ToOwnerUploadTarget(TextureUploadTarget viewTarget) const {
         const auto& ownerTargets = m_storageOwner->GetUploadTargets();
         MOBILEGL_ASSERT(!ownerTargets.empty(), "TextureObjectView: storage owner has no upload target");
         if (ownerTargets.size() == 1) {
-            // The owner keeps every layer in one blob, so there is nothing to choose.
+            // The owner keeps every layer in one blob, so there is nothing to choose HERE - which
+            // is exactly why a cube-map view over such an owner has to have its face carried by
+            // LayerByteOffset instead. See ViewLayerIndex.
             return ownerTargets[0];
         }
-        // The owner is a cube map: six independent blobs, one per face, and the view's layer
+        // The owner is a cube map: six independent blobs, one per face, and the layer this view
-        // index selects among them. A cube-map view of a cube map maps face to face; any other
+        // target names selects among them. A cube-map view of a cube map maps face to face; any
-        // view target addresses layers, which for a cube-map owner ARE its faces.
+        // other view target addresses layers, which for a cube-map owner ARE its faces.
         const Uint faceCount = static_cast<Uint>(ownerTargets.size());
-        Uint face = m_viewMinLayer;
+        return ownerTargets[std::min(ViewLayerIndex(viewTarget), faceCount - 1)];
-        if (GetTarget() == TextureTarget::TextureCubeMap) {
-            for (Uint i = 0; i < m_uploadTargets.size(); ++i) {
-                if (m_uploadTargets[i] == viewTarget) {
-                    face = m_viewMinLayer + i;
-                    break;
-                }
-            }
-        }
-        return ownerTargets[std::min(face, faceCount - 1)];
     }
 
     IntVec3 TextureObjectView::ToViewLevelSize(const IntVec3& ownerLevelSize) const {
@@ -151,7 +155,13 @@ namespace MobileGL::MG_State::GLState {
     }
 
     SizeT TextureObjectView::LayerByteOffset(TextureUploadTarget viewTarget, Uint mipmapLevel) const {
-        if (m_viewMinLayer == 0 || m_ownerMipmap == nullptr) return 0;
+        if (m_ownerMipmap == nullptr) return 0;
+        // The FACE is part of this, not just the view's origin: a cube-map view over a layered
+        // owner (a 2D array or a cube-map ARRAY) has only one blob to address, so the face its
+        // target token names lives here or nowhere. It used to live nowhere, and all six face
+        // tokens read the view's first layer-face - silently, with texels from a real layer.
+        const Uint layerIndex = ViewLayerIndex(viewTarget);
+        if (layerIndex == 0) return 0;
         const LayerAxis ownerAxis = LayerAxisOf(m_storageOwner->GetTarget());
         if (ownerAxis == LayerAxis::None) {
             // A cube-map owner keeps each face in its OWN blob, and ToOwnerUploadTarget already
@@ -173,23 +183,26 @@ namespace MobileGL::MG_State::GLState {
                                       ? static_cast<SizeT>(std::max(ownerSize.x(), 0))
                                       : static_cast<SizeT>(std::max(ownerSize.x(), 0)) *
                                             static_cast<SizeT>(std::max(ownerSize.y(), 0));
-        const SizeT offset = static_cast<SizeT>(m_viewMinLayer) * layerTexels * bytesPerTexel;
+        const SizeT offset = static_cast<SizeT>(layerIndex) * layerTexels * bytesPerTexel;
         return offset < ownerBytes ? offset : 0;
     }
 
-    IntVec3 TextureObjectView::ToOwnerRegionOffset(const IntVec3& viewOffset) const {
+    IntVec3 TextureObjectView::ToOwnerRegionOffset(TextureUploadTarget viewTarget, const IntVec3& viewOffset) const {
-        if (m_viewMinLayer == 0) return viewOffset;
+        const Uint layerIndex = ViewLayerIndex(viewTarget);
+        if (layerIndex == 0) return viewOffset;
         IntVec3 offset = viewOffset;
         // The dirty region is recorded in the OWNER's blob coordinates - that is the space its
-        // upload path walks - so the view's layer origin has to be added here even though
+        // upload path walks - so the layer this view target names has to be added here even though
         // MapMipmapData hands back an already-shifted POINTER. The two are not double-counting:
-        // one moves the bytes, the other tells the owner which of its layers moved.
+        // one moves the bytes, the other tells the owner which of its layers moved. They must agree
+        // on the layer, which is why both ask ViewLayerIndex rather than reading m_viewMinLayer -
+        // on a cube-map view the face is half the answer.
         switch (LayerAxisOf(m_storageOwner->GetTarget())) {
         case LayerAxis::Y:
-            offset.y() += static_cast<Int>(m_viewMinLayer);
+            offset.y() += static_cast<Int>(layerIndex);
             break;
         case LayerAxis::Z:
-            offset.z() += static_cast<Int>(m_viewMinLayer);
+            offset.z() += static_cast<Int>(layerIndex);
             break;
         case LayerAxis::None:
             break;
@@ -300,7 +313,7 @@ namespace MobileGL::MG_State::GLState {
                                                    IntVec3 size) {
         if (m_ownerMipmap == nullptr) return;
         m_ownerMipmap->MarkStorageDirtyRegion(ToOwnerUploadTarget(uploadTarget), ToOwnerLevel(mipmapLevel),
-                                              ToOwnerRegionOffset(offset), size);
+                                              ToOwnerRegionOffset(uploadTarget, offset), size);
     }
 
     MipmapDirtyRegion TextureObjectView::GetStorageDirtyRegion(TextureUploadTarget uploadTarget,

MobileGL/MG_State/GLState/TextureState/TextureObjectView.h

@@ -103,6 +103,18 @@ namespace MobileGL::MG_State::GLState {
         // target - arrays and cube-map arrays included - keeps all its layers in one blob, so
         // the mapping is "the owner's only target" unless one of the two sides is a cube map.
         TextureUploadTarget ToOwnerUploadTarget(TextureUploadTarget viewTarget) const;
+        // WHICH of the owner's layers a given view-side upload target names, in the owner's layer
+        // numbering. For every view target but a cube map that is just this view's layer origin -
+        // one target, one layer. A GL_TEXTURE_CUBE_MAP view addresses SIX of the owner's layers at
+        // once (GL 4.6 core 8.18), so the face its target token names is an index on top of that
+        // origin, and this is the only place that can express it when the owner keeps every layer
+        // in one blob: ToOwnerUploadTarget has a single blob to choose from there, so the face
+        // would otherwise vanish and all six tokens would read the view's first layer.
+        //
+        // Every place that turns this view into owner-side bytes goes through here - the blob
+        // choice, the byte offset, and the dirty region - so the three cannot disagree about which
+        // layer a face is.
+        Uint ViewLayerIndex(TextureUploadTarget viewTarget) const;
         Uint ToOwnerLevel(Uint viewLevel) const { return m_viewMinLevel + viewLevel; }
         // The owner's level extent rewritten into this view's shape: the owner's layer axis is
         // collapsed to one slice and the view's own layer count is imposed on the view's layer
@@ -114,8 +126,10 @@ namespace MobileGL::MG_State::GLState {
         // and a single row for a 1D array; a cube-map owner returns 0 because its faces are
         // separate blobs that ToOwnerUploadTarget already selects between.
         SizeT LayerByteOffset(TextureUploadTarget viewTarget, Uint mipmapLevel) const;
-        // A dirty-region origin moved from the view's layer space into the owner's.
+        // A dirty-region origin moved from the view's layer space into the owner's. Takes the view
-        IntVec3 ToOwnerRegionOffset(const IntVec3& viewOffset) const;
+        // target for the same reason LayerByteOffset does: on a cube-map view the target names the
+        // face, and the region has to name the same owner layer the bytes were written to.
+        IntVec3 ToOwnerRegionOffset(TextureUploadTarget viewTarget, const IntVec3& viewOffset) const;
 
         SharedPtr<ITextureObject> m_storageOwner;
         // Non-owning; m_storageOwner keeps it alive and is never a view, so this is set once in