Bug 228821 - webgl/1.0.x/conformance/textures/misc/texture-corner-case-videos.html fails on Cocoa
Summary: webgl/1.0.x/conformance/textures/misc/texture-corner-case-videos.html fails o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks: webgl2conformance
  Show dependency treegraph
 
Reported: 2021-08-05 05:19 PDT by Kimmo Kinnunen
Modified: 2021-09-22 12:54 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.20 KB, patch)
2021-08-05 06:28 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (8.36 KB, patch)
2021-08-10 02:24 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (8.48 KB, patch)
2021-08-11 05:43 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (8.64 KB, patch)
2021-08-11 05:56 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-08-05 05:19:27 PDT
webgl/1.0.x/conformance/textures/misc/texture-corner-case-videos.html fails on Cocoa
Comment 1 Radar WebKit Bug Importer 2021-08-05 05:19:57 PDT
<rdar://problem/81562236>
Comment 2 Kimmo Kinnunen 2021-08-05 06:28:56 PDT
Created attachment 434981 [details]
Patch
Comment 3 Kenneth Russell 2021-08-05 13:24:42 PDT
Comment on attachment 434981 [details]
Patch

I guess this fix was the one we've just discussed on Slack?

Let's finish the discussion there - not sure whether we should be using the natural size and scaling, or using the video's visible rectangle. The test doesn't seem to care about the size of the uploaded texture, only the contents of its various corners. I don't think Chrome scales up during texture upload per the latest changes in https://crbug.com/1175907 .
Comment 4 Kimmo Kinnunen 2021-08-10 02:24:45 PDT
Created attachment 435249 [details]
Patch
Comment 5 Kimmo Kinnunen 2021-08-10 02:27:38 PDT
(In reply to Kenneth Russell from comment #3)
> Comment on attachment 434981 [details]
> Patch
> 
> I guess this fix was the one we've just discussed on Slack?
> 
> Let's finish the discussion there - not sure whether we should be using the
> natural size and scaling, or using the video's visible rectangle.

Thanks for working on this.

How about now?
Comment 7 Kenneth Russell 2021-08-10 13:15:10 PDT
Comment on attachment 435249 [details]
Patch

Looking good - let's see what's needed to get all the tests passing.
Comment 8 Kimmo Kinnunen 2021-08-11 05:43:24 PDT
Created attachment 435338 [details]
Patch
Comment 9 Kimmo Kinnunen 2021-08-11 05:56:59 PDT
Created attachment 435339 [details]
Patch
Comment 10 Kenneth Russell 2021-08-11 12:39:41 PDT
Comment on attachment 435339 [details]
Patch

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

Looks good and pleasingly small! r+

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:-5780
> -    // FIXME: Turn this into a GPU-GPU texture copy instead of CPU readback.

Is this FIXME still relevant (to ImageBuffer::copyImage), or was the paintCurrentFrameInContext call the CPU readback? I don't remember.
Comment 11 Kimmo Kinnunen 2021-08-12 05:54:08 PDT
(In reply to Kenneth Russell from comment #10)
> Comment on attachment 435339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435339&action=review
> 
> Looks good and pleasingly small! r+
> 
> > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:-5780
> > -    // FIXME: Turn this into a GPU-GPU texture copy instead of CPU readback.
> 
> Is this FIXME still relevant (to ImageBuffer::copyImage), or was the
> paintCurrentFrameInContext call the CPU readback? I don't remember.

In practice, I think both.
This function is called for the purpose of being able to use CPU routines to process the data. I thought it's a bit redundant comment.

E.g. to process more stuff with GPU, new cases should be added to the existing GPU functionality that tries to run before this code runs..

ImageBuffer::copyImage() returns Image which is the processed with CPU routines.
Comment 12 EWS 2021-08-12 06:35:13 PDT
Committed r280963 (240469@main): <https://commits.webkit.org/240469@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435339 [details].