| Summary: | [WinCairo] GraphicsContextGL should have one more output texture for double buffering WebGL | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||
| Component: | WebGL | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cmarcelo, dino, don.olmstead, ews-watchlist, kbr, kkinnunen, kondapallykalyan, luiz, webkit-bug-importer, zan, zdobersek | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Fujii Hironori
2022-03-03 20:09:13 PST
Created attachment 453808 [details]
Patch
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. 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. 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 ``` 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. Created attachment 454020 [details]
Patch
Comment on attachment 454020 [details]
Patch
Awesome, thanks.
Comment on attachment 454020 [details]
Patch
Thank you for the review.
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]. |