Bug 257452 – Row byte count is not available in GraphicsContextGL::computeImageSizeBytes

• Component: WebGL
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

Row byte count is not available in GraphicsContextGL::computeImageSizeBytes

Needed for GPUP implementation when computing readPixels pixel rectangle byte size

[Radar WebKit Bug Importer]

<rdar://problem/109969560>

[Kimmo Kinnunen]

Pull request: https://github.com/WebKit/WebKit/pull/14450

[EWS]

Committed 264794@main (19a2252db08c): <https://commits.webkit.org/264794@main>

Reviewed commits have been landed. Closing PR #14450 and removing active labels.

[Michael Catanzaro]

Why is the test for bmalloc::roundUpToMultipleOfNonPowerOfTwo guarded by #if !USE(CAIRO)? That seems useful to test unconditionally?

[Kimmo Kinnunen]

Wincairo doesn’t build with bmalloc in include path

[Michael Catanzaro]

Ah, because bmalloc does not work on Windows. I would change that to OS(WINDOWS) then. Cairo is used on Linux too, and this code has nothing to do with Cairo.

[Michael Catanzaro]

Oh, the other problem is bmalloc is just not in the include path on any platform. This broke JSCOnly builds.

[Kimmo Kinnunen]

> Oh, the other problem is bmalloc is just not in the include path on any platform. This broke JSCOnly builds.

I think it passed all the tested builds.

Would it be possible to close this bug and fix whatever other build configurations in a subsequent bug?

[Michael Catanzaro]

So the header that the test is trying to use is: Source/bmalloc/bmalloc/Algorithm.h. This directory is added to bmalloc_PRIVATE_INCLUDE_DIRECTORIES in Source/bmalloc/CMakeLists.txt in order to ensure it's not available to higher layers that depend on the bmalloc framework. We have no other tests that depend on bmalloc except for Tools/TestWebKitAPI/Tests/WTF/bmalloc/IsoHeap.cpp, which is never built because it's not added to any CMakeLists.txt (a common problem in TestWebKitAPI).

It's an intentional attempt to ensure that higher layers cannot use the bmalloc headers, but blocking ourselves from testing bmalloc does not seem useful. I see TestWebKit depends on many private framework headers:

    set(TestWebKit_PRIVATE_INCLUDE_DIRECTORIES
        ${CMAKE_BINARY_DIR}
        ${JavaScriptCore_PRIVATE_FRAMEWORK_HEADERS_DIR}
        ${PAL_FRAMEWORK_HEADERS_DIR}
        ${TESTWEBKITAPI_DIR}
        ${WebCore_PRIVATE_FRAMEWORK_HEADERS_DIR}
        ${WebKit_FRAMEWORK_HEADERS_DIR}
    )

but TestWebCore and TestWTF do not. Don, Fujii, is this intentional? Are you trying to remove them? OK for me to just add the required include dir to TestWTF? It would conspicuously be the only such include directory, which seems pretty suspicious.

(In reply to Michael Catanzaro from comment #6)
> Ah, because bmalloc does not work on Windows. I would change that to
> OS(WINDOWS) then. Cairo is used on Linux too, and this code has nothing to
> do with Cairo.

Let's try #if !USE(SYSTEM_MALLOC).

[Michael Catanzaro]

(In reply to Kimmo Kinnunen from comment #8)
> Would it be possible to close this bug and fix whatever other build
> configurations in a subsequent bug?

I don't care too much whether we use this bug or a new bug. I generally use the original bug for build failures because it's easier than reporting a new bug, and to keep discussion of the patch and its fix in one place.

[Michael Catanzaro]

(In reply to Kimmo Kinnunen from comment #8)
> I think it passed all the tested builds.

Problem is the JSCOnly builders are all macOS bots using XCode build. There are post-commit JSCOnly Linux bots that are currently green, but I'm not sure how that's possible. Maybe they are not building TestWTF?

[Michael Catanzaro]

OK, I've tried building locally to reproduce this problem and was unable to do so: the bmalloc include directory really does somehow get added to my JSCOnly build. So I'm just going to land a small follow-up to change the preprocessor guards.

I'll open a new bug for the missing include directory if I can figure out how to trigger it.

[Michael Catanzaro]

Pull request: https://github.com/WebKit/WebKit/pull/14695

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #12)
> I'll open a new bug for the missing include directory if I can figure out
> how to trigger it.

I figured it out: the build was failing when built with USE_SYSTEM_MALLOC because bmalloc was not built but was used unconditionally. So my pull request above is actually sufficient to fix things.

[EWS]

Committed 264902@main (bdfef46eb471): <https://commits.webkit.org/264902@main>

Reviewed commits have been landed. Closing PR #14695 and removing active labels.