Changeset 243681 in webkit


Ignore:
Timestamp:
Mar 30, 2019, 3:07:21 PM (6 years ago)
Author:
dino@apple.com
Message:

gl.readPixels with type gl.FLOAT does not work
​https://bugs.webkit.org/show_bug.cgi?id=171432
<rdar://problem/31905150>

Reviewed by Antoine Quint.

Source/WebCore:

Our validation code was identifying readPixels of
type FLOAT as invalid, for three reasons:

  • we didn't support the FLOAT type at all.
  • we only allowed the combination of RGBA and

UNSIGNED_BYTE in WebGL 1 [*].

  • if we had a framebuffer of format RGBA, we assumed

we could only read into a Uint8 ArrayBuffer.

[*] This bug isn't completely fixed, so I opened
​https://bugs.webkit.org/show_bug.cgi?id=196418

Test: fast/canvas/webgl/readPixels-float.html

  • html/canvas/WebGLRenderingContextBase.cpp:

(WebCore::WebGLRenderingContextBase::readPixels):

  • flip the logic in a conditional that was clearly wrong yet thankfully had no impact.
  • support type FLOAT when the relevant extension is enabled.
  • allow FLOAT as a valid type (see new bug above)
  • create a new macro for CHECK_COMPONENT_COUNT
  • update the existing macros to not be case statements, so that we can put logic in the switch.

LayoutTests:

New test that exercises reading a framebuffer object
with a floating point texture attached.

  • platform/ios/TestExpectations: Skip this test on iOS, where floating-point

FBOs are not supported.

  • fast/canvas/webgl/readPixels-float-expected.txt: Added.
  • fast/canvas/webgl/readPixels-float.html: Added.
