Bug 170730 – REGRESSION(r215211): [GTK] Several webgl related tests are failing

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

These are the tests failing

fast/canvas/webgl/gl-teximage.html [ Failure ]
fast/canvas/webgl/oes-texture-half-float-with-image.html [ Failure ]
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgb565.html [ Failure ]
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba4444.html [ Failure ]
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image-rgba5551.html [ Failure ]
fast/canvas/webgl/tex-image-and-sub-image-2d-with-image.html [ Failure ]
fast/canvas/webgl/tex-image-and-sub-image-2d-with-potentially-subsampled-image.html [ Failure ]
fast/canvas/webgl/tex-image-with-format-and-type.html [ Failure ]
fast/canvas/webgl/tex-image-with-greyscale-image.html [ Failure ]
fast/canvas/webgl/texture-color-profile.html [ Failure ]
fast/canvas/webgl/texture-transparent-pixels-initialized.html [ Failure ]
fast/images/webgl-teximage2d.html [ Failure ]
http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html [ Failure ]
http/tests/security/webgl-remote-read-remote-image-allowed.html [ Failure ]
http/tests/webgl/1.0.2/texImage2DHTML.html [ Failure ]
http/tests/webgl/1.0.2/texSubImage2DHTML.html [ Failure ]
webgl/1.0.2/conformance/extensions/oes-texture-float-with-image.html [ Failure ]
webgl/1.0.2/conformance/textures/gl-teximage.html [ Failure ]
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-rgb565.html [ Failure ]
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-rgba4444.html [ Failure ]
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image-rgba5551.html [ Failure ]
webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-image.html [ Failure ]
webgl/1.0.2/conformance/textures/tex-image-with-format-and-type.html [ Failure ]
webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-image.html [ Failure ]

[Miguel Gomez]

The problem seems to be that setting the data to a decoder using ImageDecoder::setData() is setting m_isAllDataReceived to true. Due to this, ImageDecoder::encodedDataStatus() returns EncodedDataStatus::Complete and no size is set to the decoder.

[Miguel Gomez]

