Bug 237460 – [WinCairo] GraphicsContextGL should have one more output texture for double buffering WebGL

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

[Fujii Hironori]

[WinCairo] GraphicsContextGL should have one more output texture for double buffering WebGL

WinCairo's GraphicsContextGL has only a single output texture now.
The WebGL output can get broken if the page is interactive.
For example, Google Maps is using WebGL for the map.
If a user is drugging the map, the map becomes blank.
https://www.google.com/maps

[Fujii Hironori]

Created attachment 453808 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 453808 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:393
> +#define BACKING(backing) backing

FWIW, to me it would appear that the USE(COORDINATED_GRAPHICS) and !USE(COORDINATED_GRAPHICS) do not share that much in common.
Maybe it would make senes to just have two different functions or at least two different function contents, and just write the code linearly.
The BACKING macro and invoking it with sometimes undefined variable name looks very confusing.

[Fujii Hironori]

Comment on attachment 453808 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:393
>> +#define BACKING(backing) backing
> 
> FWIW, to me it would appear that the USE(COORDINATED_GRAPHICS) and !USE(COORDINATED_GRAPHICS) do not share that much in common.
> Maybe it would make senes to just have two different functions or at least two different function contents, and just write the code linearly.
> The BACKING macro and invoking it with sometimes undefined variable name looks very confusing.

Thank you for the comment.
But, I don't think they are doing different things.
!USE(COORDINATED_GRAPHICS) resizes two textures while USE(COORDINATED_GRAPHICS) resizes three textures or sets backings.
And, they are sharing other code, for example initializing and finalizing m_compositorTexture.

[Zan Dobersek]

Comment on attachment 453808 [details]
Patch

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

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:554
> -#if USE(COORDINATED_GRAPHICS)
> +#if !PLATFORM(COCOA)

What if you use `USE(TEXTURE_MAPPER) || USE(COORDINATED_GRAPHICS)` along with guarding the extra swaps, like you've done below?

>>> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:393
>>> +#define BACKING(backing) backing
>> 
>> FWIW, to me it would appear that the USE(COORDINATED_GRAPHICS) and !USE(COORDINATED_GRAPHICS) do not share that much in common.
>> Maybe it would make senes to just have two different functions or at least two different function contents, and just write the code linearly.
>> The BACKING macro and invoking it with sometimes undefined variable name looks very confusing.
> 
> Thank you for the comment.
> But, I don't think they are doing different things.
> !USE(COORDINATED_GRAPHICS) resizes two textures while USE(COORDINATED_GRAPHICS) resizes three textures or sets backings.
> And, they are sharing other code, for example initializing and finalizing m_compositorTexture.

It's hard to grasp what's going on here, and it's going to change soon for USE(COORDINATED_GRAPHICS) anyway.

Better duplicate the code for now in something like the following block:
```
#if USE(COORDINATED_GRAPHICS)
...
#elif USE(TEXTURE_MAPPER)
...
#endif
```

[Fujii Hironori]

Comment on attachment 453808 [details]
Patch

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

>> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:554
>> +#if !PLATFORM(COCOA)
> 
> What if you use `USE(TEXTURE_MAPPER) || USE(COORDINATED_GRAPHICS)` along with guarding the extra swaps, like you've done below?

Got it. I will revise. But, if I change here, I have to change the macro for m_compositorTexture definition in GraphicsContextGLANGLE.h for the consistency.

>>>> Source/WebCore/platform/graphics/texmap/GraphicsContextGLTextureMapperANGLE.cpp:393
>>>> +#define BACKING(backing) backing
>>> 
>>> FWIW, to me it would appear that the USE(COORDINATED_GRAPHICS) and !USE(COORDINATED_GRAPHICS) do not share that much in common.
>>> Maybe it would make senes to just have two different functions or at least two different function contents, and just write the code linearly.
>>> The BACKING macro and invoking it with sometimes undefined variable name looks very confusing.
>> 
>> Thank you for the comment.
>> But, I don't think they are doing different things.
>> !USE(COORDINATED_GRAPHICS) resizes two textures while USE(COORDINATED_GRAPHICS) resizes three textures or sets backings.
>> And, they are sharing other code, for example initializing and finalizing m_compositorTexture.
> 
> It's hard to grasp what's going on here, and it's going to change soon for USE(COORDINATED_GRAPHICS) anyway.
> 
> Better duplicate the code for now in something like the following block:
> ```
> #if USE(COORDINATED_GRAPHICS)
> ...
> #elif USE(TEXTURE_MAPPER)
> ...
> #endif
> ```

Got it. I will revise with duplicating approach. But, I thiknk USE(TEXTURE_MAPPER)) isn't needed here.

[Fujii Hironori]

Created attachment 454020 [details]
Patch

[Zan Dobersek (Reviews)]

Comment on attachment 454020 [details]
Patch

Awesome, thanks.

[Fujii Hironori]

Comment on attachment 454020 [details]
Patch

Thank you for the review.

[EWS]

Committed r290980 (248158@main): <https://commits.webkit.org/248158@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/89954884>