From fc8a98e8d982e775e552c511a8cc63296aaa551c Mon Sep 17 00:00:00 2001 From: Jamie Nicol Date: Mon, 9 Sep 2024 14:06:26 +0000 Subject: [PATCH] Bug 1913568 - Handle SurfaceTexture transforms in webrender, for reals this time. r=gfx-reviewers,media-playback-reviewers,padenot,nical On Android, SurfaceTextures provide a transform that should be applied to texture coordinates when sampling from the texture. Usually this is simply a y-flip, but sometimes it includes a scale and slight translation, eg when the video frame is contained within a larger texture. Previously we ignored this transform but performed a y-flip, meaning we rendered correctly most of the time, but not all of the time. Our first attempt to fix this was in bug 1731980. When rendering as a compositor surface with RenderCompositorOGLSWGL, we supplied the transform to CompositorOGL's shaders, which correctly fixed the bug for this rendering path. However, the attempted fix for hardware webrender in fact made things worse. As UV coordinates are supplied to webrender unnormalized, then the shaders normalize them by dividing by the actual texture size, this effectively handled the scale component of the transform. (Though not quite scaling by the correct amount, and ignoring the translation component, sometimes resulting in a pixel-wide green seam being visible at the video's edges.) When we additionally applied the transformation to the coordinates, it resulted in the scale being applied twice, and the video being rendered too far zoomed in. To make matters worse, when we received subsequent bug reports of incorrect rendering on various devices we mistakenly assumed that the devices must be buggy, rather than our code being incorrect. We therefore reverted to ignoring the transform on these devices, thereby breaking the software webrender path again. Additionally, on devices without GL_OES_EGL_image_external_essl3 support, we must sample from the SurfaceTexture using an ESSL1 shader. This means we do not have access to the correct texture size, meaning we cannot correctly normalize the UV coordinates. This results in the video being rendered too far zoomed out. And in the non-compositor-surface software webrender path, we were accidentally downscaling the texture when reading back into a CPU buffer, resulting in the video being rendered at the correct zoom, but being very blurry. This patch aims to handle the transform correctly, in all rendering paths, hopefully once and for all. For hardware webrender, we now supply the texture coordinates to webrender already normalized, using the functionality added in the previous patch. This avoids the shaders scaling the coordinates again, or using an incorrect texture size to do so. For RenderCompositorOGLSWGL, we continue to apply the transform using CompositorOGL's shaders. In the non-compositor-surface software webrender path, we make GLReadPixelsHelper apply the transform when reading from the SurfaceTexture in to the CPU buffer. Again using functionality added earlier in this patch series. This avoids downscaling the image. We can then provide the default untransformed and unnormalized UVs to webrender. As a result we can now remove the virtual function RenderTextureHost::GetUvCoords(), added in bug 1731980, as it no longer serves any purpose: we no longer want to share the implementation between RenderAndroidSurfaceTextureHost::Lock and RenderTextureHostSWGL::LockSWGL. Finally, we remove all transform overrides on the devices we mistakenly assumed were buggy. Differential Revision: https://phabricator.services.mozilla.com/D220582 --- .../platforms/android/RemoteDataDecoder.cpp | 16 +------- gfx/layers/opengl/TextureHostOGL.cpp | 11 +++++- ...RenderAndroidHardwareBufferTextureHost.cpp | 7 ++-- .../RenderAndroidSurfaceTextureHost.cpp | 39 ++++++++----------- .../RenderAndroidSurfaceTextureHost.h | 7 +--- .../RenderD3D11TextureHost.cpp | 16 ++++---- .../RenderDMABUFTextureHost.cpp | 10 ++--- .../RenderEGLImageTextureHost.cpp | 6 +-- .../RenderExternalTextureHost.cpp | 5 ++- .../RenderMacIOSurfaceTextureHost.cpp | 8 ++-- gfx/webrender_bindings/RenderTextureHost.cpp | 7 ---- gfx/webrender_bindings/RenderTextureHost.h | 7 ---- .../RenderTextureHostSWGL.cpp | 13 ++----- .../RenderTextureHostWrapper.cpp | 8 ---- .../RenderTextureHostWrapper.h | 5 --- 15 files changed, 61 insertions(+), 104 deletions(-) diff --git a/dom/media/platforms/android/RemoteDataDecoder.cpp b/dom/media/platforms/android/RemoteDataDecoder.cpp index 72cce0974ac9..59c7651f3b53 100644 --- a/dom/media/platforms/android/RemoteDataDecoder.cpp +++ b/dom/media/platforms/android/RemoteDataDecoder.cpp @@ -222,17 +222,6 @@ class RemoteVideoDecoder final : public RemoteDataDecoder { mJavaDecoder->IsAdaptivePlaybackSupported(); mIsHardwareAccelerated = mJavaDecoder->IsHardwareAccelerated(); - // On some devices we have observed that the transform obtained from - // SurfaceTexture.getTransformMatrix() is incorrect for surfaces produced by - // a MediaCodec. We therefore override the transform to be a simple y-flip - // to ensure it is rendered correctly. - const auto hardware = java::sdk::Build::HARDWARE()->ToString(); - if (hardware.EqualsASCII("mt6735") || hardware.EqualsASCII("kirin980") || - hardware.EqualsASCII("mt8696")) { - mTransformOverride = Some( - gfx::Matrix4x4::Scaling(1.0, -1.0, 1.0).PostTranslate(0.0, 1.0, 0.0)); - } - mMediaInfoFlag = MediaInfoFlag::None; mMediaInfoFlag |= mIsHardwareAccelerated ? MediaInfoFlag::HardwareDecoding : MediaInfoFlag::SoftwareDecoding; @@ -427,7 +416,7 @@ class RemoteVideoDecoder final : public RemoteDataDecoder { RefPtr img = new layers::SurfaceTextureImage( mSurfaceHandle, inputInfo.mImageSize, false /* NOT continuous */, gl::OriginPos::BottomLeft, mConfig.HasAlpha(), forceBT709ColorSpace, - mTransformOverride); + /* aTransformOverride */ Nothing()); img->AsSurfaceTextureImage()->RegisterSetCurrentCallback( std::move(releaseSample)); @@ -568,9 +557,6 @@ class RemoteVideoDecoder final : public RemoteDataDecoder { const VideoInfo mConfig; java::GeckoSurface::GlobalRef mSurface; AndroidSurfaceTextureHandle mSurfaceHandle{}; - // Used to override the SurfaceTexture transform on some devices where the - // decoder provides a buggy value. - Maybe mTransformOverride; // Only accessed on reader's task queue. bool mIsCodecSupportAdaptivePlayback = false; // Can be accessed on any thread, but only written on during init. diff --git a/gfx/layers/opengl/TextureHostOGL.cpp b/gfx/layers/opengl/TextureHostOGL.cpp index 7ff1b72c34ef..c67d014ae987 100644 --- a/gfx/layers/opengl/TextureHostOGL.cpp +++ b/gfx/layers/opengl/TextureHostOGL.cpp @@ -571,6 +571,15 @@ void SurfaceTextureHost::PushResourceUpdates( wr::ImageBufferKind::TextureExternalBT709); } + // Hardware webrender directly renders from the SurfaceTexture therefore we + // must provide it the (transformed) normalized UVs. For software webrender we + // first read from the SurfaceTexture in to a CPU buffer, which we sample from + // using unnormalized UVs. The readback code handles the texture transform. + // See RenderAndroidSurfaceTextureHost::Lock() and + // RenderAndroidSurfaceTextureHost::ReadTexImage(), respectively. + const bool normalizedUvs = + aResources.GetBackendType() == WebRenderBackend::HARDWARE; + switch (GetFormat()) { case gfx::SurfaceFormat::R8G8B8X8: case gfx::SurfaceFormat::R8G8B8A8: { @@ -583,7 +592,7 @@ void SurfaceTextureHost::PushResourceUpdates( : gfx::SurfaceFormat::B8G8R8X8; wr::ImageDescriptor descriptor(GetSize(), format); (aResources.*method)(aImageKeys[0], descriptor, aExtID, imageType, 0, - /* aNormalizedUvs */ false); + normalizedUvs); break; } default: { diff --git a/gfx/webrender_bindings/RenderAndroidHardwareBufferTextureHost.cpp b/gfx/webrender_bindings/RenderAndroidHardwareBufferTextureHost.cpp index 80b704b12f69..144c8817da5a 100644 --- a/gfx/webrender_bindings/RenderAndroidHardwareBufferTextureHost.cpp +++ b/gfx/webrender_bindings/RenderAndroidHardwareBufferTextureHost.cpp @@ -132,9 +132,10 @@ wr::WrExternalImage RenderAndroidHardwareBufferTextureHost::Lock( return InvalidToWrExternalImage(); } - const auto uvs = GetUvCoords(GetSize()); - return NativeTextureToWrExternalImage( - mTextureHandle, uvs.first.x, uvs.first.y, uvs.second.x, uvs.second.y); + const gfx::IntSize size = GetSize(); + return NativeTextureToWrExternalImage(mTextureHandle, 0.0, 0.0, + static_cast(size.width), + static_cast(size.height)); } void RenderAndroidHardwareBufferTextureHost::Unlock() {} diff --git a/gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.cpp b/gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.cpp index 48705e83b604..08b8335cebe1 100644 --- a/gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.cpp +++ b/gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.cpp @@ -72,10 +72,20 @@ wr::WrExternalImage RenderAndroidSurfaceTextureHost::Lock(uint8_t aChannelIndex, UpdateTexImageIfNecessary(); - const auto uvs = GetUvCoords(mSize); - return NativeTextureToWrExternalImage(mSurfTex->GetTexName(), uvs.first.x, - uvs.first.y, uvs.second.x, - uvs.second.y); + const gfx::Matrix4x4 transform = GetTextureTransform(); + // We expect this transform to always be rectilinear, usually just a + // y-flip and sometimes an x and y scale/translation. This allows us + // to simply transform 2 points here instead of 4. + MOZ_ASSERT(transform.IsRectilinear(), + "Unexpected non-rectilinear transform returned from " + "SurfaceTexture.GetTransformMatrix()"); + gfx::Point uv0(0.0, 0.0); + gfx::Point uv1(1.0, 1.0); + uv0 = transform.TransformPoint(uv0); + uv1 = transform.TransformPoint(uv1); + + return NativeTextureToWrExternalImage(mSurfTex->GetTexName(), uv0.x, uv0.y, + uv1.x, uv1.y); } void RenderAndroidSurfaceTextureHost::Unlock() {} @@ -251,7 +261,7 @@ RenderAndroidSurfaceTextureHost::ReadTexImage() { bool ret = mGL->ReadTexImageHelper()->ReadTexImage( surf, mSurfTex->GetTexName(), LOCAL_GL_TEXTURE_EXTERNAL, mSize, - gfx::Matrix4x4(), shaderConfig, /* aYInvert */ false); + GetTextureTransform(), shaderConfig, /* aYInvert */ false); if (!ret) { return nullptr; } @@ -288,8 +298,7 @@ void RenderAndroidSurfaceTextureHost::UnmapPlanes() { } } -std::pair RenderAndroidSurfaceTextureHost::GetUvCoords( - gfx::IntSize aTextureSize) const { +gfx::Matrix4x4 RenderAndroidSurfaceTextureHost::GetTextureTransform() const { gfx::Matrix4x4 transform; // GetTransformMatrix() returns the transform set by the producer side of the @@ -305,21 +314,7 @@ std::pair RenderAndroidSurfaceTextureHost::GetUvCoords( gl::AndroidSurfaceTexture::GetTransformMatrix(surf, &transform); } - // We expect this transform to always be rectilinear, usually just a - // y-flip and sometimes an x and y scale. This allows this function - // to simply transform and return 2 points here instead of 4. - MOZ_ASSERT(transform.IsRectilinear(), - "Unexpected non-rectilinear transform returned from " - "SurfaceTexture.GetTransformMatrix()"); - - transform.PostScale(aTextureSize.width, aTextureSize.height, 0.0); - - gfx::Point uv0 = gfx::Point(0.0, 0.0); - gfx::Point uv1 = gfx::Point(1.0, 1.0); - uv0 = transform.TransformPoint(uv0); - uv1 = transform.TransformPoint(uv1); - - return std::make_pair(uv0, uv1); + return transform; } RefPtr diff --git a/gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.h b/gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.h index d8ecfa907055..c7ce5b528402 100644 --- a/gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.h +++ b/gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.h @@ -69,12 +69,9 @@ class RenderAndroidSurfaceTextureHost final : public RenderTextureHostSWGL { virtual ~RenderAndroidSurfaceTextureHost(); bool EnsureAttachedToGLContext(); - already_AddRefed ReadTexImage(); + gfx::Matrix4x4 GetTextureTransform() const; - // Returns the UV coordinates to be used when sampling the texture, taking in - // to account the SurfaceTexture's transform if applicable. - std::pair GetUvCoords( - gfx::IntSize aTextureSize) const override; + already_AddRefed ReadTexImage(); enum PrepareStatus { STATUS_NONE, diff --git a/gfx/webrender_bindings/RenderD3D11TextureHost.cpp b/gfx/webrender_bindings/RenderD3D11TextureHost.cpp index eeb0f57d2c8f..931c536a3d61 100644 --- a/gfx/webrender_bindings/RenderD3D11TextureHost.cpp +++ b/gfx/webrender_bindings/RenderD3D11TextureHost.cpp @@ -380,10 +380,10 @@ wr::WrExternalImage RenderDXGITextureHost::Lock(uint8_t aChannelIndex, return InvalidToWrExternalImage(); } - const auto uvs = GetUvCoords(GetSize(aChannelIndex)); - return NativeTextureToWrExternalImage(GetGLHandle(aChannelIndex), uvs.first.x, - uvs.first.y, uvs.second.x, - uvs.second.y); + const gfx::IntSize size = GetSize(aChannelIndex); + return NativeTextureToWrExternalImage(GetGLHandle(aChannelIndex), 0.0, 0.0, + static_cast(size.width), + static_cast(size.height)); } bool RenderDXGITextureHost::LockInternal() { @@ -701,10 +701,10 @@ wr::WrExternalImage RenderDXGIYCbCrTextureHost::Lock(uint8_t aChannelIndex, return InvalidToWrExternalImage(); } - const auto uvs = GetUvCoords(GetSize(aChannelIndex)); - return NativeTextureToWrExternalImage(GetGLHandle(aChannelIndex), uvs.first.x, - uvs.first.y, uvs.second.x, - uvs.second.y); + const gfx::IntSize size = GetSize(aChannelIndex); + return NativeTextureToWrExternalImage(GetGLHandle(aChannelIndex), 0.0, 0.0, + static_cast(size.width), + static_cast(size.height)); } void RenderDXGIYCbCrTextureHost::Unlock() { diff --git a/gfx/webrender_bindings/RenderDMABUFTextureHost.cpp b/gfx/webrender_bindings/RenderDMABUFTextureHost.cpp index f52535b95a44..4d54680dbfcf 100644 --- a/gfx/webrender_bindings/RenderDMABUFTextureHost.cpp +++ b/gfx/webrender_bindings/RenderDMABUFTextureHost.cpp @@ -46,11 +46,11 @@ wr::WrExternalImage RenderDMABUFTextureHost::Lock(uint8_t aChannelIndex, mSurface->GetTexture(aChannelIndex)); } - const auto uvs = GetUvCoords(gfx::IntSize( - mSurface->GetWidth(aChannelIndex), mSurface->GetHeight(aChannelIndex))); - return NativeTextureToWrExternalImage(mSurface->GetTexture(aChannelIndex), - uvs.first.x, uvs.first.y, uvs.second.x, - uvs.second.y); + const gfx::IntSize size(mSurface->GetWidth(aChannelIndex), + mSurface->GetHeight(aChannelIndex)); + return NativeTextureToWrExternalImage( + mSurface->GetTexture(aChannelIndex), 0.0, 0.0, + static_cast(size.width), static_cast(size.height)); } void RenderDMABUFTextureHost::Unlock() {} diff --git a/gfx/webrender_bindings/RenderEGLImageTextureHost.cpp b/gfx/webrender_bindings/RenderEGLImageTextureHost.cpp index 6db6dcd41ee1..ff8d1e764b2f 100644 --- a/gfx/webrender_bindings/RenderEGLImageTextureHost.cpp +++ b/gfx/webrender_bindings/RenderEGLImageTextureHost.cpp @@ -54,9 +54,9 @@ wr::WrExternalImage RenderEGLImageTextureHost::Lock(uint8_t aChannelIndex, return InvalidToWrExternalImage(); } - const auto uvs = GetUvCoords(mSize); - return NativeTextureToWrExternalImage( - mTextureHandle, uvs.first.x, uvs.first.y, uvs.second.x, uvs.second.y); + return NativeTextureToWrExternalImage(mTextureHandle, 0.0, 0.0, + static_cast(mSize.width), + static_cast(mSize.height)); } void RenderEGLImageTextureHost::Unlock() {} diff --git a/gfx/webrender_bindings/RenderExternalTextureHost.cpp b/gfx/webrender_bindings/RenderExternalTextureHost.cpp index 460a12c6abae..155004881e1b 100644 --- a/gfx/webrender_bindings/RenderExternalTextureHost.cpp +++ b/gfx/webrender_bindings/RenderExternalTextureHost.cpp @@ -166,9 +166,10 @@ void RenderExternalTextureHost::UpdateTexture(size_t aIndex) { texture = new layers::DirectMapTextureSource(mGL, mSurfaces[aIndex]); const GLuint handle = texture->GetTextureHandle(); - const auto uvs = GetUvCoords(texture->GetSize()); + const gfx::IntSize size = texture->GetSize(); mImages[aIndex] = NativeTextureToWrExternalImage( - handle, uvs.first.x, uvs.first.y, uvs.second.x, uvs.second.y); + handle, 0.0, 0.0, static_cast(size.width), + static_cast(size.height)); } MOZ_ASSERT(mGL->GetError() == LOCAL_GL_NO_ERROR); diff --git a/gfx/webrender_bindings/RenderMacIOSurfaceTextureHost.cpp b/gfx/webrender_bindings/RenderMacIOSurfaceTextureHost.cpp index f675015eb166..4572cf25ab03 100644 --- a/gfx/webrender_bindings/RenderMacIOSurfaceTextureHost.cpp +++ b/gfx/webrender_bindings/RenderMacIOSurfaceTextureHost.cpp @@ -105,10 +105,10 @@ wr::WrExternalImage RenderMacIOSurfaceTextureHost::Lock(uint8_t aChannelIndex, } } - const auto uvs = GetUvCoords(GetSize(aChannelIndex)); - return NativeTextureToWrExternalImage(GetGLHandle(aChannelIndex), uvs.first.x, - uvs.first.y, uvs.second.x, - uvs.second.y); + const auto size = GetSize(aChannelIndex); + return NativeTextureToWrExternalImage(GetGLHandle(aChannelIndex), 0.0, 0.0, + static_cast(size.width), + static_cast(size.height)); } void RenderMacIOSurfaceTextureHost::Unlock() {} diff --git a/gfx/webrender_bindings/RenderTextureHost.cpp b/gfx/webrender_bindings/RenderTextureHost.cpp index 2972b820ea8d..11b6469745cc 100644 --- a/gfx/webrender_bindings/RenderTextureHost.cpp +++ b/gfx/webrender_bindings/RenderTextureHost.cpp @@ -44,13 +44,6 @@ wr::WrExternalImage RenderTextureHost::LockSWGL(uint8_t aChannelIndex, return InvalidToWrExternalImage(); } -std::pair RenderTextureHost::GetUvCoords( - gfx::IntSize aTextureSize) const { - return std::make_pair(gfx::Point(0.0, 0.0), - gfx::Point(static_cast(aTextureSize.width), - static_cast(aTextureSize.height))); -} - void RenderTextureHost::Destroy() { MOZ_ASSERT_UNREACHABLE("unexpected to be called"); } diff --git a/gfx/webrender_bindings/RenderTextureHost.h b/gfx/webrender_bindings/RenderTextureHost.h index 09f4991e65fe..2eee879a288d 100644 --- a/gfx/webrender_bindings/RenderTextureHost.h +++ b/gfx/webrender_bindings/RenderTextureHost.h @@ -169,13 +169,6 @@ class RenderTextureHost { protected: virtual ~RenderTextureHost(); - // Returns the UV coordinates to be used when sampling the texture, in pixels. - // For most implementations these will be (0, 0) and (size.x, size.y), but - // some texture types (such as RenderAndroidSurfaceTextureHost) require an - // additional transform to be applied to the coordinates. - virtual std::pair GetUvCoords( - gfx::IntSize aTextureSize) const; - bool mIsFromDRMSource; // protected by RenderThread::mRenderTextureMapLock diff --git a/gfx/webrender_bindings/RenderTextureHostSWGL.cpp b/gfx/webrender_bindings/RenderTextureHostSWGL.cpp index ce6b00e6014c..a4b7ec235874 100644 --- a/gfx/webrender_bindings/RenderTextureHostSWGL.cpp +++ b/gfx/webrender_bindings/RenderTextureHostSWGL.cpp @@ -120,22 +120,17 @@ wr::WrExternalImage RenderTextureHostSWGL::LockSWGL( } const PlaneInfo& plane = mPlanes[aChannelIndex]; - const auto uvs = GetUvCoords(plane.mSize); - // Prefer native textures, unless our backend forbids it. - // If the GetUvCoords call above returned anything other than the default, - // for example if this is a RenderAndroidSurfaceTextureHost, then this won't - // be handled correctly in the RawDataToWrExternalImage path. But we shouldn't - // hit this path in practice with a RenderAndroidSurfaceTextureHost. layers::TextureHost::NativeTexturePolicy policy = layers::TextureHost::BackendNativeTexturePolicy( layers::WebRenderBackend::SOFTWARE, plane.mSize); return policy == layers::TextureHost::NativeTexturePolicy::FORBID ? RawDataToWrExternalImage((uint8_t*)plane.mData, plane.mStride * plane.mSize.height) - : NativeTextureToWrExternalImage(plane.mTexture, uvs.first.x, - uvs.first.y, uvs.second.x, - uvs.second.y); + : NativeTextureToWrExternalImage( + plane.mTexture, 0.0, 0.0, + static_cast(plane.mSize.width), + static_cast(plane.mSize.height)); } void RenderTextureHostSWGL::UnlockSWGL() { diff --git a/gfx/webrender_bindings/RenderTextureHostWrapper.cpp b/gfx/webrender_bindings/RenderTextureHostWrapper.cpp index b44fd85722f8..d91a8216de5a 100644 --- a/gfx/webrender_bindings/RenderTextureHostWrapper.cpp +++ b/gfx/webrender_bindings/RenderTextureHostWrapper.cpp @@ -52,14 +52,6 @@ void RenderTextureHostWrapper::Unlock() { } } -std::pair RenderTextureHostWrapper::GetUvCoords( - gfx::IntSize aTextureSize) const { - if (mTextureHost) { - return mTextureHost->GetUvCoords(aTextureSize); - } - return RenderTextureHost::GetUvCoords(aTextureSize); -} - void RenderTextureHostWrapper::ClearCachedResources() { if (mTextureHost) { mTextureHost->ClearCachedResources(); diff --git a/gfx/webrender_bindings/RenderTextureHostWrapper.h b/gfx/webrender_bindings/RenderTextureHostWrapper.h index 7ed47b9a7c65..09a4d8b47fee 100644 --- a/gfx/webrender_bindings/RenderTextureHostWrapper.h +++ b/gfx/webrender_bindings/RenderTextureHostWrapper.h @@ -68,11 +68,6 @@ class RenderTextureHostWrapper final : public RenderTextureHostSWGL { // size of the wrapped object (which reports itself). size_t Bytes() override { return 0; } - protected: - // RenderTextureHost - std::pair GetUvCoords( - gfx::IntSize aTextureSize) const override; - private: ~RenderTextureHostWrapper() override;