(In reply to Miguel Gomez from comment #1)
> The problem seems to be that setting the data to a decoder using
> ImageDecoder::setData() is setting m_isAllDataReceived to true. Due to this,
> ImageDecoder::encodedDataStatus() returns EncodedDataStatus::Complete and no
> size is set to the decoder.

The enumeration for EncodedDataStatus doesn't have a value to indicate that the data was set but the size is not available yet. As ImageDecoder::encodedDataStatus() is implemented now, having all the data set returns EncodedDataStatus::Complete, which means that the size is already set, which may not be true.

[Miguel Gomez]

Created attachment 306900 [details]
Patch

[Carlos Garcia Campos]

Please check if Complete doesn't really mean that decode is complete not that all encoded data is available.

[Miguel Gomez]

(In reply to Carlos Garcia Campos from comment #4)
> Please check if Complete doesn't really mean that decode is complete not
> that all encoded data is available.

Said, could you shed some light on this? What's the exact meaning of EncodedDataStatus::Complete? Does it just mean that all the the encoded data has been received, or does it also mean that the decoding process is complete?

[Michael Catanzaro]

The fact that you're asking this question indicates that there needs to be a comment in the code.

[Carlos Alberto Lopez Perez]

(In reply to Miguel Gomez from comment #5)
> (In reply to Carlos Garcia Campos from comment #4)
> > Please check if Complete doesn't really mean that decode is complete not
> > that all encoded data is available.
> 
> Said, could you shed some light on this? What's the exact meaning of
> EncodedDataStatus::Complete? Does it just mean that all the the encoded data
> has been received, or does it also mean that the decoding process is
> complete?

Adding said to the CC

[Miguel Gomez]

(In reply to Carlos Alberto Lopez Perez from comment #7)
> (In reply to Miguel Gomez from comment #5)
> > (In reply to Carlos Garcia Campos from comment #4)
> > > Please check if Complete doesn't really mean that decode is complete not
> > > that all encoded data is available.
> > 
> > Said, could you shed some light on this? What's the exact meaning of
> > EncodedDataStatus::Complete? Does it just mean that all the the encoded data
> > has been received, or does it also mean that the decoding process is
> > complete?
> 
> Adding said to the CC

Oh, my fault, I forgot to add him to the CC :(

[Said Abou-Hallawa]

(In reply to Miguel Gomez from comment #5)
> (In reply to Carlos Garcia Campos from comment #4)
> > Please check if Complete doesn't really mean that decode is complete not
> > that all encoded data is available.
> 
> Said, could you shed some light on this? What's the exact meaning of
> EncodedDataStatus::Complete? Does it just mean that all the the encoded data
> has been received, or does it also mean that the decoding process is
> complete?

It means all the encoded data has been received.

Please remember decoding the image frames is an ongoing process; it does not finish. The image frames can be deleted and re-decoded. The only part we have to decode while receiving the encoded data is the part that makes us know the image size. This part of the encoded data is decoded only once. After knowing the size of the image, no further decoding will be done until an image frame is drawn.

[Said Abou-Hallawa]

Comment on attachment 306900 [details]
Patch

I do not think this is the right fix for this bug.

-- A quick fix is the following:
1. Change back the ImageDecoder:: isSizeAvailable() 

<< bool isSizeAvailable() { return encodedDataStatus() >= EncodedDataStatus::SizeAvailable; }
>> bool isSizeAvailable() { return m_sizeAvailable; }

2. Change back the condition in all the decoders encodedDataStatus() functions:

EncodedDataStatus JPEGImageDecoder::encodedDataStatus() const
{
<<    if (ImageDecoder::encodedDataStatus() < EncodedDataStatus::SizeAvailable)
>>    if (ImageDecoder::isSizeAvailable())

-- A better but more involving fix is the following:

1. Replace the ImageDecoder members m_failed, m_sizeAvailable, m_isAllDataReceived by a single member named m_encodedDataStatus. Initialize it with EncodedDataStatus::TypeAvailable.

2. Make ImageDecoder::setData() fixes the m_encodedDataStatus if its value is  EncodedDataStatus::TypeAvailable or allDataReceived is true.

        virtual void setData(SharedBuffer& data, bool allDataReceived)
        {
            if (m_encodedDataStatus == EncodedDataStatus::Error)
                return;

            m_data = &data;
            if (m_encodedDataStatus == EncodedDataStatus::TypeAvailable)
                decode();
            
            if (m_encodedDataStatus == EncodedDataStatus::Error)
                return;

            if (allDataReceived) {
                ASSERT(m_encodedDataStatus == EncodedDataStatus:: SizeAvailable);
                m_encodedDataStatus == EncodedDataStatus::Complete;
           }
        }

3. Add a pure virtual function named decode() in ImageDecoder. And implement it in all the decoders. For example GIFImageDecoder will have the following implementation: 

void decode() override { decode(0, GIFSizeQuery); }

4. Delete all the implementations of encodedDataStatus() and make ImageDecoder:: encodedDataStatus() not virtual and make it returns the member m_encodedDataStatus.

5. Change ImageDecoder methods setFailed() and fail(), setSize(),  encodedDataStatus() and isSizeAvailable() to use directly the member m_encodedDataStatus.

EncodedDataStatus encodedDataStatus() const { return m_encodedDataStatus; }
bool failed() const { return m_encodedDataStatus == EncodedDataStatus::Error; }
bool isSizeAvailable() { return m_encodedDataStatus >= EncodedDataStatus::SizeAvailable; }

[Miguel Gomez]

> I do not think this is the right fix for this bug.
> 
> -- A quick fix is the following:

Thanks for the tips Said. I'll work on it :)

[Miguel Gomez]

Created attachment 308238 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 308238 [details]
Patch

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

Apart from the naming and making it protected, the patch looks good to me, but I think it's would be better if Said also took a look at it. Thanks!

> Source/WebCore/platform/image-decoders/ImageDecoder.h:71
> +    virtual void decode() = 0;

I think this is a bit confusing, because this actually means decode until you get the size. So, I would rename it to make it clear this doesn't decode the whole image. I also assume that this is async, can be called with partial data and can fail if don't have enough data yet, right? Based on that assumptions I would call this startDecodingUntilSizeIsAvailable() or tryDecodingUntilSizeIsAvailable() or something similar. I would also make it protected, since this is only used inside setData().

[Build Bot]

Comment on attachment 308238 [details]
Patch

Attachment 308238 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3609500

New failing tests:
http/tests/websocket/tests/hybi/workers/worker-reload.html

[Build Bot]

Created attachment 308241 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308238 [details]
Patch

Attachment 308238 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3609587

New failing tests:
compositing/absolute-inside-out-of-view-fixed.html

[Build Bot]

Created attachment 308243 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Miguel Gomez]

Created attachment 308245 [details]
Patch

[Miguel Gomez]

> > Source/WebCore/platform/image-decoders/ImageDecoder.h:71
> > +    virtual void decode() = 0;
> 
> I think this is a bit confusing, because this actually means decode until
> you get the size. So, I would rename it to make it clear this doesn't decode
> the whole image. I also assume that this is async, can be called with
> partial data and can fail if don't have enough data yet, right? Based on
> that assumptions I would call this startDecodingUntilSizeIsAvailable() or
> tryDecodingUntilSizeIsAvailable() or something similar. I would also make it
> protected, since this is only used inside setData().

I renamed the method to tryDecodeSize and made it private. I'll wait for Said to give the thumbs up before setting cq+

[Said Abou-Hallawa]

Comment on attachment 308238 [details]
Patch

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

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:71
>> +    virtual void decode() = 0;
> 
> I think this is a bit confusing, because this actually means decode until you get the size. So, I would rename it to make it clear this doesn't decode the whole image. I also assume that this is async, can be called with partial data and can fail if don't have enough data yet, right? Based on that assumptions I would call this startDecodingUntilSizeIsAvailable() or tryDecodingUntilSizeIsAvailable() or something similar. I would also make it protected, since this is only used inside setData().

Calling the decoder with partial data should not make it fails. When all the data is not received, the decoder consume  as much it can to answer the caller request. If the data is not complete, it may not set the size of the image or it may return an incomplete frame. But in any case it should not fail because of incomplete data.

[Said Abou-Hallawa]

Comment on attachment 308245 [details]
Patch

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

> Source/WebCore/platform/image-decoders/ImageDecoder.h:73
> +    bool isAllDataReceived() const { return m_encodedDataStatus == EncodedDataStatus::Complete; }

There is a potential bug if this function is called form inside the decode() functions. ImageDecoder::setData() calls tryDecodeSize() and later it sets m_encodedDataStatus = EncodedDataStatus::Complete. So the decode() can call isAllDataReceived() and it returns false although setData received allDataReceived = true. Luckily when searching for who calls isAllDataReceived(), I found out they are the decode() functions. So I would suggest the following:

1. Change tryDecodeSize() and all the decode() functions to take an argument named allDataReceived.
2. Replace all the calls inside the decode() functions to isAllDataReceived() with the new argument allDataReceived.
3. If decode() is called from inside setData(), the argument allDataReceived is passed. Otherwise, pass isAllDataReceived() instead.

isAllDataReceived() should return true only if all the data is received AND no decoding error happens. That is why checking failed() should happen before calling isAllDataReceived(). Calling isAllDataReceived() while failed() is true is just meaningless.

[Miguel Gomez]

Created attachment 308372 [details]
Patch

[Miguel Gomez]

(In reply to Miguel Gomez from comment #22)
> Created attachment 308372 [details]
> Patch

A couple of comments about the patch:

* ICOImageDecoder::decode() calls decodeAtAtindex(), which calls setDataForPNGDecoderAtIndex(), which uses isAllDataReceived() and the not the allDataReceived value passed to decode. I haven't changed this cause decodeAtIndex() is not called when we are just requesting the size (from inside ImageDecoder::setData), so it should not be a problem. I can change it if you prefer, adding the allDataReceived parameter to those calls as well.

* WEBPImageDecoder doesn't use isAllDataReceived() at all. I've added the parameter to decode() anyway to be coherent with the rest of the decoders, but marked it as unused.

[Said Abou-Hallawa]

Comment on attachment 308372 [details]
Patch

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

> Source/WebCore/platform/image-decoders/ImageDecoder.h:73
> +    bool isAllDataReceived() const { return m_encodedDataStatus == EncodedDataStatus::Complete; }

Since setData() is the only place which sets m_encodedDataStatus to EncodedDataStatus::Complete, I think we should enforce all the callee of setData() to not call this function at least through an assertion in this function

ASSERT(!m_inSetData);
return m_encodedDataStatus == EncodedDataStatus::Complete;

Or something similar.

> Source/WebCore/platform/image-decoders/webp/WEBPImageDecoder.cpp:81
> +bool WEBPImageDecoder::decode(bool onlySize, bool allDataReceived)

I think we usually omit the argument if it is not used. It is okay to keep it in the header file though especially with built-in types.
bool WEBPImageDecoder::decode(bool onlySize, bool)

[Said Abou-Hallawa]

(In reply to Miguel Gomez from comment #23)
> (In reply to Miguel Gomez from comment #22)
> > Created attachment 308372 [details]
> > Patch
> 
> A couple of comments about the patch:
> 
> * ICOImageDecoder::decode() calls decodeAtAtindex(), which calls
> setDataForPNGDecoderAtIndex(), which uses isAllDataReceived() and the not
> the allDataReceived value passed to decode. I haven't changed this cause
> decodeAtIndex() is not called when we are just requesting the size (from
> inside ImageDecoder::setData), so it should not be a problem. I can change
> it if you prefer, adding the allDataReceived parameter to those calls as
> well.
> 

I think it's up to you. If you add an assertion in ImageDecoder::isAllDataReceived() this should be enough.

> * WEBPImageDecoder doesn't use isAllDataReceived() at all. I've added the
> parameter to decode() anyway to be coherent with the rest of the decoders,
> but marked it as unused.

[Miguel Gomez]

Created attachment 308514 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308514 [details]
Patch

Clearing flags on attachment: 308514

Committed r215924: <http://trac.webkit.org/changeset/215924>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.