From b6d52b4ad1b27e94ed061cd7e8baee479a361c0c Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Mon, 26 Jun 2017 11:43:51 -0400 Subject: [PATCH] Refactor copy validation to add helper functions useful for T->B too. Also fixes validation not taking the mip-level into account when checking if the copy would fit in the texture. --- src/backend/common/CommandBuffer.cpp | 140 ++++++++++++++----------- src/backend/common/Commands.h | 24 +++-- src/backend/metal/CommandBufferMTL.mm | 36 ++++--- src/backend/opengl/CommandBufferGL.cpp | 18 ++-- 4 files changed, 122 insertions(+), 96 deletions(-) diff --git a/src/backend/common/CommandBuffer.cpp b/src/backend/common/CommandBuffer.cpp index 121b4da520..d21eeca135 100644 --- a/src/backend/common/CommandBuffer.cpp +++ b/src/backend/common/CommandBuffer.cpp @@ -29,6 +29,56 @@ namespace backend { + namespace { + + bool ValidateCopyLocationFitsInTexture(CommandBufferBuilder* builder, const TextureCopyLocation& location) { + const TextureBase* texture = location.texture.Get(); + if (location.level >= texture->GetNumMipLevels()) { + builder->HandleError("Copy mip-level out of range"); + return false; + } + + // All texture dimensions are in uint32_t so by doing checks in uint64_t we avoid overflows. + uint64_t level = location.level; + if (uint64_t(location.x) + uint64_t(location.width) > (static_cast(texture->GetWidth()) >> level) || + uint64_t(location.y) + uint64_t(location.height) > (static_cast(texture->GetHeight()) >> level)) { + builder->HandleError("Copy would touch outside of the texture"); + return false; + } + + // TODO(cwallez@chromium.org): Check the depth bound differently for 2D arrays and 3D textures + if (location.z != 0 || location.depth != 1) { + builder->HandleError("No support for z != 0 and depth != 1 for now"); + return false; + } + + return true; + } + + bool FitsInBuffer(const BufferBase* buffer, uint32_t offset, uint32_t size) { + uint32_t bufferSize = buffer->GetSize(); + return offset <= bufferSize && (size <= (bufferSize - offset)); + } + + bool ValidateCopySizeFitsInBuffer(CommandBufferBuilder* builder, const BufferCopyLocation& location, uint32_t dataSize) { + if (!FitsInBuffer(location.buffer.Get(), location.offset, dataSize)) { + builder->HandleError("Copy would overflow the buffer"); + return false; + } + + return true; + } + + bool ComputeTextureCopyBufferSize(CommandBufferBuilder* builder, const TextureCopyLocation& location, uint32_t* bufferSize) { + // TODO(cwallez@chromium.org): check for overflows + uint32_t pixelSize = TextureFormatPixelSize(location.texture->GetFormat()); + *bufferSize = location.width * location.height * location.depth * pixelSize; + + return true; + } + + } + CommandBufferBase::CommandBufferBase(CommandBufferBuilder* builder) : device(builder->device), buffersTransitioned(std::move(builder->state->buffersTransitioned)), @@ -210,27 +260,11 @@ namespace backend { case Command::CopyBufferToBuffer: { CopyBufferToBufferCmd* copy = iterator.NextCommand(); - BufferBase* source = copy->source.Get(); - BufferBase* destination = copy->destination.Get(); - uint32_t sourceOffset = copy->sourceOffset; - uint32_t destinationOffset = copy->destinationOffset; - uint32_t size = copy->size; - - uint64_t sourceEnd = uint64_t(sourceOffset) + uint64_t(size); - if (sourceEnd > uint64_t(source->GetSize())) { - HandleError("Copy would read after end of the source buffer"); - return false; - } - - uint64_t destinationEnd = uint64_t(destinationOffset) + uint64_t(size); - if (destinationEnd > uint64_t(destination->GetSize())) { - HandleError("Copy would read after end of the destination buffer"); - return false; - } - - if (!state->ValidateCanCopy() || - !state->ValidateCanUseBufferAs(source, nxt::BufferUsageBit::TransferSrc) || - !state->ValidateCanUseBufferAs(destination, nxt::BufferUsageBit::TransferDst)) { + if (!ValidateCopySizeFitsInBuffer(this, copy->source, copy->size) || + !ValidateCopySizeFitsInBuffer(this, copy->destination, copy->size) || + !state->ValidateCanCopy() || + !state->ValidateCanUseBufferAs(copy->source.buffer.Get(), nxt::BufferUsageBit::TransferSrc) || + !state->ValidateCanUseBufferAs(copy->destination.buffer.Get(), nxt::BufferUsageBit::TransferDst)) { return false; } } @@ -239,36 +273,14 @@ namespace backend { case Command::CopyBufferToTexture: { CopyBufferToTextureCmd* copy = iterator.NextCommand(); - BufferBase* buffer = copy->buffer.Get(); - uint32_t bufferOffset = copy->bufferOffset; - TextureBase* texture = copy->texture.Get(); - uint64_t width = copy->width; - uint64_t height = copy->height; - uint64_t depth = copy->depth; - uint64_t x = copy->x; - uint64_t y = copy->y; - uint64_t z = copy->z; - uint32_t level = copy->level; - // TODO(cwallez@chromium.org): check for overflows - uint64_t pixelSize = TextureFormatPixelSize(texture->GetFormat()); - uint64_t dataSize = width * height * depth * pixelSize; - if (dataSize + static_cast(bufferOffset) > static_cast(buffer->GetSize())) { - HandleError("Copy would read after end of the buffer"); - return false; - } - - if (x + width > static_cast(texture->GetWidth()) || - y + height > static_cast(texture->GetHeight()) || - z + depth > static_cast(texture->GetDepth()) || - level > texture->GetNumMipLevels()) { - HandleError("Copy would write outside of the texture"); - return false; - } - - if (!state->ValidateCanCopy() || - !state->ValidateCanUseBufferAs(buffer, nxt::BufferUsageBit::TransferSrc) || - !state->ValidateCanUseTextureAs(texture, nxt::TextureUsageBit::TransferDst)) { + uint32_t bufferCopySize = 0; + if (!ComputeTextureCopyBufferSize(this, copy->destination, &bufferCopySize) || + !ValidateCopyLocationFitsInTexture(this, copy->destination) || + !ValidateCopySizeFitsInBuffer(this, copy->source, bufferCopySize) || + !state->ValidateCanCopy() || + !state->ValidateCanUseBufferAs(copy->source.buffer.Get(), nxt::BufferUsageBit::TransferSrc) || + !state->ValidateCanUseTextureAs(copy->destination.texture.Get(), nxt::TextureUsageBit::TransferDst)) { return false; } } @@ -424,10 +436,10 @@ namespace backend { void CommandBufferBuilder::CopyBufferToBuffer(BufferBase* source, uint32_t sourceOffset, BufferBase* destination, uint32_t destinationOffset, uint32_t size) { CopyBufferToBufferCmd* copy = allocator.Allocate(Command::CopyBufferToBuffer); new(copy) CopyBufferToBufferCmd; - copy->source = source; - copy->sourceOffset = sourceOffset; - copy->destination = destination; - copy->destinationOffset = destinationOffset; + copy->source.buffer = source; + copy->source.offset = sourceOffset; + copy->destination.buffer = destination; + copy->destination.offset = destinationOffset; copy->size = size; } @@ -436,16 +448,16 @@ namespace backend { uint32_t width, uint32_t height, uint32_t depth, uint32_t level) { CopyBufferToTextureCmd* copy = allocator.Allocate(Command::CopyBufferToTexture); new(copy) CopyBufferToTextureCmd; - copy->buffer = buffer; - copy->bufferOffset = bufferOffset; - copy->texture = texture; - copy->x = x; - copy->y = y; - copy->z = z; - copy->width = width; - copy->height = height; - copy->depth = depth; - copy->level = level; + copy->source.buffer = buffer; + copy->source.offset = bufferOffset; + copy->destination.texture = texture; + copy->destination.x = x; + copy->destination.y = y; + copy->destination.z = z; + copy->destination.width = width; + copy->destination.height = height; + copy->destination.depth = depth; + copy->destination.level = level; } void CommandBufferBuilder::Dispatch(uint32_t x, uint32_t y, uint32_t z) { diff --git a/src/backend/common/Commands.h b/src/backend/common/Commands.h index d1b1ecb736..9809412b4c 100644 --- a/src/backend/common/Commands.h +++ b/src/backend/common/Commands.h @@ -54,23 +54,29 @@ namespace backend { Ref framebuffer; }; - struct CopyBufferToBufferCmd { - Ref source; - Ref destination; - uint32_t sourceOffset; - uint32_t destinationOffset; - uint32_t size; + struct BufferCopyLocation { + Ref buffer; + uint32_t offset; }; - struct CopyBufferToTextureCmd { - Ref buffer; - uint32_t bufferOffset; + struct TextureCopyLocation { Ref texture; uint32_t x, y, z; uint32_t width, height, depth; uint32_t level; }; + struct CopyBufferToBufferCmd { + BufferCopyLocation source; + BufferCopyLocation destination; + uint32_t size; + }; + + struct CopyBufferToTextureCmd { + BufferCopyLocation source; + TextureCopyLocation destination; + }; + struct DispatchCmd { uint32_t x; uint32_t y; diff --git a/src/backend/metal/CommandBufferMTL.mm b/src/backend/metal/CommandBufferMTL.mm index 41b74b72fb..77c45569f3 100644 --- a/src/backend/metal/CommandBufferMTL.mm +++ b/src/backend/metal/CommandBufferMTL.mm @@ -163,13 +163,15 @@ namespace metal { case Command::CopyBufferToBuffer: { CopyBufferToBufferCmd* copy = commands.NextCommand(); + auto& src = copy->source; + auto& dst = copy->destination; encoders.EnsureBlit(commandBuffer); [encoders.blit - copyFromBuffer:ToBackend(copy->source)->GetMTLBuffer() - sourceOffset:copy->sourceOffset - toBuffer:ToBackend(copy->destination)->GetMTLBuffer() - destinationOffset:copy->destinationOffset + copyFromBuffer:ToBackend(src.buffer)->GetMTLBuffer() + sourceOffset:src.offset + toBuffer:ToBackend(dst.buffer)->GetMTLBuffer() + destinationOffset:dst.offset size:copy->size]; } break; @@ -177,30 +179,32 @@ namespace metal { case Command::CopyBufferToTexture: { CopyBufferToTextureCmd* copy = commands.NextCommand(); - Buffer* buffer = ToBackend(copy->buffer.Get()); - Texture* texture = ToBackend(copy->texture.Get()); + auto& src = copy->source; + auto& dst = copy->destination; + Buffer* buffer = ToBackend(src.buffer.Get()); + Texture* texture = ToBackend(dst.texture.Get()); - unsigned rowSize = copy->width * TextureFormatPixelSize(texture->GetFormat()); + unsigned rowSize = dst.width * TextureFormatPixelSize(texture->GetFormat()); MTLOrigin origin; - origin.x = copy->x; - origin.y = copy->y; - origin.z = copy->z; + origin.x = dst.x; + origin.y = dst.y; + origin.z = dst.z; MTLSize size; - size.width = copy->width; - size.height = copy->height; - size.depth = copy->depth; + size.width = dst.width; + size.height = dst.height; + size.depth = dst.depth; encoders.EnsureBlit(commandBuffer); [encoders.blit copyFromBuffer:buffer->GetMTLBuffer() - sourceOffset:copy->bufferOffset + sourceOffset:src.offset sourceBytesPerRow:rowSize - sourceBytesPerImage:(rowSize * copy->height) + sourceBytesPerImage:(rowSize * dst.height) sourceSize:size toTexture:texture->GetMTLTexture() destinationSlice:0 - destinationLevel:copy->level + destinationLevel:dst.level destinationOrigin:origin]; } break; diff --git a/src/backend/opengl/CommandBufferGL.cpp b/src/backend/opengl/CommandBufferGL.cpp index 1ec2daaf91..d258cf61c2 100644 --- a/src/backend/opengl/CommandBufferGL.cpp +++ b/src/backend/opengl/CommandBufferGL.cpp @@ -81,10 +81,12 @@ namespace opengl { case Command::CopyBufferToBuffer: { CopyBufferToBufferCmd* copy = commands.NextCommand(); + auto& src = copy->source; + auto& dst = copy->destination; - glBindBuffer(GL_PIXEL_PACK_BUFFER, ToBackend(copy->source)->GetHandle()); - glBindBuffer(GL_PIXEL_UNPACK_BUFFER, ToBackend(copy->destination)->GetHandle()); - glCopyBufferSubData(GL_PIXEL_PACK_BUFFER, GL_PIXEL_UNPACK_BUFFER, copy->sourceOffset, copy->destinationOffset, copy->size); + glBindBuffer(GL_PIXEL_PACK_BUFFER, ToBackend(src.buffer)->GetHandle()); + glBindBuffer(GL_PIXEL_UNPACK_BUFFER, ToBackend(dst.buffer)->GetHandle()); + glCopyBufferSubData(GL_PIXEL_PACK_BUFFER, GL_PIXEL_UNPACK_BUFFER, src.offset, dst.offset, copy->size); glBindBuffer(GL_PIXEL_PACK_BUFFER, 0); glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); @@ -94,8 +96,10 @@ namespace opengl { case Command::CopyBufferToTexture: { CopyBufferToTextureCmd* copy = commands.NextCommand(); - Buffer* buffer = ToBackend(copy->buffer.Get()); - Texture* texture = ToBackend(copy->texture.Get()); + auto& src = copy->source; + auto& dst = copy->destination; + Buffer* buffer = ToBackend(src.buffer.Get()); + Texture* texture = ToBackend(dst.texture.Get()); GLenum target = texture->GetGLTarget(); auto format = texture->GetGLFormat(); @@ -103,9 +107,9 @@ namespace opengl { glActiveTexture(GL_TEXTURE0); glBindTexture(target, texture->GetHandle()); - glTexSubImage2D(target, copy->level, copy->x, copy->y, copy->width, copy->height, + glTexSubImage2D(target, dst.level, dst.x, dst.y, dst.width, dst.height, format.format, format.type, - reinterpret_cast(static_cast(copy->bufferOffset))); + reinterpret_cast(static_cast(src.offset))); glBindBuffer(GL_PIXEL_UNPACK_BUFFER, 0); } break;