WebGL & 16-bit video stream: upload to FLOAT texture.

third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp

Index: third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
diff --git a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
index 896056569e8b63c617b29734b1aefaa53d39effb..b9af74da456b0f0c1cec6844bac68cde4970ad4d 100644
--- a/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
+++ b/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
@@ -501,6 +501,28 @@ class ScopedFramebufferRestorer {
   Member<WebGLRenderingContextBase> m_context;
 };
 
+class ScopedUnpackParametersResetRestore {
+  STACK_ALLOCATED();
+
+ public:
+  explicit ScopedUnpackParametersResetRestore(
+      WebGLRenderingContextBase* context,
+      bool enabled = true)

[pdr., 2016/11/22 19:16:01]

This restore class is a nice way of cleaning up the reset/restore calls but the
bool for enabling it is not obvious at the callsites. WDYT of using
base::Optional for this?

The pattern would look something like when there is a conditional:
Optional<ScopedUnpackParametersResetRestore> resetRestoreScope;
if (should reset restore)
  resetRestoreScope.emplace(this);

The pattern would look something like this when there's no conditional:
ScopedUnpackParametersResetRestore resetRestoreScope(this);

[Ken Russell (switch to Gerrit), 2016/11/22 22:47:44]

+1, this is nice.
I prefer the way Alex has done this. The Optional class seems more complex and
harder to understand to me.

+      : m_context(context), m_enabled(enabled) {
+    if (enabled)
+      m_context->resetUnpackParameters();
+  }
+
+  ~ScopedUnpackParametersResetRestore() {
+    if (m_enabled)
+      m_context->restoreUnpackParameters();
+  }
+
+ private:
+  Member<WebGLRenderingContextBase> m_context;
+  bool m_enabled;
+};
+
 static void formatWebGLStatusString(const StringView& glInfo,
                                     const StringView& infoString,
                                     StringBuilder& builder) {
@@ -4417,7 +4439,7 @@ void WebGLRenderingContextBase::texImageImpl(
     }
   }
 
-  resetUnpackParameters();
+  ScopedUnpackParametersResetRestore temporaryResetUnpack(this);
   if (functionID == TexImage2D) {
     texImage2DBase(target, level, internalformat,
                    adjustedSourceImageRect.width(),
@@ -4443,7 +4465,6 @@ void WebGLRenderingContextBase::texImageImpl(
           depth, format, type, needConversion ? data.data() : imagePixelData);
     }
   }
-  restoreUnpackParameters();
 }
 
 bool WebGLRenderingContextBase::validateTexFunc(
@@ -4650,16 +4671,14 @@ void WebGLRenderingContextBase::texImageHelperDOMArrayBufferView(
     return;
   }
 
-  if (changeUnpackAlignment)
-    resetUnpackParameters();
+  ScopedUnpackParametersResetRestore temporaryResetUnpack(
+      this, changeUnpackAlignment);
   if (functionID == TexImage2D)
     texImage2DBase(target, level, internalformat, width, height, border, format,
                    type, data);
   else if (functionID == TexSubImage2D)
     contextGL()->TexSubImage2D(target, level, xoffset, yoffset, width, height,
                                format, type, data);
-  if (changeUnpackAlignment)
-    restoreUnpackParameters();
 }
 
 void WebGLRenderingContextBase::texImage2D(GLenum target,
@@ -4747,7 +4766,7 @@ void WebGLRenderingContextBase::texImageHelperImageData(
       return;
     }
   }
-  resetUnpackParameters();
+  ScopedUnpackParametersResetRestore temporaryResetUnpack(this);
   const uint8_t* bytes = needConversion ? data.data() : pixels->data()->data();
   if (functionID == TexImage2D) {
     DCHECK_EQ(unpackImageHeight, 0);
@@ -4776,7 +4795,6 @@ void WebGLRenderingContextBase::texImageHelperImageData(
                                  depth, format, type, bytes);
     }
   }
