diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 39b9534cd4..7bf5edb8ee 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,6 +7,7 @@ A new header is inserted each time a *tag* is created. - gltfio: support skinning with bones that do not belong to any scene - gltfio: add attachSkin / detachSkin method to FilamentAsset +- Metal: fix issues seen with dynamic resolution on M1 Macs - engine: add a "global" mode for render primitive's `blendOrder` - engine: remove `RenderManager::setGeometryAt(index, count)` [⚠️ **API Change**] diff --git a/filament/backend/src/metal/MetalDriver.mm b/filament/backend/src/metal/MetalDriver.mm index 2cb7ad6cc0..2013f2c4c6 100644 --- a/filament/backend/src/metal/MetalDriver.mm +++ b/filament/backend/src/metal/MetalDriver.mm @@ -1073,33 +1073,13 @@ void MetalDriver::blit(TargetBufferFlags buffers, dstRect.left >= 0 && dstRect.bottom >= 0, "Source and destination rects must be positive."); - // Metal's texture coordinates have (0, 0) at the top-left of the texture, but Filament's - // coordinates have (0, 0) at bottom-left. - const NSInteger srcHeight = - srcTarget->isDefaultRenderTarget() ? - mContext->currentReadSwapChain->getSurfaceHeight() : srcTarget->height; - MTLRegion srcRegion = MTLRegionMake2D( - (NSUInteger) srcRect.left, - srcHeight - (NSUInteger) srcRect.bottom - srcRect.height, - srcRect.width, srcRect.height); - - const NSInteger dstHeight = - dstTarget->isDefaultRenderTarget() ? - mContext->currentDrawSwapChain->getSurfaceHeight() : dstTarget->height; - MTLRegion dstRegion = MTLRegionMake2D( - (NSUInteger) dstRect.left, - dstHeight - (NSUInteger) dstRect.bottom - dstRect.height, - dstRect.width, dstRect.height); - auto isBlitableTextureType = [](MTLTextureType t) { return t == MTLTextureType2D || t == MTLTextureType2DMultisample || t == MTLTextureType2DArray; }; - MetalBlitter::BlitArgs args; - args.filter = filter; - args.source.region = srcRegion; - args.destination.region = dstRegion; + // MetalBlitter supports blitting color and depth simultaneously, but for simplicitly we'll blit + // them separately. In practice, Filament only ever blits a single buffer at a time anyway. if (any(buffers & TargetBufferFlags::COLOR_ALL)) { // We always blit from/to the COLOR0 attachment. @@ -1111,12 +1091,17 @@ void MetalDriver::blit(TargetBufferFlags buffers, isBlitableTextureType(dstColorAttachment.getTexture().textureType), "Metal does not support blitting to/from non-2D textures."); + MetalBlitter::BlitArgs args; + args.filter = filter; + args.source.region = srcColorAttachment.getRegionFromClientRect(srcRect); + args.destination.region = dstColorAttachment.getRegionFromClientRect(dstRect); args.source.color = srcColorAttachment.getTexture(); args.destination.color = dstColorAttachment.getTexture(); args.source.level = srcColorAttachment.level; args.destination.level = dstColorAttachment.level; args.source.slice = srcColorAttachment.layer; args.destination.slice = dstColorAttachment.layer; + mContext->blitter->blit(getPendingCommandBuffer(mContext), args); } } @@ -1129,34 +1114,19 @@ void MetalDriver::blit(TargetBufferFlags buffers, isBlitableTextureType(dstDepthAttachment.getTexture().textureType), "Metal does not support blitting to/from non-2D textures."); + MetalBlitter::BlitArgs args; + args.filter = filter; + args.source.region = srcDepthAttachment.getRegionFromClientRect(srcRect); + args.destination.region = dstDepthAttachment.getRegionFromClientRect(dstRect); args.source.depth = srcDepthAttachment.getTexture(); args.destination.depth = dstDepthAttachment.getTexture(); - - if (args.blitColor()) { - // If blitting color, we've already set the source and destination levels and slices. - // Check that they match the requested depth levels/slices. - ASSERT_PRECONDITION(args.source.level == srcDepthAttachment.level, - "Color and depth source LOD must match. (%d != %d)", - args.source.level, srcDepthAttachment.level); - ASSERT_PRECONDITION(args.destination.level == dstDepthAttachment.level, - "Color and depth destination LOD must match. (%d != %d)", - args.destination.level, dstDepthAttachment.level); - ASSERT_PRECONDITION(args.source.slice == srcDepthAttachment.layer, - "Color and depth source layer must match. (%d != %d)", - args.source.slice, srcDepthAttachment.layer); - ASSERT_PRECONDITION(args.destination.slice == dstDepthAttachment.layer, - "Color and depth destination layer must match. (%d != %d)", - args.destination.slice, dstDepthAttachment.layer); - } - args.source.level = srcDepthAttachment.level; args.destination.level = dstDepthAttachment.level; args.source.slice = srcDepthAttachment.layer; args.destination.slice = dstDepthAttachment.layer; + mContext->blitter->blit(getPendingCommandBuffer(mContext), args); } } - - mContext->blitter->blit(getPendingCommandBuffer(mContext), args); } void MetalDriver::draw(PipelineState ps, Handle rph, uint32_t instanceCount) { diff --git a/filament/backend/src/metal/MetalHandles.h b/filament/backend/src/metal/MetalHandles.h index f26a683e1a..52f04266f4 100644 --- a/filament/backend/src/metal/MetalHandles.h +++ b/filament/backend/src/metal/MetalHandles.h @@ -257,6 +257,16 @@ public: return texture ? texture.pixelFormat : MTLPixelFormatInvalid; } + MTLRegion getRegionFromClientRect(Viewport rect) { + // Convert the Filament rect into Metal texture coordinates, taking into account Metal's + // inverted texture space and the mip level. Note that the underlying Texture could be + // larger than the RenderTarget. Metal's texture coordinates have (0, 0) at the top-left + // of the texture, but Filament's coordinates have (0, 0) at bottom-left. + const auto mipheight = texture.height >> level; + return MTLRegionMake2D((NSUInteger)rect.left, + mipheight - (NSUInteger)rect.bottom - rect.height, rect.width, rect.height); + } + explicit operator bool() const { return texture != nil; } diff --git a/filament/backend/test/test_Blit.cpp b/filament/backend/test/test_Blit.cpp index 9671ee06b6..25bad59d48 100644 --- a/filament/backend/test/test_Blit.cpp +++ b/filament/backend/test/test_Blit.cpp @@ -876,6 +876,8 @@ TEST_F(BackendTest, BlitRegion) { constexpr int kDstTexHeight = 384; constexpr auto kDstTexFormat = TextureFormat::RGBA8; constexpr int kNumLevels = 3; + constexpr int kSrcLevel = 1; + constexpr int kDstLevel = 0; // Create a SwapChain and make it current. We don't really use it so the res doesn't matter. auto swapChain = api.createSwapChainHeadless(256, 256, 0); @@ -894,23 +896,12 @@ TEST_F(BackendTest, BlitRegion) { SamplerType::SAMPLER_2D, kNumLevels, kDstTexFormat, 1, kDstTexWidth, kDstTexHeight, 1, TextureUsage::SAMPLEABLE | TextureUsage::COLOR_ATTACHMENT); - // Create a RenderTarget for each texture's miplevel. - Handle srcRenderTargets[kNumLevels]; - Handle dstRenderTargets[kNumLevels]; - for (uint8_t level = 0; level < kNumLevels; level++) { - srcRenderTargets[level] = api.createRenderTarget( TargetBufferFlags::COLOR, - kSrcTexWidth >> level, kSrcTexHeight >> level, 1, { srcTexture, level, 0 }, {}, {}); - dstRenderTargets[level] = api.createRenderTarget( TargetBufferFlags::COLOR, - kDstTexWidth >> level, kDstTexHeight >> level, 1, { dstTexture, level, 0 }, {}, {}); - } - // Blit one-quarter of src level 1 to dst level 0. - const int srcLevel = 1; Viewport srcRect = { - .left = (kSrcTexWidth >> srcLevel) / 2, - .bottom = (kSrcTexHeight >> srcLevel) / 2, - .width = (kSrcTexWidth >> srcLevel) / 2, - .height = (kSrcTexHeight >> srcLevel) / 2, + .left = 0, + .bottom = 0, + .width = (kSrcTexWidth >> kSrcLevel) / 2, + .height = (kSrcTexHeight >> kSrcLevel) / 2, }; Viewport dstRect = { .left = 10, @@ -919,9 +910,18 @@ TEST_F(BackendTest, BlitRegion) { .height = kDstTexHeight - 10, }; - api.blit(TargetBufferFlags::COLOR0, dstRenderTargets[0], - dstRect, srcRenderTargets[srcLevel], - srcRect, SamplerMagFilter::LINEAR); + // Create a RenderTarget for each texture's miplevel. + // We purposely set the render target width and height to smaller than the texture, to check + // that this case is handled correctly. + Handle srcRenderTarget = + api.createRenderTarget(TargetBufferFlags::COLOR, srcRect.width, + srcRect.height, 1, {srcTexture, kSrcLevel, 0}, {}, {}); + Handle dstRenderTarget = + api.createRenderTarget(TargetBufferFlags::COLOR, kDstTexWidth >> kDstLevel, + kDstTexHeight >> kDstLevel, 1, {dstTexture, kDstLevel, 0}, {}, {}); + + api.blit(TargetBufferFlags::COLOR0, dstRenderTarget, dstRect, srcRenderTarget, srcRect, + SamplerMagFilter::LINEAR); // Push through an empty frame to allow the texture to upload and the blit to execute. api.beginFrame(0, 0); @@ -931,7 +931,7 @@ TEST_F(BackendTest, BlitRegion) { // Grab a screenshot. ScreenshotParams params { kDstTexWidth, kDstTexHeight, "BlitRegion.png" }; api.beginFrame(0, 0); - dumpScreenshot(api, dstRenderTargets[0], ¶ms); + dumpScreenshot(api, dstRenderTarget, ¶ms); api.commit(swapChain); api.endFrame(0); @@ -940,12 +940,23 @@ TEST_F(BackendTest, BlitRegion) { executeCommands(); getDriver().purge(); + // Check if the image matches perfectly to our golden run. + // + // TODO: for some reason, this test has very, very slight (as in one pixel) differences between + // OpenGL and Metal. So disable golden checking for now. + // Use the compare tool from ImageMagick to see visual differences: + // compare -verbose -metric mae BlitRegion_Metal.png BlitRegion_OpenGL.png difference.png + // + // const uint32_t expected = 0x74fa34ed; + // printf("Computed hash is 0x%8.8x, Expected 0x%8.8x\n", params.pixelHashResult, expected); + // EXPECT_TRUE(params.pixelHashResult == expected); + // Cleanup. api.destroyTexture(srcTexture); api.destroyTexture(dstTexture); api.destroySwapChain(swapChain); - for (auto rt : srcRenderTargets) api.destroyRenderTarget(rt); - for (auto rt : dstRenderTargets) api.destroyRenderTarget(rt); + api.destroyRenderTarget(srcRenderTarget); + api.destroyRenderTarget(dstRenderTarget); executeCommands(); }