Location:
trunk
Files:
2 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r243678 r243681  
     12019-03-29  Dean Jackson  <dino@apple.com>
     2
     3        gl.readPixels with type gl.FLOAT does not work
     4        https://bugs.webkit.org/show_bug.cgi?id=171432
     5        <rdar://problem/31905150>
     6
     7        Reviewed by Antoine Quint.
     8
     9        New test that exercises reading a framebuffer object
     10        with a floating point texture attached.
     11
     12        * platform/ios/TestExpectations: Skip this test on iOS, where floating-point
     13        FBOs are not supported.
     14        * fast/canvas/webgl/readPixels-float-expected.txt: Added.
     15        * fast/canvas/webgl/readPixels-float.html: Added.
     16
    1172019-03-30  Zalan Bujtas  <zalan@apple.com>
    218

  • trunk/LayoutTests/platform/ios/TestExpectations

    r243666 r243681  
    27662766fast/canvas/webgl/texImage2D-video-flipY-true.html [ Skip ]
    27672767
     2768# rendering to floating point FBOs not supported on iOS
     2769fast/canvas/webgl/readPixels-float.html [ Skip ]
     2770
    27682771# Skipped on iOS since UIHelper.activateAt() doesn't produce a user gesture that ITP captures on iOS
    27692772webkit.org/b/174120 http/tests/resourceLoadStatistics/user-interaction-in-cross-origin-sub-frame.html [ Skip ]

  • trunk/Source/WebCore/ChangeLog

    r243680 r243681  
     12019-03-29  Dean Jackson  <dino@apple.com>
     2
     3        gl.readPixels with type gl.FLOAT does not work
     4        https://bugs.webkit.org/show_bug.cgi?id=171432
     5        <rdar://problem/31905150>
     6
     7        Reviewed by Antoine Quint.
     8
     9        Our validation code was identifying readPixels of
     10        type FLOAT as invalid, for three reasons:
     11        - we didn't support the FLOAT type at all.
     12        - we only allowed the combination of RGBA and
     13        UNSIGNED_BYTE in WebGL 1 [*].
     14        - if we had a framebuffer of format RGBA, we assumed
     15        we could only read into a Uint8 ArrayBuffer.
     16
     17        [*] This bug isn't completely fixed, so I opened
     18        https://bugs.webkit.org/show_bug.cgi?id=196418
     19
     20        Test: fast/canvas/webgl/readPixels-float.html
     21
     22        * html/canvas/WebGLRenderingContextBase.cpp:
     23        (WebCore::WebGLRenderingContextBase::readPixels):
     24        - flip the logic in a conditional that was clearly wrong yet
     25          thankfully had no impact.
     26        - support type FLOAT when the relevant extension is enabled.
     27        - allow FLOAT as a valid type (see new bug above)
     28        - create a new macro for CHECK_COMPONENT_COUNT
     29        - update the existing macros to not be case statements,
     30          so that we can put logic in the switch.
     31
    1322019-03-30  Antti Koivisto  <antti@apple.com>
    233

  • trunk/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp

    r243506 r243681  
    34503450    } else {
    34513451        if (m_attributes.alpha)
     3452            internalFormat = GraphicsContext3D::RGBA8;
     3453        else
    34523454            internalFormat = GraphicsContext3D::RGB8;
    3453         else
    3454             internalFormat = GraphicsContext3D::RGBA8;
    34553455    }
    34563456
    … …  
    34763476        case GraphicsContext3D::UNSIGNED_SHORT_5_5_5_1:
    34773477            break;
     3478        case GraphicsContext3D::FLOAT:
     3479            if (!m_oesTextureFloat && !m_oesTextureHalfFloat) {
     3480                synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "invalid type");
     3481                return;
     3482            }
     3483            break;
    34783484        default:
    34793485            synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "invalid type");
    34803486            return;
    34813487        }
    3482         if (format != GraphicsContext3D::RGBA || type != GraphicsContext3D::UNSIGNED_BYTE) {
    3483             synthesizeGLError(GraphicsContext3D::INVALID_OPERATION, "readPixels", "format not RGBA or type not UNSIGNED_BYTE");
     3488        if (format != GraphicsContext3D::RGBA || (type != GraphicsContext3D::UNSIGNED_BYTE && type != GraphicsContext3D::FLOAT)) {
     3489            synthesizeGLError(GraphicsContext3D::INVALID_OPERATION, "readPixels", "format not RGBA or type not UNSIGNED_BYTE|FLOAT");
    34843490            return;
    34853491        }
    … …  
    34933499    }
    34943500
    3495 #define INTERNAL_FORMAT_CHECK(themeMacro, typeMacro, pixelTypeMacro) case InternalFormatTheme::themeMacro: \
    3496         if (type != GraphicsContext3D::typeMacro || pixels.getType() != JSC::pixelTypeMacro) { \
    3497             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
    3498             return; \
    3499         } \
    3500         if (format != GraphicsContext3D::RED && format != GraphicsContext3D::RG && format != GraphicsContext3D::RGB && format != GraphicsContext3D::RGBA) { \
    3501             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Unknown format"); \
    3502             return; \
    3503         } \
    3504         if (numberOfComponentsForFormat(format) < internalFormatComponentCount) { \
    3505             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Not enough components in format"); \
    3506             return; \
    3507         } \
    3508         break;
    3509 
    3510 #define INTERNAL_FORMAT_INTEGER_CHECK(themeMacro, typeMacro, pixelTypeMacro) case InternalFormatTheme::themeMacro: \
    3511         if (type != GraphicsContext3D::typeMacro || pixels.getType() != JSC::pixelTypeMacro) { \
    3512             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
    3513             return; \
    3514         } \
    3515         if (format != GraphicsContext3D::RED_INTEGER && format != GraphicsContext3D::RG_INTEGER && format != GraphicsContext3D::RGB_INTEGER && format != GraphicsContext3D::RGBA_INTEGER) { \
    3516             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Unknown format"); \
    3517             return; \
    3518         } \
    3519         if (numberOfComponentsForFormat(format) < internalFormatComponentCount) { \
    3520             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Not enough components in format"); \
    3521             return; \
    3522         } \
    3523         break;
    3524 
    3525 #define PACKED_INTERNAL_FORMAT_CHECK(internalFormatMacro, formatMacro, type0Macro, pixelType0Macro, type1Macro, pixelType1Macro) case GraphicsContext3D::internalFormatMacro: \
    3526         if (!(type == GraphicsContext3D::type0Macro && pixels.getType() == JSC::pixelType0Macro) \
    3527             && !(type == GraphicsContext3D::type1Macro && pixels.getType() == JSC::pixelType1Macro)) { \
    3528             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
    3529             return; \
    3530         } \
    3531         if (format != GraphicsContext3D::formatMacro) { \
    3532             synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Invalid format"); \
    3533             return; \
    3534         } \
    3535         break;
     3501#define CHECK_COMPONENT_COUNT \
     3502    if (numberOfComponentsForFormat(format) < internalFormatComponentCount) { \
     3503        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Not enough components in format"); \
     3504        return; \
     3505    }
     3506
     3507#define INTERNAL_FORMAT_CHECK(typeMacro, pixelTypeMacro) \
     3508    if (type != GraphicsContext3D::typeMacro || pixels.getType() != JSC::pixelTypeMacro) { \
     3509        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
     3510        return; \
     3511    } \
     3512    if (format != GraphicsContext3D::RED && format != GraphicsContext3D::RG && format != GraphicsContext3D::RGB && format != GraphicsContext3D::RGBA) { \
     3513        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Unknown format"); \
     3514        return; \
     3515    } \
     3516    CHECK_COMPONENT_COUNT
     3517
     3518#define INTERNAL_FORMAT_INTEGER_CHECK(typeMacro, pixelTypeMacro) \
     3519    if (type != GraphicsContext3D::typeMacro || pixels.getType() != JSC::pixelTypeMacro) { \
     3520        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
     3521        return; \
     3522    } \
     3523    if (format != GraphicsContext3D::RED_INTEGER && format != GraphicsContext3D::RG_INTEGER && format != GraphicsContext3D::RGB_INTEGER && format != GraphicsContext3D::RGBA_INTEGER) { \
     3524        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Unknown format"); \
     3525        return; \
     3526    } \
     3527    CHECK_COMPONENT_COUNT
     3528
     3529#define CASE_PACKED_INTERNAL_FORMAT_CHECK(internalFormatMacro, formatMacro, type0Macro, pixelType0Macro, type1Macro, pixelType1Macro) case GraphicsContext3D::internalFormatMacro: \
     3530    if (!(type == GraphicsContext3D::type0Macro && pixels.getType() == JSC::pixelType0Macro) \
     3531        && !(type == GraphicsContext3D::type1Macro && pixels.getType() == JSC::pixelType1Macro)) { \
     3532        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "type does not match internal format"); \
     3533        return; \
     3534    } \
     3535    if (format != GraphicsContext3D::formatMacro) { \
     3536        synthesizeGLError(GraphicsContext3D::INVALID_ENUM, "readPixels", "Invalid format"); \
     3537        return; \
     3538    } \
     3539    break;
    35363540
    35373541    switch (internalFormatTheme) {
    3538     INTERNAL_FORMAT_CHECK        (NormalizedFixedPoint      , UNSIGNED_BYTE, TypeUint8  );
    3539     INTERNAL_FORMAT_CHECK        (SignedNormalizedFixedPoint, BYTE         , TypeInt8   );
    3540     INTERNAL_FORMAT_CHECK        (FloatingPoint             , FLOAT        , TypeFloat32);
    3541     INTERNAL_FORMAT_INTEGER_CHECK(SignedInteger             , INT          , TypeInt32  );
    3542     INTERNAL_FORMAT_INTEGER_CHECK(UnsignedInteger           , UNSIGNED_INT , TypeUint32 );
     3542    case InternalFormatTheme::NormalizedFixedPoint:
     3543        if (type == GraphicsContext3D::FLOAT) {
     3544            INTERNAL_FORMAT_CHECK(FLOAT, TypeFloat32);
     3545        } else {
     3546            INTERNAL_FORMAT_CHECK(UNSIGNED_BYTE, TypeUint8);
     3547        }
     3548        break;
     3549    case InternalFormatTheme::SignedNormalizedFixedPoint:
     3550        INTERNAL_FORMAT_CHECK(BYTE, TypeInt8);
     3551        break;
     3552    case InternalFormatTheme::FloatingPoint:
     3553        INTERNAL_FORMAT_CHECK(FLOAT, TypeFloat32);
     3554        break;
     3555    case InternalFormatTheme::SignedInteger:
     3556        INTERNAL_FORMAT_INTEGER_CHECK(INT, TypeInt32);
     3557        break;
     3558    case InternalFormatTheme::UnsignedInteger:
     3559        INTERNAL_FORMAT_INTEGER_CHECK(UNSIGNED_INT, TypeUint32);
     3560        break;
    35433561    case InternalFormatTheme::Packed:
    35443562        switch (internalFormat) {
    3545         PACKED_INTERNAL_FORMAT_CHECK(RGB565        , RGB         , UNSIGNED_SHORT_5_6_5        , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
    3546         PACKED_INTERNAL_FORMAT_CHECK(RGB5_A1       , RGBA        , UNSIGNED_SHORT_5_5_5_1      , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
    3547         PACKED_INTERNAL_FORMAT_CHECK(RGBA4         , RGBA        , UNSIGNED_SHORT_4_4_4_4      , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
    3548         PACKED_INTERNAL_FORMAT_CHECK(RGB9_E5       , RGB         , UNSIGNED_INT_5_9_9_9_REV    , TypeUint32, UNSIGNED_INT_5_9_9_9_REV   , TypeUint32 );
    3549         PACKED_INTERNAL_FORMAT_CHECK(RGB10_A2      , RGBA        , UNSIGNED_INT_2_10_10_10_REV , TypeUint32, UNSIGNED_INT_2_10_10_10_REV, TypeUint32 );
    3550         PACKED_INTERNAL_FORMAT_CHECK(R11F_G11F_B10F, RGB         , UNSIGNED_INT_10F_11F_11F_REV, TypeUint32, FLOAT                      , TypeFloat32);
    3551         PACKED_INTERNAL_FORMAT_CHECK(RGB10_A2UI    , RGBA_INTEGER, UNSIGNED_INT_2_10_10_10_REV , TypeUint32, UNSIGNED_INT_2_10_10_10_REV, TypeUint32 );
     3563            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB565        , RGB         , UNSIGNED_SHORT_5_6_5        , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
     3564            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB5_A1       , RGBA        , UNSIGNED_SHORT_5_5_5_1      , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
     3565            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGBA4         , RGBA        , UNSIGNED_SHORT_4_4_4_4      , TypeUint16, UNSIGNED_BYTE              , TypeUint8  );
     3566            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB9_E5       , RGB         , UNSIGNED_INT_5_9_9_9_REV    , TypeUint32, UNSIGNED_INT_5_9_9_9_REV   , TypeUint32 );
     3567            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB10_A2      , RGBA        , UNSIGNED_INT_2_10_10_10_REV , TypeUint32, UNSIGNED_INT_2_10_10_10_REV, TypeUint32 );
     3568            CASE_PACKED_INTERNAL_FORMAT_CHECK(R11F_G11F_B10F, RGB         , UNSIGNED_INT_10F_11F_11F_REV, TypeUint32, FLOAT                      , TypeFloat32);
     3569            CASE_PACKED_INTERNAL_FORMAT_CHECK(RGB10_A2UI    , RGBA_INTEGER, UNSIGNED_INT_2_10_10_10_REV , TypeUint32, UNSIGNED_INT_2_10_10_10_REV, TypeUint32 );
    35523570        }
    35533571        break;
    … …  
    35553573        ASSERT_NOT_REACHED();
    35563574    }
     3575#undef CHECK_COMPONENT_COUNT
    35573576#undef INTERNAL_FORMAT_CHECK
    35583577#undef INTERNAL_FORMAT_INTEGER_CHECK
    3559 #undef PACKED_INTERNAL_FORMAT_CHECK
     3578#undef CASE_PACKED_INTERNAL_FORMAT_CHECK
    35603579
    35613580    // Calculate array size, taking into consideration of PACK_ALIGNMENT.
Note: See TracChangeset for help on using the changeset viewer.