Bug 228821 – webgl/1.0.x/conformance/textures/misc/texture-corner-case-videos.html fails on Cocoa

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kimmo Kinnunen]

webgl/1.0.x/conformance/textures/misc/texture-corner-case-videos.html fails on Cocoa

[Radar WebKit Bug Importer]

<rdar://problem/81562236>

[Kimmo Kinnunen]

Created attachment 434981 [details]
Patch

[Kenneth Russell]

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 .

[Kimmo Kinnunen]

Created attachment 435249 [details]
Patch

[Kimmo Kinnunen]

(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?

[Kimmo Kinnunen]

Links for posteriority:
https://github.com/KhronosGroup/WebGL/pull/2464
https://www.khronos.org/registry/webgl/specs/latest/1.0/#6.9
https://html.spec.whatwg.org/multipage/media.html#htmlvideoelement

https://github.com/KhronosGroup/WebGL/issues/3309

[Kenneth Russell]

Comment on attachment 435249 [details]
Patch

Looking good - let's see what's needed to get all the tests passing.

[Kimmo Kinnunen]

Created attachment 435338 [details]
Patch

[Kimmo Kinnunen]

Created attachment 435339 [details]
Patch

[Kenneth Russell]

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.

[Kimmo Kinnunen]

(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.

[EWS]

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].