Bug 203148 - texImage2d fails to set same video frame to multiple textures
Summary: texImage2d fails to set same video frame to multiple textures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: Safari 13
Hardware: All Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
: 234230 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-10-18 02:43 PDT by Simon Taylor
Modified: 2022-02-28 02:22 PST (History)
14 users (show)

See Also:


Attachments
Example page (7.90 KB, text/html)
2019-10-18 03:05 PDT, Simon Taylor
no flags Details
Patch (24.69 KB, patch)
2022-01-21 07:43 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (29.86 KB, patch)
2022-01-21 09:10 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (29.52 KB, patch)
2022-01-24 00:53 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Taylor 2019-10-18 02:43:07 PDT
Code such as this in a requestAnimationFrame callback usually results in only the first bound texture being updated:
  
  // video is a HTMLVideoElement
  
  // Update tex1        
  gl.bindTexture(gl.TEXTURE_2D, tex1);
  gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGB, gl.RGB, gl.UNSIGNED_BYTE, video);
  
  // Update tex2 immediately
  gl.bindTexture(gl.TEXTURE_2D, tex2);
  gl.texImage2D(gl.TEXTURE_2D, 0, gl.RGB, gl.RGB, gl.UNSIGNED_BYTE, video);

It looks like WebKit has an optimisation to skip updating a WebGL texture from a video if the frame hasn't changed since the previous texImage2d call.

This is a welcome optimisation but it appears not to verify that the currently bound texture is the same one that was bound for the previous call. There are other cases that should also prevent this optimization (I have not tested these), such as any other glTex[Sub]Image having being called since the last frame upload, the texture having been used as a draw buffer in a Framebuffer object, and perhaps others.

Effectively I suppose the test should be something like:
bool canSkipUpload = (lastWriteActionToBoundTexId == videoUpload && lastVideoUploadFrameToBoundTexId == video.currentFrame);

One use-case for uploading the same video to different textures is to implement pipelined processing of the data in the frame, alternating the texture used for the upload each frame. Something like this:

===
rAF #1:
upload frame 1 to tex 1
start processing data in tex 1

rAF #2:
upload frame 2 to tex 2
start processing data in tex 2
render from tex 1 (frame 1) along with processed result

rAF #3:
upload frame 3 to tex 1
start processing data in tex 1
render from tex 2 (frame 2) along with processed result

etc...

===

Clearly it would also be nice to have an API to query whether the frame has changed before doing the upload, so the whole repeated "upload and process" step can be skipped, but that's a separate issue. I can at least rate-limit the uploads to approximate that, but still need the guarantee that the texture will actually be updated even if the bound texture is different and the frame happens to be the same.
Comment 1 Simon Taylor 2019-10-18 03:05:15 PDT
Created attachment 381291 [details]
Example page
Comment 2 Simon Taylor 2019-10-18 03:06:15 PDT
There is a hosted example here for testing purposes:
https://tango-bravo.net/webkit-bug-203148/index.html

This works fine on Chrome, and shows the broken behaviour in Safari 13 on both MacOS and iOS.
Comment 3 Radar WebKit Bug Importer 2019-10-19 12:03:51 PDT
<rdar://problem/56436745>
Comment 4 Evan Odabashian 2019-11-13 16:27:21 PST
Just wanted to chime in that we're running into this issue as well.
Comment 5 Kimmo Kinnunen 2021-10-01 06:52:12 PDT
Thanks for the report. I can confirm the issue.
Comment 6 Kimmo Kinnunen 2021-12-14 05:37:01 PST
*** Bug 234230 has been marked as a duplicate of this bug. ***
Comment 7 Kimmo Kinnunen 2022-01-21 07:43:35 PST
Created attachment 449655 [details]
Patch
Comment 8 Kimmo Kinnunen 2022-01-21 09:10:18 PST
Created attachment 449664 [details]
Patch
Comment 9 Darin Adler 2022-01-21 11:17:57 PST
Comment on attachment 449664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449664&action=review

> Source/WebCore/platform/graphics/cv/GraphicsContextGLCVCocoa.cpp:611
> +    if (it != m_knownContent.end()) {
> +        if (it->value == content) {

Use && instead of nesting?

> Source/WebCore/platform/graphics/cv/GraphicsContextGLCVCocoa.h:71
> +        // TODO: Switch back to UnsafePointer<IOSurfaceRef> once UnsafePointer is safe to compare.

WebKit style uses FIXME for this. Means the same as TODO.

> Source/WebCore/platform/graphics/cv/GraphicsContextGLCVCocoa.h:81
> +        bool operator==(const TextureContent& other) const;

Remove the name "other" please. I also think you might want to add a blank line above this.
Comment 10 Kimmo Kinnunen 2022-01-24 00:53:53 PST
Created attachment 449786 [details]
Patch for landing
Comment 11 EWS 2022-01-24 03:53:55 PST
Committed r288434 (246323@main): <https://commits.webkit.org/246323@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449786 [details].
Comment 12 Simon Taylor 2022-02-28 02:22:45 PST
As a note for people watching this for information on when the fix lands in iOS Safari - this is fixed in iOS 15.4 beta 4.