-  restoreUnpackParameters();
 }
 
 void WebGLRenderingContextBase::texImage2D(GLenum target,
@@ -5146,60 +5164,70 @@ void WebGLRenderingContextBase::texImageHelperHTMLVideoElement(
       sourceImageRect == sentinelEmptyRect() ||
       sourceImageRect ==
           IntRect(0, 0, video->videoWidth(), video->videoHeight());
-  if (functionID == TexImage2D && sourceImageRectIsDefault && depth == 1) {
+  if (functionID == TexImage2D && sourceImageRectIsDefault && depth == 1 &&
+      GL_TEXTURE_2D == target && Extensions3DUtil::canUseCopyTextureCHROMIUM(
+                                     target, internalformat, type, level)) {

[Ken Russell (switch to Gerrit), 2016/11/22 22:47:44]

This change to the if-test will make the semi-accelerated code path below (using
an accelerated image buffer) be taken less often. What is the rationale for
making this change? Can you figure out a way to preserve the current properties
of the code? If you want to inject the attempted call to
HTMLVideoElement::texImageImpl, can you figure out a slightly different code
structure to achieve this?

[aleksandar.stojiljkovic, 2016/11/22 23:21:06]

The change here is orthogonal in respect to the rest of the code. It is
resolving performance issue of using float textures regardless what the video
format is.
The only difference is that semi-accelerated code is under
Extensions3DUtil::canUseCopyTextureCHROMIUM.
And it should be, as ImageBuffer::copyToPlatformtexture l.5207 returns early
false (for e.g. float formats after canUseCopyTextureCHROMIUM) in [1]. 
So, there is no need to do new AcceleratedImageBufferSurface,
video->paintCurrentFrame and texImage2DBase in 5204 as the code would just early
leave call to l.5207.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

 

[aleksandar.stojiljkovic, 2016/11/29 11:59:53]

Done. This landed in crrev.com/2527343002.

     DCHECK_EQ(xoffset, 0);
     DCHECK_EQ(yoffset, 0);
     DCHECK_EQ(zoffset, 0);
     // Go through the fast path doing a GPU-GPU textures copy without a readback
     // to system memory if possible.  Otherwise, it will fall back to the normal
     // SW path.
-    if (GL_TEXTURE_2D == target) {
-      if (Extensions3DUtil::canUseCopyTextureCHROMIUM(target, internalformat,
-                                                      type, level) &&
-          video->copyVideoTextureToPlatformTexture(
-              contextGL(), texture->object(), internalformat, type,
-              m_unpackPremultiplyAlpha, m_unpackFlipY)) {
-        return;
-      }
+    if (video->copyVideoTextureToPlatformTexture(
+            contextGL(), texture->object(), internalformat, type,
+            m_unpackPremultiplyAlpha, m_unpackFlipY)) {
+      return;
+    }
 
-      // Try using an accelerated image buffer, this allows YUV conversion to be
-      // done on the GPU.
-      std::unique_ptr<ImageBufferSurface> surface =
-          wrapUnique(new AcceleratedImageBufferSurface(
-              IntSize(video->videoWidth(), video->videoHeight())));
-      if (surface->isValid()) {
-        std::unique_ptr<ImageBuffer> imageBuffer(
-            ImageBuffer::create(std::move(surface)));
-        if (imageBuffer) {
-          // The video element paints an RGBA frame into our surface here. By
-          // using an AcceleratedImageBufferSurface, we enable the
-          // WebMediaPlayer implementation to do any necessary color space
-          // conversion on the GPU (though it
-          // may still do a CPU conversion and upload the results).
-          video->paintCurrentFrame(
-              imageBuffer->canvas(),
-              IntRect(0, 0, video->videoWidth(), video->videoHeight()),
-              nullptr);
-
-          // This is a straight GPU-GPU copy, any necessary color space
-          // conversion was handled in the paintCurrentFrameInContext() call.
-
-          // Note that copyToPlatformTexture no longer allocates the
-          // destination texture.
-          texImage2DBase(target, level, internalformat, video->videoWidth(),
-                         video->videoHeight(), 0, format, type, nullptr);
-
-          if (imageBuffer->copyToPlatformTexture(
-                  contextGL(), texture->object(), internalformat, type, level,
-                  m_unpackPremultiplyAlpha, m_unpackFlipY, IntPoint(0, 0),
-                  IntRect(0, 0, video->videoWidth(), video->videoHeight()))) {
-            return;
-          }
+    // Try using an accelerated image buffer, this allows YUV conversion to be
+    // done on the GPU.
+    std::unique_ptr<ImageBufferSurface> surface =
+        wrapUnique(new AcceleratedImageBufferSurface(
+            IntSize(video->videoWidth(), video->videoHeight())));
+    if (surface->isValid()) {
+      std::unique_ptr<ImageBuffer> imageBuffer(
+          ImageBuffer::create(std::move(surface)));
+      if (imageBuffer) {
+        // The video element paints an RGBA frame into our surface here. By
+        // using an AcceleratedImageBufferSurface, we enable the WebMediaPlayer
+        // implementation to do any necessary color space conversion on the GPU
+        // (though it may still do a CPU conversion and upload the results).
+        video->paintCurrentFrame(
+            imageBuffer->canvas(),
+            IntRect(0, 0, video->videoWidth(), video->videoHeight()), nullptr);
+
+        // This is a straight GPU-GPU copy, any necessary color space conversion
+        // was handled in the paintCurrentFrameInContext() call.
+
+        // Note that copyToPlatformTexture no longer allocates the destination
+        // texture.
+        texImage2DBase(target, level, internalformat, video->videoWidth(),
+                       video->videoHeight(), 0, format, type, nullptr);
+
+        if (imageBuffer->copyToPlatformTexture(
+                contextGL(), texture->object(), internalformat, type, level,
+                m_unpackPremultiplyAlpha, m_unpackFlipY, IntPoint(0, 0),
+                IntRect(0, 0, video->videoWidth(), video->videoHeight()))) {
+          return;
         }
       }
     }
   }
 
+  {
+    // Try using optimized CPU-GPU path for some formats: e.g. Y16 and Y8. It
+    // leaves early for other formats or if frame is stored on GPU.

[Ken Russell (switch to Gerrit), 2016/11/22 22:47:44]

This now needs to take into consideration the fact that a source sub-rectangle
might be specified for WebGL 2.0. The easiest way for the moment would be to
skip attempting this block if (!sourceImageRectIsDefault). It can be expanded
later to handle sub-rectangle uploads.

[aleksandar.stojiljkovic, 2016/11/22 23:21:06]

Thanks. I'll do so.

[aleksandar.stojiljkovic, 2016/11/27 20:44:00]

Done.

+    ScopedUnpackParametersResetRestore(
+        this, m_unpackFlipY || m_unpackPremultiplyAlpha);
+    if (video->texImageImpl(
+            static_cast<WebMediaPlayer::TexImageFunctionID>(functionID), target,
+            contextGL(), level, convertTexInternalFormat(internalformat, type),
+            format, type, xoffset, yoffset, zoffset, m_unpackFlipY,
+            m_unpackPremultiplyAlpha &&
+                m_unpackColorspaceConversion == GL_NONE))
+      return;
+  }
+
   RefPtr<Image> image = videoFrameToImage(video);
   if (!image)
     return;
@@ -5341,7 +5369,7 @@ void WebGLRenderingContextBase::texImageHelperImageBitmap(
       return;
     }
   }
-  resetUnpackParameters();
+  ScopedUnpackParametersResetRestore temporaryResetUnpack(this);
   if (functionID == TexImage2D) {
     texImage2DBase(target, level, internalformat, width, height, 0, format,
                    type, needConversion ? data.data() : pixelDataPtr);
@@ -5359,7 +5387,6 @@ void WebGLRenderingContextBase::texImageHelperImageBitmap(
                                height, depth, format, type,
                                needConversion ? data.data() : pixelDataPtr);
   }
-  restoreUnpackParameters();
 }
 
 void WebGLRenderingContextBase::texImage2D(GLenum target,