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

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<layers::Image> 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<gfx::Matrix4x4> mTransformOverride;
   // Only accessed on reader's task queue.
   bool mIsCodecSupportAdaptivePlayback = false;
   // Can be accessed on any thread, but only written on during init.

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: {

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<float>(size.width),
+                                        static_cast<float>(size.height));
 }
 
 void RenderAndroidHardwareBufferTextureHost::Unlock() {}

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<gfx::Point, gfx::Point> 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<gfx::Point, gfx::Point> 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<layers::TextureSource>

gfx/webrender_bindings/RenderAndroidSurfaceTextureHost.h

@@ -69,12 +69,9 @@ class RenderAndroidSurfaceTextureHost final : public RenderTextureHostSWGL {
   virtual ~RenderAndroidSurfaceTextureHost();
   bool EnsureAttachedToGLContext();
 
-  already_AddRefed<gfx::DataSourceSurface> 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<gfx::Point, gfx::Point> GetUvCoords(
-      gfx::IntSize aTextureSize) const override;
+  already_AddRefed<gfx::DataSourceSurface> ReadTexImage();
 
   enum PrepareStatus {
     STATUS_NONE,

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<float>(size.width),
+                                        static_cast<float>(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<float>(size.width),
+                                        static_cast<float>(size.height));
 }
 
 void RenderDXGIYCbCrTextureHost::Unlock() {

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<float>(size.width), static_cast<float>(size.height));
 }
 
 void RenderDMABUFTextureHost::Unlock() {}

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<float>(mSize.width),
+                                        static_cast<float>(mSize.height));
 }
 
 void RenderEGLImageTextureHost::Unlock() {}

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<float>(size.width),
+        static_cast<float>(size.height));
   }
 
   MOZ_ASSERT(mGL->GetError() == LOCAL_GL_NO_ERROR);

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<float>(size.width),
+                                        static_cast<float>(size.height));
 }
 
 void RenderMacIOSurfaceTextureHost::Unlock() {}

gfx/webrender_bindings/RenderTextureHost.cpp

@@ -44,13 +44,6 @@ wr::WrExternalImage RenderTextureHost::LockSWGL(uint8_t aChannelIndex,
   return InvalidToWrExternalImage();
 }
 
-std::pair<gfx::Point, gfx::Point> RenderTextureHost::GetUvCoords(
-    gfx::IntSize aTextureSize) const {
-  return std::make_pair(gfx::Point(0.0, 0.0),
-                        gfx::Point(static_cast<float>(aTextureSize.width),
-                                   static_cast<float>(aTextureSize.height)));
-}
-
 void RenderTextureHost::Destroy() {
   MOZ_ASSERT_UNREACHABLE("unexpected to be called");
 }

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<gfx::Point, gfx::Point> GetUvCoords(
-      gfx::IntSize aTextureSize) const;
-
   bool mIsFromDRMSource;
 
   // protected by RenderThread::mRenderTextureMapLock

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<float>(plane.mSize.width),
+                   static_cast<float>(plane.mSize.height));
 }
 
 void RenderTextureHostSWGL::UnlockSWGL() {

gfx/webrender_bindings/RenderTextureHostWrapper.cpp

@@ -52,14 +52,6 @@ void RenderTextureHostWrapper::Unlock() {
   }
 }
 
-std::pair<gfx::Point, gfx::Point> RenderTextureHostWrapper::GetUvCoords(
-    gfx::IntSize aTextureSize) const {
-  if (mTextureHost) {
-    return mTextureHost->GetUvCoords(aTextureSize);
-  }
-  return RenderTextureHost::GetUvCoords(aTextureSize);
-}
-
 void RenderTextureHostWrapper::ClearCachedResources() {
   if (mTextureHost) {
     mTextureHost->ClearCachedResources();

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<gfx::Point, gfx::Point> GetUvCoords(
-      gfx::IntSize aTextureSize) const override;
-
  private:
   ~RenderTextureHostWrapper() override;