From 10e206867af2179dba718e289aa814333d83301d Mon Sep 17 00:00:00 2001 From: Colin Cornaby Date: Sun, 5 Nov 2023 16:30:57 -0800 Subject: [PATCH] Work on sanitizing use of sizeof and buffer sizes --- .../pfMetalPipeline/plMetalDevice.cpp | 105 ++++++++++++++---- .../pfMetalPipeline/plMetalFragmentShader.cpp | 2 +- .../pfMetalPipeline/plMetalPipeline.cpp | 23 ++-- .../pfMetalPipeline/plMetalPipelineState.cpp | 8 +- .../pfMetalPipeline/plMetalPlateManager.cpp | 6 +- .../pfMetalPipeline/plMetalVertexShader.cpp | 2 +- 6 files changed, 104 insertions(+), 42 deletions(-) diff --git a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalDevice.cpp b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalDevice.cpp index 94a4741f3a..ce452f2eef 100644 --- a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalDevice.cpp +++ b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalDevice.cpp @@ -60,12 +60,77 @@ You can contact Cyan Worlds, Inc. by email legal@cyan.com #include "pfMetalPipeline/plMetalPipelineState.h" #include "pfMetalPipeline/ShaderSrc/ShaderTypes.h" +/// Macros for getting/setting data in a vertex buffer +template +static inline void inlCopy(uint8_t*& src, uint8_t*& dst) +{ + T* src_ptr = reinterpret_cast(src); + T* dst_ptr = reinterpret_cast(dst); + *dst_ptr = *src_ptr; + src += sizeof(T); + dst += sizeof(T); +} + +static inline void inlCopy(const uint8_t*& src, uint8_t*& dst, size_t sz) +{ + memcpy(dst, src, sz); + src += sz; + dst += sz; +} + +template +static inline const uint8_t* inlExtract(const uint8_t* src, T* val) +{ + const T* ptr = reinterpret_cast(src); + *val = *ptr++; + return reinterpret_cast(ptr); +} + +template<> +inline const uint8_t* inlExtract(const uint8_t* src, hsPoint3* val) +{ + const float* src_ptr = reinterpret_cast(src); + float* dst_ptr = reinterpret_cast(val); + *dst_ptr++ = *src_ptr++; + *dst_ptr++ = *src_ptr++; + *dst_ptr++ = *src_ptr++; + *dst_ptr = 1.f; + return reinterpret_cast(src_ptr); +} + +template<> +inline const uint8_t* inlExtract(const uint8_t* src, hsVector3* val) +{ + const float* src_ptr = reinterpret_cast(src); + float* dst_ptr = reinterpret_cast(val); + *dst_ptr++ = *src_ptr++; + *dst_ptr++ = *src_ptr++; + *dst_ptr++ = *src_ptr++; + *dst_ptr = 0.f; + return reinterpret_cast(src_ptr); +} + +template +static inline void inlSkip(uint8_t*& src) +{ + src += sizeof(T) * N; +} + +template +static inline uint8_t* inlStuff(uint8_t* dst, const T* val) +{ + T* ptr = reinterpret_cast(dst); + *ptr++ = *val; + return reinterpret_cast(ptr); +} + matrix_float4x4* hsMatrix2SIMD(const hsMatrix44& src, matrix_float4x4* dst) { + constexpr auto matrixSize = sizeof(matrix_float4x4); if (src.fFlags & hsMatrix44::kIsIdent) { - memcpy(dst, &matrix_identity_float4x4, sizeof(float) * 16); + memcpy(dst, &matrix_identity_float4x4, matrixSize); } else { - memcpy(dst, &src.fMap, sizeof(matrix_float4x4)); + memcpy(dst, &src.fMap, matrixSize); } return dst; @@ -170,11 +235,11 @@ void plMetalDevice::Clear(bool shouldClearColor, simd_float4 clearColor, bool sh // we're actually drawing to screen. if (fCurrentRenderTargetCommandEncoder) { - half4 halfClearColor; - halfClearColor[0] = clearColor.r; - halfClearColor[1] = clearColor.g; - halfClearColor[2] = clearColor.b; - halfClearColor[3] = clearColor.a; + half4 clearColor; + clearColor[0] = clearColor.r; + clearColor[1] = clearColor.g; + clearColor[2] = clearColor.b; + clearColor[3] = clearColor.a; plMetalDevice::plMetalLinkedPipeline* linkedPipeline = plMetalClearPipelineState(this, shouldClearColor, shouldClearDepth).GetRenderPipelineState(); const MTL::RenderPipelineState* pipelineState = linkedPipeline->pipelineState; @@ -190,7 +255,7 @@ void plMetalDevice::Clear(bool shouldClearColor, simd_float4 clearColor, bool sh CurrentRenderCommandEncoder()->setCullMode(MTL::CullModeNone); CurrentRenderCommandEncoder()->setVertexBytes(&clearCoords, sizeof(clearCoords), 0); - CurrentRenderCommandEncoder()->setFragmentBytes(&halfClearColor, sizeof(halfClearColor), 0); + CurrentRenderCommandEncoder()->setFragmentBytes(&clearColor, sizeof(clearColor), 0); CurrentRenderCommandEncoder()->setFragmentBytes(&clearDepth, sizeof(float), 1); CurrentRenderCommandEncoder()->drawPrimitives(MTL::PrimitiveTypeTriangleStrip, NS::UInteger(0), NS::UInteger(4)); } else { @@ -434,7 +499,7 @@ bool plMetalDevice::BeginRender() static uint32_t IGetBufferFormatSize(uint8_t format) { - uint32_t size = sizeof(float) * 6 + sizeof(uint32_t) * 2; // Position and normal, and two packed colors + uint32_t size = sizeof(hsPoint3) * 2 + sizeof(hsColor32) * 2; // Position and normal, and two packed colors switch (format & plGBufferGroup::kSkinWeightMask) { case plGBufferGroup::kSkinNoWeights: @@ -446,7 +511,7 @@ static uint32_t IGetBufferFormatSize(uint8_t format) hsAssert(false, "Invalid skin weight value in IGetBufferFormatSize()"); } - size += sizeof(float) * 3 * plGBufferGroup::CalcNumUVs(format); + size += sizeof(hsPoint3) * plGBufferGroup::CalcNumUVs(format); return size; } @@ -579,26 +644,20 @@ void plMetalDevice::FillVolatileVertexBufferRef(plMetalDevice::VertexBufferRef* uint8_t* dst = ref->fData; uint8_t* src = group->GetVertBufferData(idx); - size_t uvChanSize = plGBufferGroup::CalcNumUVs(group->GetVertexFormat()) * sizeof(float) * 3; + size_t uvChanSize = plGBufferGroup::CalcNumUVs(group->GetVertexFormat()) * sizeof(hsPoint3); uint8_t numWeights = (group->GetVertexFormat() & plGBufferGroup::kSkinWeightMask) >> 4; for (uint32_t i = 0; i < ref->fCount; ++i) { - memcpy(dst, src, sizeof(hsPoint3)); // pre-pos - dst += sizeof(hsPoint3); - src += sizeof(hsPoint3); + inlCopy(src, dst); // pre-pos src += numWeights * sizeof(float); // weights if (group->GetVertexFormat() & plGBufferGroup::kSkinIndices) - src += sizeof(uint32_t); // indices - - memcpy(dst, src, sizeof(hsVector3)); // pre-normal - dst += sizeof(hsVector3); - src += sizeof(hsVector3); - - memcpy(dst, src, sizeof(uint32_t) * 2); // diffuse & specular - dst += sizeof(uint32_t) * 2; - src += sizeof(uint32_t) * 2; + inlSkip(src); // indices + + inlCopy(src, dst); // pre-normal + inlCopy(src, dst); // diffuse + inlCopy(src, dst); // specular // UVWs memcpy(dst, src, uvChanSize); diff --git a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalFragmentShader.cpp b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalFragmentShader.cpp index 8b0c886033..8daad2b76e 100644 --- a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalFragmentShader.cpp +++ b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalFragmentShader.cpp @@ -68,7 +68,7 @@ bool plMetalFragmentShader::ISetConstants(plMetalPipeline* pipe) { if (fOwner->GetNumConsts()) { float* ptr = (float*)fOwner->GetConstBasePtr(); - pipe->GetMetalDevice()->CurrentRenderCommandEncoder()->setFragmentBytes(ptr, fOwner->GetNumConsts() * sizeof(float) * 4, VertexShaderArgumentMaterialShaderUniforms); + pipe->GetMetalDevice()->CurrentRenderCommandEncoder()->setFragmentBytes(ptr, fOwner->GetNumConsts() * sizeof(simd_float4), VertexShaderArgumentMaterialShaderUniforms); } return true; diff --git a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.cpp b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.cpp index e1d9664ff7..2c4ca4f66c 100644 --- a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.cpp +++ b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipeline.cpp @@ -864,17 +864,19 @@ bool plMetalPipeline::SetGamma(const uint16_t* const tabR, const uint16_t* const per channel data. The Metal renderer supports up to 10 bit colors - but it can subsample the texture to interpolate the colors in between what the LUT defines. */ + constexpr size_t numLuts = 256; + MTL::TextureDescriptor* texDescriptor = MTL::TextureDescriptor::alloc()->init()->autorelease(); texDescriptor->setTextureType(MTL::TextureType1DArray); - texDescriptor->setWidth(256); + texDescriptor->setWidth(numLuts); texDescriptor->setPixelFormat(MTL::PixelFormatR16Uint); texDescriptor->setArrayLength(3); fDevice.fGammaLUTTexture = fDevice.fMetalDevice->newTexture(texDescriptor); - fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, 256), 0, 0, tabR, 256 * sizeof(uint16_t), 0); - fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, 256), 0, 1, tabG, 256 * sizeof(uint16_t), 0); - fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, 256), 0, 2, tabB, 256 * sizeof(uint16_t), 0); + fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, numLuts), 0, 0, tabR, 0, 0); + fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, numLuts), 0, 1, tabG, 0, 0); + fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, numLuts), 0, 2, tabB, 0, 0); return true; } @@ -893,18 +895,19 @@ bool plMetalPipeline::SetGamma10(const uint16_t* const tabR, const uint16_t* con by normalized co-ordinate - not value. So the width of the texture can vary. */ + constexpr size_t numLuts = 1024; MTL::TextureDescriptor* texDescriptor = MTL::TextureDescriptor::alloc()->init()->autorelease(); texDescriptor->setTextureType(MTL::TextureType1DArray); - texDescriptor->setWidth(1024); + texDescriptor->setWidth(numLuts); texDescriptor->setPixelFormat(MTL::PixelFormatR16Uint); texDescriptor->setArrayLength(3); fDevice.fGammaLUTTexture = fDevice.fMetalDevice->newTexture(texDescriptor); - fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, 1024), 0, 0, tabR, 1024 * sizeof(uint16_t), 0); - fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, 1024), 0, 1, tabG, 1024 * sizeof(uint16_t), 0); - fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, 1024), 0, 2, tabB, 1024 * sizeof(uint16_t), 0); + fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, numLuts), 0, 0, tabR, 0, 0); + fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, numLuts), 0, 1, tabG, 0, 0); + fDevice.fGammaLUTTexture->replaceRegion(MTL::Region(0, numLuts), 0, 2, tabB, 0, 0); return true; } @@ -2751,9 +2754,9 @@ void plMetalPipeline::IPreprocessAvatarTextures() vertexDescriptor->attributes()->object(0)->setOffset(0); vertexDescriptor->attributes()->object(1)->setFormat(MTL::VertexFormatFloat2); vertexDescriptor->attributes()->object(1)->setBufferIndex(0); - vertexDescriptor->attributes()->object(1)->setOffset(sizeof(float) * 2); + vertexDescriptor->attributes()->object(1)->setOffset(sizeof(simd_float2)); - vertexDescriptor->layouts()->object(0)->setStride(sizeof(float) * 4); + vertexDescriptor->layouts()->object(0)->setStride(sizeof(simd_float2) * 2); descriptor->setVertexDescriptor(vertexDescriptor); diff --git a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipelineState.cpp b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipelineState.cpp index 4f511dc026..9cd719ad22 100644 --- a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipelineState.cpp +++ b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPipelineState.cpp @@ -123,14 +123,14 @@ size_t plMetalMaterialPassPipelineState::GetHash() const void plMetalRenderSpanPipelineState::ConfigureVertexDescriptor(MTL::VertexDescriptor* vertexDescriptor) { int vertOffset = 0; - int skinWeightOffset = vertOffset + (sizeof(float) * 3); + int skinWeightOffset = vertOffset + sizeof(hsPoint3); if (fHasSkinIndices) { skinWeightOffset += sizeof(uint32_t); } int normOffset = skinWeightOffset + (sizeof(float) * this->fNumWeights); - int colorOffset = normOffset + (sizeof(float) * 3); + int colorOffset = normOffset + sizeof(hsPoint3); int baseUvOffset = colorOffset + (sizeof(uint32_t) * 2); - int stride = baseUvOffset + (sizeof(float) * 3 * this->fNumUVs); + int stride = baseUvOffset + sizeof(hsPoint3) * this->fNumUVs; vertexDescriptor->attributes()->object(VertexAttributePosition)->setFormat(MTL::VertexFormatFloat3); vertexDescriptor->attributes()->object(VertexAttributePosition)->setBufferIndex(0); @@ -151,7 +151,7 @@ void plMetalRenderSpanPipelineState::ConfigureVertexDescriptor(MTL::VertexDescri for (int i = 0; i < this->fNumUVs; i++) { vertexDescriptor->attributes()->object(VertexAttributeTexcoord + i)->setFormat(MTL::VertexFormatFloat3); vertexDescriptor->attributes()->object(VertexAttributeTexcoord + i)->setBufferIndex(0); - vertexDescriptor->attributes()->object(VertexAttributeTexcoord + i)->setOffset(baseUvOffset + (i * sizeof(float) * 3)); + vertexDescriptor->attributes()->object(VertexAttributeTexcoord + i)->setOffset(baseUvOffset + (i * sizeof(hsPoint3))); } vertexDescriptor->attributes()->object(VertexAttributeColor)->setFormat(MTL::VertexFormatUChar4); diff --git a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPlateManager.cpp b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPlateManager.cpp index 4e927dc8b5..82db658b66 100644 --- a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPlateManager.cpp +++ b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalPlateManager.cpp @@ -79,7 +79,7 @@ void plMetalPlateManager::ICreateGeometry() fVtxBuffer = pipeline->fDevice.fMetalDevice->newBuffer(&vertexBuffer, sizeof(plateVertexBuffer), MTL::StorageModeManaged); fVtxBuffer->retain(); - idxBuffer = pipeline->fDevice.fMetalDevice->newBuffer(&indices, sizeof(uint16_t) * 6, MTL::StorageModeManaged); + idxBuffer = pipeline->fDevice.fMetalDevice->newBuffer(&indices, sizeof(indices), MTL::StorageModeManaged); } } @@ -157,8 +157,8 @@ void plMetalPlatePipelineState::ConfigureVertexDescriptor(MTL::VertexDescriptor* vertexDescriptor->attributes()->object(1)->setBufferIndex(VertexAttributeTexcoord); vertexDescriptor->attributes()->object(1)->setOffset(0); - vertexDescriptor->layouts()->object(0)->setStride(sizeof(float) * 2); - vertexDescriptor->layouts()->object(1)->setStride(sizeof(float) * 2); + vertexDescriptor->layouts()->object(0)->setStride(sizeof(simd_float2)); + vertexDescriptor->layouts()->object(1)->setStride(sizeof(simd_float2)); } void plMetalPlatePipelineState::GetFunctionConstants(MTL::FunctionConstantValues*) const diff --git a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalVertexShader.cpp b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalVertexShader.cpp index 7aeedf0f3b..f2303bf89b 100644 --- a/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalVertexShader.cpp +++ b/Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalVertexShader.cpp @@ -68,7 +68,7 @@ bool plMetalVertexShader::ISetConstants(plMetalPipeline* pipe) { if (fOwner->GetNumConsts()) { float* ptr = (float*)fOwner->GetConstBasePtr(); - pipe->GetMetalDevice()->CurrentRenderCommandEncoder()->setVertexBytes(ptr, fOwner->GetNumConsts() * sizeof(float) * 4, VertexShaderArgumentMaterialShaderUniforms); + pipe->GetMetalDevice()->CurrentRenderCommandEncoder()->setVertexBytes(ptr, fOwner->GetNumConsts() * sizeof(simd_float4), VertexShaderArgumentMaterialShaderUniforms); } return true;